Re: [Qemu-devel] [PATCH v1 1/1] s390x: fix storage attributes migration for non-small guests

[Claudio Imbrenda]

On Thu, 18 Jan 2018 18:02:40 +0100
Cornelia Huck <address@hidden> wrote:

> On Thu, 18 Jan 2018 17:52:29 +0100
> Claudio Imbrenda <address@hidden> wrote:
> 
> > On Thu, 18 Jan 2018 17:20:34 +0100
> > Cornelia Huck <address@hidden> wrote:
> >   
> > > On Mon, 15 Jan 2018 17:52:02 +0100
> > > Claudio Imbrenda <address@hidden> 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 <address@hidden>
> > > > 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));      
> > >     
> >   
>