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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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,
>>
>