[Qemu-devel] [PATCH/RFC 0/2] valgrind fallout for s390x/kvm

[Qemu-devel] [PATCH/RFC 0/2] valgrind fallout for s390x/kvm

[Christian Borntraeger]

I need these patches below to run valgrind on s390x/kvm without
a lot of false positives.


Christian Borntraeger (2):
  s390x/migration: use zero flag parameter
  s390x/kvm: use valgrind annotations for kvm device attributes

 hw/s390x/s390-skeys-kvm.c    | 12 ++++++++-
 hw/s390x/s390-stattrib-kvm.c |  7 ++++++
 target/s390x/kvm.c           | 59 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 69 insertions(+), 9 deletions(-)

-- 
2.9.4

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

[Christian Borntraeger]

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

[Qemu-devel] [PATCH 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Christian Borntraeger]

the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing
side effects. Use valgrind annotations to properly mark
all storage changes instead of using memset or designated
initializers.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-skeys-kvm.c    | 12 ++++++++++-
 hw/s390x/s390-stattrib-kvm.c |  7 ++++++
 target/s390x/kvm.c           | 51 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
index dc54ed8..0986795 100644
--- a/hw/s390x/s390-skeys-kvm.c
+++ b/hw/s390x/s390-skeys-kvm.c
@@ -13,6 +13,9 @@
 #include "hw/s390x/storage-keys.h"
 #include "sysemu/kvm.h"
 #include "qemu/error-report.h"
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
 
 static int kvm_s390_skeys_enabled(S390SKeysState *ss)
 {
@@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
         .count = count,
         .skeydata_addr = (__u64)keys
     };
+    int ret;
 
-    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
+    ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
+    if (!ret) {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(keys, count);
+#endif
+    }
+    return ret;
 }
 
 static int kvm_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index 41770a7..eb2b220 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -18,6 +18,9 @@
 #include "exec/ram_addr.h"
 #include "cpu.h"
 #include "kvm_s390x.h"
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
 
 Object *kvm_s390_stattrib_create(void)
 {
@@ -54,6 +57,10 @@ static int kvm_s390_stattrib_read_helper(S390StAttribState *sa,
     if (r < 0) {
         error_report("KVM_S390_GET_CMMA_BITS failed: %s", strerror(-r));
         return r;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(values, clog.count);
+#endif
     }
 
     *start_gfn = clog.start_gfn;
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index b0439a1..d3f3ddb 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -50,6 +50,9 @@
 #include "exec/memattrs.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/s390-virtio-hcall.h"
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
 
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
@@ -176,8 +179,13 @@ int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit)
     rc = kvm_s390_query_mem_limit(hw_limit);
     if (rc) {
         return rc;
-    } else if (*hw_limit < new_limit) {
-        return -E2BIG;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(hw_limit, sizeof(*hw_limit));
+#endif
+        if (*hw_limit < new_limit) {
+            return -E2BIG;
+        }
     }
 
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
@@ -312,6 +320,10 @@ static int compat_disable_facilities(KVMState *s, __u64 fac_mask[], int len)
     if (rc) {
         error_report("KVM_GET_DEVICE_ATTR failed with rc %d", rc);
         return  rc;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(&mach_attrs, sizeof(mach_attrs));
+#endif
     }
 
     cpu_attrs.cpuid = mach_attrs.cpuid | 0xff00000000000000UL;
@@ -704,11 +716,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
     r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
     if (r) {
         return r;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(tod_low, sizeof(tod_low));
+#endif
     }
 
     attr.attr = KVM_S390_VM_TOD_HIGH;
     attr.addr = (uint64_t)tod_high;
-    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+    r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+    if (!r) {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(tod_high, sizeof(tod_high));
+#endif
+    }
+    return r;
 }
 
 int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
@@ -722,6 +744,11 @@ int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
     };
 
     r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+    if (!r) {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(&gtod, sizeof(gtod));
+#endif
+    }
     *tod_high = gtod.epoch_idx;
     *tod_low  = gtod.tod;
 
@@ -2085,6 +2112,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
         cpu->irqstate_saved_size = 0;
         error_report("Migration of interrupt state failed");
         return;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(cpu->irqstate, bytes);
+#endif
     }
 
     cpu->irqstate_saved_size = bytes;
@@ -2170,6 +2201,10 @@ static int query_cpu_subfunc(S390FeatBitmap features)
     rc = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
     if (rc) {
         return  rc;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop));
