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 <address@hidden>
> ---

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,