| Message ID | 1516035122-7617-1-git-send-email-imbrenda@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] s390x: fix storage attributes migration for non-small guests | expand |
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)); >
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));
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)); >
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)); > > >
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)); > > > > > >
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)); > > > > > > > > > >
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] ...
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.
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));
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(-)