+#endif
     }
 
     /*
@@ -2280,6 +2315,10 @@ static int query_cpu_feat(S390FeatBitmap features)
     rc = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
     if (rc) {
         return  rc;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop));
+#endif
     }
 
     for (i = 0; i < ARRAY_SIZE(kvm_to_feat); i++) {
@@ -2328,7 +2367,7 @@ bool kvm_s390_cpu_models_supported(void)
 
 void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
 {
-    struct kvm_s390_vm_cpu_machine prop = {};
+    struct kvm_s390_vm_cpu_machine prop;
     struct kvm_device_attr attr = {
         .group = KVM_S390_VM_CPU_MODEL,
         .attr = KVM_S390_VM_CPU_MACHINE,
@@ -2349,6 +2388,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     if (rc) {
         error_setg(errp, "KVM: Error querying host CPU model: %d", rc);
         return;
+    } else {
+#ifdef CONFIG_VALGRIND_H
+        VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop));
+#endif
     }
 
     cpu_type = cpuid_type(prop.cpuid);
-- 
2.9.4

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

[Cornelia Huck]

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 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Cornelia Huck]

On Mon, 20 Nov 2017 13:35:25 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing
> side effects. Use valgrind annotations to properly mark
> all storage changes instead of using memset or designated
> initializers.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-skeys-kvm.c    | 12 ++++++++++-
>  hw/s390x/s390-stattrib-kvm.c |  7 ++++++
>  target/s390x/kvm.c           | 51 ++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 65 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
> index dc54ed8..0986795 100644
> --- a/hw/s390x/s390-skeys-kvm.c
> +++ b/hw/s390x/s390-skeys-kvm.c
> @@ -13,6 +13,9 @@
>  #include "hw/s390x/storage-keys.h"
>  #include "sysemu/kvm.h"
>  #include "qemu/error-report.h"
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
>  
>  static int kvm_s390_skeys_enabled(S390SKeysState *ss)
>  {
> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
>          .count = count,
>          .skeydata_addr = (__u64)keys
>      };
> +    int ret;
>  
> -    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
> +    ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
> +    if (!ret) {
> +#ifdef CONFIG_VALGRIND_H
> +        VALGRIND_MAKE_MEM_DEFINED(keys, count);
> +#endif

This looks ugly :(

Is s390x the only one hitting those side effects? If we need to
sprinkle those all over the source code, it improves valgrind results
but makes the code harder to read...

(And no, I don't have a better idea.)

> +    }
> +    return ret;
>  }

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

[Thomas Huth]

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

[Christian Borntraeger]

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 :-/

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

[Christian Borntraeger]

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 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Christian Borntraeger]

On 11/20/2017 02:00 PM, Cornelia Huck wrote:
> On Mon, 20 Nov 2017 13:35:25 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing
>> side effects. Use valgrind annotations to properly mark
>> all storage changes instead of using memset or designated
>> initializers.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/s390x/s390-skeys-kvm.c    | 12 ++++++++++-
>>  hw/s390x/s390-stattrib-kvm.c |  7 ++++++
>>  target/s390x/kvm.c           | 51 ++++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
>> index dc54ed8..0986795 100644
>> --- a/hw/s390x/s390-skeys-kvm.c
>> +++ b/hw/s390x/s390-skeys-kvm.c
>> @@ -13,6 +13,9 @@
>>  #include "hw/s390x/storage-keys.h"
>>  #include "sysemu/kvm.h"
>>  #include "qemu/error-report.h"
>> +#ifdef CONFIG_VALGRIND_H
>> +#include <valgrind/memcheck.h>
>> +#endif
>>  
>>  static int kvm_s390_skeys_enabled(S390SKeysState *ss)
>>  {
>> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
>>          .count = count,
>>          .skeydata_addr = (__u64)keys
>>      };
>> +    int ret;
>>  
>> -    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
>> +    if (!ret) {
>> +#ifdef CONFIG_VALGRIND_H
>> +        VALGRIND_MAKE_MEM_DEFINED(keys, count);
>> +#endif
> 
> This looks ugly :(
> 
> Is s390x the only one hitting those side effects? If we need to
> sprinkle those all over the source code, it improves valgrind results
> but makes the code harder to read...
>
> (And no, I don't have a better idea.)

We could provide a wrapper?


> 
>> +    }
>> +    return ret;
>>  }
> 

Re: [Qemu-devel] [PATCH 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Christian Borntraeger]

On 11/20/2017 02:00 PM, Cornelia Huck wrote:
> On Mon, 20 Nov 2017 13:35:25 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing
>> side effects. Use valgrind annotations to properly mark
>> all storage changes instead of using memset or designated
>> initializers.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/s390x/s390-skeys-kvm.c    | 12 ++++++++++-
>>  hw/s390x/s390-stattrib-kvm.c |  7 ++++++
>>  target/s390x/kvm.c           | 51 ++++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
>> index dc54ed8..0986795 100644
>> --- a/hw/s390x/s390-skeys-kvm.c
>> +++ b/hw/s390x/s390-skeys-kvm.c
>> @@ -13,6 +13,9 @@
>>  #include "hw/s390x/storage-keys.h"
>>  #include "sysemu/kvm.h"
>>  #include "qemu/error-report.h"
>> +#ifdef CONFIG_VALGRIND_H
>> +#include <valgrind/memcheck.h>
>> +#endif
>>  
>>  static int kvm_s390_skeys_enabled(S390SKeysState *ss)
>>  {
>> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
>>          .count = count,
>>          .skeydata_addr = (__u64)keys
>>      };
>> +    int ret;
>>  
>> -    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
>> +    if (!ret) {
>> +#ifdef CONFIG_VALGRIND_H
>> +        VALGRIND_MAKE_MEM_DEFINED(keys, count);
>> +#endif
> 
> This looks ugly :(
> 
> Is s390x the only one hitting those side effects?

Almost nobody else uses the device attributes. And when they use it they
have the same problem
We papered over some bugs by zero-initializing structs but I think marking
only the really changed memory will help to detect real bugs (e.g. if code
reads ioctl data but the ioctl fails we will not detect this when pre-zeroing.


 If we need to
> sprinkle those all over the source code, it improves valgrind results
> but makes the code harder to read...
> 
> (And no, I don't have a better idea.)
> 
>> +    }
>> +    return ret;
>>  }
> 

Re: [Qemu-devel] [PATCH 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Cornelia Huck]

On Mon, 20 Nov 2017 14:11:03 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/20/2017 02:00 PM, Cornelia Huck wrote:
> > On Mon, 20 Nov 2017 13:35:25 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing
> >> side effects. Use valgrind annotations to properly mark
> >> all storage changes instead of using memset or designated
> >> initializers.
> >>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  hw/s390x/s390-skeys-kvm.c    | 12 ++++++++++-
> >>  hw/s390x/s390-stattrib-kvm.c |  7 ++++++
> >>  target/s390x/kvm.c           | 51 ++++++++++++++++++++++++++++++++++++++++----
> >>  3 files changed, 65 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
> >> index dc54ed8..0986795 100644
> >> --- a/hw/s390x/s390-skeys-kvm.c
> >> +++ b/hw/s390x/s390-skeys-kvm.c
> >> @@ -13,6 +13,9 @@
> >>  #include "hw/s390x/storage-keys.h"
> >>  #include "sysemu/kvm.h"
> >>  #include "qemu/error-report.h"
> >> +#ifdef CONFIG_VALGRIND_H
> >> +#include <valgrind/memcheck.h>
> >> +#endif
> >>  
> >>  static int kvm_s390_skeys_enabled(S390SKeysState *ss)
> >>  {
> >> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
> >>          .count = count,
> >>          .skeydata_addr = (__u64)keys
> >>      };
> >> +    int ret;
> >>  
> >> -    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
> >> +    ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
> >> +    if (!ret) {
> >> +#ifdef CONFIG_VALGRIND_H
> >> +        VALGRIND_MAKE_MEM_DEFINED(keys, count);
> >> +#endif  
> > 
> > This looks ugly :(
> > 
> > Is s390x the only one hitting those side effects?  
> 
> Almost nobody else uses the device attributes. And when they use it they
> have the same problem

Makes sense.

> We papered over some bugs by zero-initializing structs but I think marking
> only the really changed memory will help to detect real bugs (e.g. if code
> reads ioctl data but the ioctl fails we will not detect this when pre-zeroing.

Yes, from a "finding bugs" standpoint that's definitely an
improvement. Maybe a wrapper would improve the readability.

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

[Christian Borntraeger]

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.
The same is true for SET_IRQ_STATE. While QEMU is now fixed in
that regard, we should make sure to not use the flag and pad
fields in the kernel. A corresponding kernel documentation patch
will be submitted later.

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

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 343fcec..76065ec 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;
@@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
 int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
 {
     CPUState *cs = CPU(cpu);
-    struct kvm_s390_irq_state irq_state;
+    struct kvm_s390_irq_state irq_state = {
+        .buf = (uint64_t) cpu->irqstate,
+        .len = cpu->irqstate_saved_size,
+    };
     int r;
 
     if (cpu->irqstate_saved_size == 0) {
@@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
         return -ENOSYS;
     }
 
-    irq_state.buf = (uint64_t) cpu->irqstate;
-    irq_state.len = cpu->irqstate_saved_size;
-
     r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state);
     if (r) {
         error_report("Setting interrupt state failed %d", r);
-- 
2.9.4

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

[Thomas Huth]

On 22.11.2017 15:26, 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.
> The same is true for SET_IRQ_STATE. While QEMU is now fixed in
> that regard, we should make sure to not use the flag and pad
> fields in the kernel. A corresponding kernel documentation patch
> will be submitted later.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/kvm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 343fcec..76065ec 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;
> @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>  int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    struct kvm_s390_irq_state irq_state;
> +    struct kvm_s390_irq_state irq_state = {
> +        .buf = (uint64_t) cpu->irqstate,
> +        .len = cpu->irqstate_saved_size,
> +    };
>      int r;
>  
>      if (cpu->irqstate_saved_size == 0) {
> @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>          return -ENOSYS;
>      }
>  
> -    irq_state.buf = (uint64_t) cpu->irqstate;
> -    irq_state.len = cpu->irqstate_saved_size;
> -
>      r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state);
>      if (r) {
>          error_report("Setting interrupt state failed %d", r);
> 

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

IMHO that would still be fine for 2.11, too.

 Thomas

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

[Cornelia Huck]

On Wed, 22 Nov 2017 15:26:26 +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.
> The same is true for SET_IRQ_STATE. While QEMU is now fixed in
> that regard, we should make sure to not use the flag and pad
> fields in the kernel. A corresponding kernel documentation patch
> will be submitted later.

I'd reword this a bit; it is confusing to read changelogs describing a
then-future change which already happened. (I assume that we will do
the kernel sanitizing for 4.15?)

"valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
undefined value for flags. Kernels prior to 4.15 did not use that
field, and later kernels ignore it for compatibility reasons, but we
better play safe.

The same is true for SET_IRQ_STATE. We should make sure to not use the
flag field, either."

[I do not see a 'pad' field in the structure; that is a bug in the
kernel documentation.]

> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/kvm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 343fcec..76065ec 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;
> @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>  int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    struct kvm_s390_irq_state irq_state;
> +    struct kvm_s390_irq_state irq_state = {
> +        .buf = (uint64_t) cpu->irqstate,
> +        .len = cpu->irqstate_saved_size,
> +    };
>      int r;
>  
>      if (cpu->irqstate_saved_size == 0) {
> @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>          return -ENOSYS;
>      }
>  
> -    irq_state.buf = (uint64_t) cpu->irqstate;
> -    irq_state.len = cpu->irqstate_saved_size;
> -
>      r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state);
>      if (r) {
>          error_report("Setting interrupt state failed %d", r);

Patch looks good.

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

[Cornelia Huck]

On Wed, 22 Nov 2017 15:37:55 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 22.11.2017 15:26, 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.
> > The same is true for SET_IRQ_STATE. While QEMU is now fixed in
> > that regard, we should make sure to not use the flag and pad
> > fields in the kernel. A corresponding kernel documentation patch
> > will be submitted later.
> > 
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  target/s390x/kvm.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)

(...)

> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> IMHO that would still be fine for 2.11, too.

I can queue this if another fix comes along, but I'd not push it on its
own.

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

[Christian Borntraeger]

On 11/22/2017 03:43 PM, Cornelia Huck wrote:
> On Wed, 22 Nov 2017 15:26:26 +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.
>> The same is true for SET_IRQ_STATE. While QEMU is now fixed in
>> that regard, we should make sure to not use the flag and pad
>> fields in the kernel. A corresponding kernel documentation patch
>> will be submitted later.
> 
> I'd reword this a bit; it is confusing to read changelogs describing a
> then-future change which already happened. (I assume that we will do
> the kernel sanitizing for 4.15?)
> 
> "valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
> undefined value for flags. Kernels prior to 4.15 did not use that
> field, and later kernels ignore it for compatibility reasons, but we
> better play safe.
> 
> The same is true for SET_IRQ_STATE. We should make sure to not use the
> flag field, either."
> 
> [I do not see a 'pad' field in the structure; that is a bug in the
> kernel documentation.]

Would be ok for me. Can you take the patch and change the wording yourself?


> 
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  target/s390x/kvm.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 343fcec..76065ec 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;
>> @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>>  int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>> -    struct kvm_s390_irq_state irq_state;
>> +    struct kvm_s390_irq_state irq_state = {
>> +        .buf = (uint64_t) cpu->irqstate,
>> +        .len = cpu->irqstate_saved_size,
>> +    };
>>      int r;
>>  
>>      if (cpu->irqstate_saved_size == 0) {
>> @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>>          return -ENOSYS;
>>      }
>>  
>> -    irq_state.buf = (uint64_t) cpu->irqstate;
>> -    irq_state.len = cpu->irqstate_saved_size;
>> -
>>      r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state);
>>      if (r) {
>>          error_report("Setting interrupt state failed %d", r);
> 
> Patch looks good.
>