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é <address@hidden>" to the it.

> > > - 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()?
>
> 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,