Re: [PATCH v2 2/4] target/ppc: init 'lpcr' in kvmppc_enable_cap_large_decr()

[David Gibson]

On Thu, Mar 31, 2022 at 03:46:57PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 3/31/22 14:36, Richard Henderson wrote:
> > On 3/31/22 11:17, Daniel Henrique Barboza wrote:
> > > > Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
> > > > one of the valgrind hinting mechanisms to inform it that
> > > > kvm_get_one_reg() writes the variable at *target?
> > > 
> > > I didn't find a way of doing that looking in the memcheck helpers
> > > (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would 
> > > be a
> > > good way of solving this warning because we would put stuff inside a 
> > > specific
> > > function X and all callers of X would be covered by it.
> > > 
> > > What I did find instead is a memcheck macro called 
> > > VALGRIND_MAKE_MEM_DEFINED that
> > > tells Valgrind that the var was initialized.
> > > 
> > > This patch would then be something as follows:
> > > 
> > > 
> > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > > index dc93b99189..b0e22fa283 100644
> > > --- a/target/ppc/kvm.c
> > > +++ b/target/ppc/kvm.c
> > > @@ -56,6 +56,10 @@
> > >   #define DEBUG_RETURN_GUEST 0
> > >   #define DEBUG_RETURN_GDB   1
> > > 
> > > +#ifdef CONFIG_VALGRIND_H
> > > +#include <valgrind/memcheck.h>
> > > +#endif
> > > +
> > >   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> > >       KVM_CAP_LAST_INFO
> > >   };
> > > @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, 
> > > int enable)
> > >       CPUState *cs = CPU(cpu);
> > >       uint64_t lpcr;
> > > 
> > > +#ifdef CONFIG_VALGRIND_H
> > > +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
> > > +#endif
> > > +
> > >       kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> > >       /* Do we need to modify the LPCR? */
> > > 
> > > 
> > > CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.
> > > 
> > > I agree that this "Valgrind is complaining about variable initialization" 
> > > is a whack-a-mole
> > > situation that will keep happening in the future if we keep adding this 
> > > same code pattern
> > > (passing as reference an uninitialized var). For now, given that we have 
> > > only 4 instances
> > > to fix it in ppc code (as far as I'm aware of), and we don't have a 
> > > better way of telling
> > > Valgrind that we know what we're doing, I think we're better of 
> > > initializing these vars.
> > 
> > I would instead put this annotation inside kvm_get_one_reg, so that it 
> > covers all kvm hosts.  But it's too late to do this for 7.0.
> 
> I wasn't planning on pushing these changes for 7.0 since they aren't fixing 
> mem
> leaks or anything really bad. It's more of a quality of life improvement when
> using Valgrind.
> 
> I also tried to put this annotation in kvm_get_one_reg() and it didn't solve 
> the
> warning.

That's weird, I'm pretty sure that should work.  I'd double check to
make sure you had all the parameters right (e.g. could you have marked
the pointer itself as initialized, rather than the memory it points
to).

> I didn't find a way of telling Valgrind "consider that every time this
> function is called with parameter X it initializes X". That would be a good 
> solution
> to put in the common KVM files and fix the problem for everybody.
> 
> 
> Daniel
> 
> 
> 
> > 
> > 
> > r~
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature