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

[Daniel Henrique Barboza]

On 4/1/22 00:40, David Gibson wrote:
> 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).


You're right. I got confused with different setups here and there and thought 
that
it didn't work.

I sent a patch to kvm-all.c that tries to do that:


https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00507.html


As for this series, for now I'm willing to take it since it improves the 
situation with
simple initializations. We can reconsider it if we make good progress through 
the common
code. At any rate these are 7.1 patches, so we have time.



Thanks,


Daniel



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