1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH/RFC 0/2] valgrind fallout for s390x/kvm
  4. View patch

[Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter

Christian Borntraeger posted 2 patches 8 years, 2 months ago
[Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter
Posted by Christian Borntraeger 8 years, 2 months ago 
valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
undefined value for flags. Right now this is unused, but we
better play safe.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target/s390x/kvm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 343fcec..b0439a1 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
 
 void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
 {
-    struct kvm_s390_irq_state irq_state;
+    struct kvm_s390_irq_state irq_state = {
+        .buf = (uint64_t) cpu->irqstate,
+        .len = VCPU_IRQ_BUF_SIZE,
+    };
     CPUState *cs = CPU(cpu);
     int32_t bytes;
 
@@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
         return;
     }
 
-    irq_state.buf = (uint64_t) cpu->irqstate;
-    irq_state.len = VCPU_IRQ_BUF_SIZE;
-
     bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state);
     if (bytes < 0) {
         cpu->irqstate_saved_size = 0;
-- 
2.9.4
Re: [Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter
Posted by Cornelia Huck 8 years, 2 months ago 
On Mon, 20 Nov 2017 13:35:24 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
> undefined value for flags. Right now this is unused, but we
> better play safe.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/kvm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 343fcec..b0439a1 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>  
>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>  {
> -    struct kvm_s390_irq_state irq_state;
> +    struct kvm_s390_irq_state irq_state = {
> +        .buf = (uint64_t) cpu->irqstate,
> +        .len = VCPU_IRQ_BUF_SIZE,
> +    };
>      CPUState *cs = CPU(cpu);
>      int32_t bytes;
>  
> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>          return;
>      }
>  
> -    irq_state.buf = (uint64_t) cpu->irqstate;
> -    irq_state.len = VCPU_IRQ_BUF_SIZE;
> -
>      bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state);
>      if (bytes < 0) {
>          cpu->irqstate_saved_size = 0;

I'm wondering why it does not also complain for KVM_S390_SET_IRQ_STATE?
It would make sense to use a struct initializer there as well.
Re: [Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter
Posted by Christian Borntraeger 8 years, 2 months ago 

On 11/20/2017 01:57 PM, Cornelia Huck wrote:
> On Mon, 20 Nov 2017 13:35:24 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
>> undefined value for flags. Right now this is unused, but we
>> better play safe.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  target/s390x/kvm.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 343fcec..b0439a1 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>>  
>>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>>  {
>> -    struct kvm_s390_irq_state irq_state;
>> +    struct kvm_s390_irq_state irq_state = {
>> +        .buf = (uint64_t) cpu->irqstate,
>> +        .len = VCPU_IRQ_BUF_SIZE,
>> +    };
>>      CPUState *cs = CPU(cpu);
>>      int32_t bytes;
>>  
>> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>>          return;
>>      }
>>  
>> -    irq_state.buf = (uint64_t) cpu->irqstate;
>> -    irq_state.len = VCPU_IRQ_BUF_SIZE;
>> -
>>      bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state);
>>      if (bytes < 0) {
>>          cpu->irqstate_saved_size = 0;
> 
> I'm wondering why it does not also complain for KVM_S390_SET_IRQ_STATE?

I guess that my tests do not having peding interrupts during migration and we exit early in

int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
{
    CPUState *cs = CPU(cpu);
    struct kvm_s390_irq_state irq_state;
    int r;

-->    if (cpu->irqstate_saved_size == 0) {
-->        return 0;
    }




> It would make sense to use a struct initializer there as well.

Yes, I think you are right.
>
Re: [Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter
Posted by Thomas Huth 8 years, 2 months ago 
On 20.11.2017 13:35, Christian Borntraeger wrote:
> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
> undefined value for flags. Right now this is unused, but we
> better play safe.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/kvm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 343fcec..b0439a1 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>  
>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>  {
> -    struct kvm_s390_irq_state irq_state;
> +    struct kvm_s390_irq_state irq_state = {
> +        .buf = (uint64_t) cpu->irqstate,
> +        .len = VCPU_IRQ_BUF_SIZE,
> +    };
>      CPUState *cs = CPU(cpu);
>      int32_t bytes;
>  
> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>          return;
>      }
>  
> -    irq_state.buf = (uint64_t) cpu->irqstate;
> -    irq_state.len = VCPU_IRQ_BUF_SIZE;
> -
>      bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state);
>      if (bytes < 0) {
>          cpu->irqstate_saved_size = 0;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

BTW, should we have a check in the kernel that flags is properly set to
zero?

 Thomas
Re: [Qemu-devel] [PATCH 1/2] s390x/migration: use zero flag parameter
Posted by Christian Borntraeger 8 years, 2 months ago 

On 11/20/2017 02:01 PM, Thomas Huth wrote:
> On 20.11.2017 13:35, Christian Borntraeger wrote:
>> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
>> undefined value for flags. Right now this is unused, but we
>> better play safe.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  target/s390x/kvm.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 343fcec..b0439a1 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>>  
>>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>>  {
>> -    struct kvm_s390_irq_state irq_state;
>> +    struct kvm_s390_irq_state irq_state = {
>> +        .buf = (uint64_t) cpu->irqstate,
>> +        .len = VCPU_IRQ_BUF_SIZE,
>> +    };
>>      CPUState *cs = CPU(cpu);
>>      int32_t bytes;
>>  
>> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>>          return;
>>      }
>>  
>> -    irq_state.buf = (uint64_t) cpu->irqstate;
>> -    irq_state.len = VCPU_IRQ_BUF_SIZE;
>> -
>>      bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state);
>>      if (bytes < 0) {
>>          cpu->irqstate_saved_size = 0;
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> BTW, should we have a check in the kernel that flags is properly set to
> zero?

We should have one. Doing that now will break old QEMUs :-/

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.