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

[Claudio Imbrenda]

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

...
[snip]

> > > > > > 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  
> 
> Err, yes :) I simply wanted to comment that this looks worse than "not
> migrated completely".

yeah, that's true :) but the offset ended up big enough to always
get -EFAULT from the kernel and get ignored by qemu afterwards, which
then resulted in not all values being migrated.

should I change the description to explain the issue in more detail?

[snip]
...