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

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

[Claudio Imbrenda]

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 <imbrenda@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);
+            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);
+            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));
-- 
2.7.4

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

[Christian Borntraeger]

CCing qemu-s390x.

On 01/15/2018 05:52 PM, Claudio Imbrenda 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 <imbrenda@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);
> +            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);
> +            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));
> 

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

[Cornelia Huck]

On Mon, 15 Jan 2018 17:52:02 +0100
Claudio Imbrenda <imbrenda@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 <imbrenda@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?

> +            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);
> +            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));

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

[Claudio Imbrenda]

On Thu, 18 Jan 2018 17:20:34 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 15 Jan 2018 17:52:02 +0100
> Claudio Imbrenda <imbrenda@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 <imbrenda@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.

> > +            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));  
> 

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

[Cornelia Huck]

On Thu, 18 Jan 2018 17:52:29 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Thu, 18 Jan 2018 17:20:34 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, 15 Jan 2018 17:52:02 +0100
> > Claudio Imbrenda <imbrenda@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 <imbrenda@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.)

But maybe I'm simply too tired.

> 
> > > +            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));    
> >   
> 

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 <cohuck@redhat.com> wrote:

> On Thu, 18 Jan 2018 17:52:29 +0100
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > On Thu, 18 Jan 2018 17:20:34 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Mon, 15 Jan 2018 17:52:02 +0100
> > > Claudio Imbrenda <imbrenda@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 <imbrenda@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));      
> > >     
> >   
> 

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

[Cornelia Huck]

On Thu, 18 Jan 2018 18:23:17 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Thu, 18 Jan 2018 18:02:40 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu, 18 Jan 2018 17:52:29 +0100
> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> >   
> > > On Thu, 18 Jan 2018 17:20:34 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > On Mon, 15 Jan 2018 17:52:02 +0100
> > > > Claudio Imbrenda <imbrenda@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 <imbrenda@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

Err, yes :) I simply wanted to comment that this looks worse than "not
migrated completely".

> 
> > 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));        
> > > >       
> > >     
> >   
> 

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 <cohuck@redhat.com> 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]
...

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

[Cornelia Huck]

On Thu, 18 Jan 2018 18:33:51 +0100
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Thu, 18 Jan 2018 18:25:47 +0100
> Cornelia Huck <cohuck@redhat.com> 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.

So the moral is: If you're wrong, be really wrong? :)

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

Just mentioning something like out-of-bounds due to wrong offset or so
would be good.