On Thu, 18 Jan 2018 18:02:40 +0100 Cornelia Huck <coh...@redhat.com> wrote:
> On Thu, 18 Jan 2018 17:52:29 +0100 > Claudio Imbrenda <imbre...@linux.vnet.ibm.com> wrote: > > > On Thu, 18 Jan 2018 17:20:34 +0100 > > Cornelia Huck <coh...@redhat.com> wrote: > > > > > On Mon, 15 Jan 2018 17:52:02 +0100 > > > Claudio Imbrenda <imbre...@linux.vnet.ibm.com> wrote: > > > > > > > Fix storage attribute migration so that it does not fail for > > > > guests with more than a few GB of RAM. Migration itself was > > > > successful, but storage attributes were not migrated completely. > > > > > > > > This patch fixes the migration of all storage attributes, even > > > > when the guest have large amounts of memory. > > > > > > > > Signed-off-by: Claudio Imbrenda <imbre...@linux.vnet.ibm.com> > > > > Fixes: 903fd80b03243476 ("s390x/migration: Storage attributes > > > > device") --- > > > > hw/s390x/s390-stattrib-kvm.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/s390x/s390-stattrib-kvm.c > > > > b/hw/s390x/s390-stattrib-kvm.c index 41770a7..480551c 100644 > > > > --- a/hw/s390x/s390-stattrib-kvm.c > > > > +++ b/hw/s390x/s390-stattrib-kvm.c > > > > @@ -116,7 +116,7 @@ static void > > > > kvm_s390_stattrib_synchronize(S390StAttribState *sa) for (cx = > > > > 0; cx + len <= max; cx += len) { clog.start_gfn = cx; > > > > clog.count = len; > > > > - clog.values = (uint64_t)(sas->incoming_buffer + cx > > > > * len); > > > > > > Hm, doesn't that even imply that you reference an area beyond the > > > buffer, as the <= max check does not catch this? > > > > what do you mean? > > > > cx + len <= max catches the cases where you would write beyond the > > end of the buffer. if cx + len == max then we are filling the > > buffer to the last byte. and we will get out at the next > > iteration. > > Yes, but the problem is that your offset is too long, isn't it? (Where > cx + len <= max, but you use an offset of cx * len which may be > > max.) which is exactly why I'm removing that line. look at the very beginning of the line, there is a - the replacement line (the one that starts with a +) has only cx > But maybe I'm simply too tired. looks like it :) > > > > > > + clog.values = (uint64_t)(sas->incoming_buffer + > > > > cx); r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, > > > > &clog); if (r) { > > > > error_report("KVM_S390_SET_CMMA_BITS failed: > > > > %s", strerror(-r)); @@ -126,7 +126,7 @@ static void > > > > kvm_s390_stattrib_synchronize(S390StAttribState *sa) if (cx < > > > > max) { clog.start_gfn = cx; > > > > clog.count = max - cx; > > > > - clog.values = (uint64_t)(sas->incoming_buffer + cx > > > > * len); > > > > and here we fill in the last pieces if there are any leftovers, > > which at this point are guaranteed to be smaller than len. > > > > > > + clog.values = (uint64_t)(sas->incoming_buffer + > > > > cx); r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, > > > > &clog); if (r) { > > > > error_report("KVM_S390_SET_CMMA_BITS failed: > > > > %s", strerror(-r)); > > > > > >