[PATCH] s390x/kvm: help valgrind in several places

[PATCH] s390x/kvm: help valgrind in several places

[Christian Borntraeger]

We need some little help in the code to reduce the valgrind noise.
- some designated initializers for the cpu model features and subfunctions
- mark memory as defined for sida memory reads

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

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 69881a0da0..bcd0ee0d14 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -52,6 +52,10 @@
 #include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/pv.h"
 
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
+
 #ifndef DEBUG_KVM
 #define DEBUG_KVM  0
 #endif
@@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
         error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
         abort();
     }
+
+#ifdef CONFIG_VALGRIND_H
+    if (!is_write) {
+        VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
+    }
+#endif
+
     return ret;
 }
 
@@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 
 static int query_cpu_subfunc(S390FeatBitmap features)
 {
-    struct kvm_s390_vm_cpu_subfunc prop;
+    struct kvm_s390_vm_cpu_subfunc prop = {};
     struct kvm_device_attr attr = {
         .group = KVM_S390_VM_CPU_MODEL,
         .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
@@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
 
 static int query_cpu_feat(S390FeatBitmap features)
 {
-    struct kvm_s390_vm_cpu_feat prop;
+    struct kvm_s390_vm_cpu_feat prop = {};
     struct kvm_device_attr attr = {
         .group = KVM_S390_VM_CPU_MODEL,
         .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
-- 
2.25.1

Re: [PATCH] s390x/kvm: help valgrind in several places

[Philippe Mathieu-Daudé]

Hi Christian,

On 4/28/20 8:31 PM, Christian Borntraeger wrote:
> We need some little help in the code to reduce the valgrind noise.
> - some designated initializers for the cpu model features and subfunctions

^ This could go as trivial patch while we discuss the rest.

> - mark memory as defined for sida memory reads
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---

I couldn't apply this patch, then figured out it targets s390-next.

>   target/s390x/kvm.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 69881a0da0..bcd0ee0d14 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -52,6 +52,10 @@
>   #include "hw/s390x/s390-virtio-hcall.h"
>   #include "hw/s390x/pv.h"
>   
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>   #ifndef DEBUG_KVM
>   #define DEBUG_KVM  0
>   #endif
> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
>           error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
>           abort();
>       }

What about kvm_s390_mem_op()?

> +
> +#ifdef CONFIG_VALGRIND_H
> +    if (!is_write) {
> +        VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
> +    }
> +#endif

I agree with this macro usage, but think it should be widely accessible 
by the whole codebase (and other targets).

"exec/memory.h" is for MemoryRegion and AddressSpace. Maybe 
"exec/ram_addr.h" is a better place for common helpers.

If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the 
obvious place.

> +
>       return ret;
>   }
>   
> @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>   
>   static int query_cpu_subfunc(S390FeatBitmap features)
>   {
> -    struct kvm_s390_vm_cpu_subfunc prop;
> +    struct kvm_s390_vm_cpu_subfunc prop = {};
>       struct kvm_device_attr attr = {
>           .group = KVM_S390_VM_CPU_MODEL,
>           .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
>   
>   static int query_cpu_feat(S390FeatBitmap features)
>   {
> -    struct kvm_s390_vm_cpu_feat prop;
> +    struct kvm_s390_vm_cpu_feat prop = {};
>       struct kvm_device_attr attr = {
>           .group = KVM_S390_VM_CPU_MODEL,
>           .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
> 

Re: [PATCH] s390x/kvm: help valgrind in several places

[Christian Borntraeger]

On 29.04.20 09:00, Philippe Mathieu-Daudé wrote:
> Hi Christian,
> 
> On 4/28/20 8:31 PM, Christian Borntraeger wrote:
>> We need some little help in the code to reduce the valgrind noise.
>> - some designated initializers for the cpu model features and subfunctions
> 
> ^ This could go as trivial patch while we discuss the rest.

I can certainly split.

> 
>> - mark memory as defined for sida memory reads
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
> 
> I couldn't apply this patch, then figured out it targets s390-next.
> 
>>   target/s390x/kvm.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 69881a0da0..bcd0ee0d14 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -52,6 +52,10 @@
>>   #include "hw/s390x/s390-virtio-hcall.h"
>>   #include "hw/s390x/pv.h"
>>   +#ifdef CONFIG_VALGRIND_H
>> +#include <valgrind/memcheck.h>
>> +#endif
>> +
>>   #ifndef DEBUG_KVM
>>   #define DEBUG_KVM  0
>>   #endif
>> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
>>           error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
>>           abort();
>>       }
> 
> What about kvm_s390_mem_op()?

I have not triggered something in here, but you are right, there should be cases where we make conditions
depend of that content. Will change my testing and add something here as well. 
> 
>> +
>> +#ifdef CONFIG_VALGRIND_H
>> +    if (!is_write) {
>> +        VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
>> +    }
>> +#endif
> 
> I agree with this macro usage, but think it should be widely accessible by the whole codebase (and other targets).
> 
> "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h" is a better place for common helpers.
> 
> If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place.

This macro IS available for the whole codebase if you include valgrind/memcheck.h.
We used it in the past (before 2.2) for kvm memory.
See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark memory as initialized).

The only thing that we could discuss is introducing a new global function like
valgrind_make_mem_defined that would hide the ifdefs.
Is there interest in such a thing?
It is likely that these corner cases (valgrind not able to see that this is defined) are more likely 
o happen with KVM.  But it would be useful for anything not only KVM.
 

> 
>> +
>>       return ret;
>>   }
>>   @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>>     static int query_cpu_subfunc(S390FeatBitmap features)
>>   {
>> -    struct kvm_s390_vm_cpu_subfunc prop;
>> +    struct kvm_s390_vm_cpu_subfunc prop = {};
>>       struct kvm_device_attr attr = {
>>           .group = KVM_S390_VM_CPU_MODEL,
>>           .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
>> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
>>     static int query_cpu_feat(S390FeatBitmap features)
>>   {
>> -    struct kvm_s390_vm_cpu_feat prop;
>> +    struct kvm_s390_vm_cpu_feat prop = {};
>>       struct kvm_device_attr attr = {
>>           .group = KVM_S390_VM_CPU_MODEL,
>>           .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
>>
> 

Re: [PATCH] s390x/kvm: help valgrind in several places

[Philippe Mathieu-Daudé]

+Paolo

On 4/29/20 9:21 AM, Christian Borntraeger wrote:
> 
> On 29.04.20 09:00, Philippe Mathieu-Daudé wrote:
>> Hi Christian,
>>
>> On 4/28/20 8:31 PM, Christian Borntraeger wrote:
>>> We need some little help in the code to reduce the valgrind noise.
>>> - some designated initializers for the cpu model features and subfunctions
>>
>> ^ This could go as trivial patch while we discuss the rest.
> 
> I can certainly split.

If you split then please directly include "Reviewed-by: Philippe 
Mathieu-Daudé <philmd@redhat.com>" to the it.

> 
>>
>>> - mark memory as defined for sida memory reads
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>
>> I couldn't apply this patch, then figured out it targets s390-next.
>>
>>>    target/s390x/kvm.c | 15 +++++++++++++--
>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 69881a0da0..bcd0ee0d14 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -52,6 +52,10 @@
>>>    #include "hw/s390x/s390-virtio-hcall.h"
>>>    #include "hw/s390x/pv.h"
>>>    +#ifdef CONFIG_VALGRIND_H
>>> +#include <valgrind/memcheck.h>
>>> +#endif
>>> +
>>>    #ifndef DEBUG_KVM
>>>    #define DEBUG_KVM  0
>>>    #endif
>>> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
>>>            error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
>>>            abort();
>>>        }
>>
>> What about kvm_s390_mem_op()?
> 
> I have not triggered something in here, but you are right, there should be cases where we make conditions
> depend of that content. Will change my testing and add something here as well.
>>
>>> +
>>> +#ifdef CONFIG_VALGRIND_H
>>> +    if (!is_write) {
>>> +        VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
>>> +    }
>>> +#endif
>>
>> I agree with this macro usage, but think it should be widely accessible by the whole codebase (and other targets).
>>
>> "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h" is a better place for common helpers.
>>
>> If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place.
> 
> This macro IS available for the whole codebase if you include valgrind/memcheck.h.
> We used it in the past (before 2.2) for kvm memory.
> See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark memory as initialized).
> 
> The only thing that we could discuss is introducing a new global function like
> valgrind_make_mem_defined that would hide the ifdefs.
> Is there interest in such a thing?
> It is likely that these corner cases (valgrind not able to see that this is defined) are more likely
> o happen with KVM.  But it would be useful for anything not only KVM.

Correct. What I wanted to say here is, if we can use this code 
elsewhere, then it is worth adding a global helper (generic or KVM).

If you reuse this code twice, having an inlined function makes the code 
more readable anyway.

See for example in util/coroutine-ucontext.c:

static inline void valgrind_stack_deregister(CoroutineUContext *co)
{
     VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
}

Maybe we could have something like:

static inline void valgrind_define_memory(void *ptr,
                                           size_t size,
                                           bool is_defined)
{
     if (is_defined) {
         VALGRIND_MAKE_MEM_DEFINED(ptr, size);
     }
}

> 
>>
>>> +
>>>        return ret;
>>>    }
>>>    @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>>>      static int query_cpu_subfunc(S390FeatBitmap features)
>>>    {
>>> -    struct kvm_s390_vm_cpu_subfunc prop;
>>> +    struct kvm_s390_vm_cpu_subfunc prop = {};
>>>        struct kvm_device_attr attr = {
>>>            .group = KVM_S390_VM_CPU_MODEL,
>>>            .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
>>> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
>>>      static int query_cpu_feat(S390FeatBitmap features)
>>>    {
>>> -    struct kvm_s390_vm_cpu_feat prop;
>>> +    struct kvm_s390_vm_cpu_feat prop = {};
>>>        struct kvm_device_attr attr = {
>>>            .group = KVM_S390_VM_CPU_MODEL,
>>>            .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
>>>
>>
>