Re: [RFC PATCH 1/1] kvm-all.c: hint Valgrind that kvm_get_one_reg() inits memory

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

On Tue, Apr 05, 2022 at 03:30:26PM +0100, Peter Maydell wrote:
> On Tue, 5 Apr 2022 at 14:07, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
> >
> > There is a lot of Valgrind warnings about conditional jump depending on
> > unintialized values like this one (taken from a pSeries guest):
> >
> >  Conditional jump or move depends on uninitialised value(s)
> >     at 0xB011DC: kvmppc_enable_cap_large_decr (kvm.c:2544)
> >     by 0x92F28F: cap_large_decr_cpu_apply (spapr_caps.c:523)
> >     by 0x930C37: spapr_caps_cpu_apply (spapr_caps.c:921)
> >     by 0x955D3B: spapr_reset_vcpu (spapr_cpu_core.c:73)
> > (...)
> >   Uninitialised value was created by a stack allocation
> >     at 0xB01150: kvmppc_enable_cap_large_decr (kvm.c:2538)
> >
> > In this case, the alleged unintialized value is the 'lpcr' variable that
> > is written by kvm_get_one_reg() and then used in an if clause:
> >
> > int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
> > {
> >     CPUState *cs = CPU(cpu);
> >     uint64_t lpcr;
> >
> >     kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> >     /* Do we need to modify the LPCR? */
> >     if (!!(lpcr & LPCR_LD) != !!enable) { <---- Valgrind warns here
> > (...)
> >
> > A quick fix is to init the variable that kvm_get_one_reg() is going to
> > write ('lpcr' in the example above). Another idea is to convince
> > Valgrind that kvm_get_one_reg() inits the 'void *target' memory in case
> > the ioctl() is successful. This will put some boilerplate in the
> > function but it will bring benefit for its other callers.
> 
> Doesn't Valgrind have a way of modelling ioctls where it
> knows what data is read and written ? In general
> ioctl-using programs don't need to have special case
> "I am running under valgrind" handling, so this seems to
> me like valgrind is missing support for this particular ioctl.

I think that's true, but would obviously have a much larger lead time
- someone would need to figure out how to add support for this
specific ioctl() (handling any ambiguity about what type of fd we're
dealing with), get it merged then we'd need to update to the new
valgrind to get the benefits.

> More generally, how much use is running QEMU with KVM enabled
> under valgrind anyway? Valgrind has no way of knowing about
> writes to memory that the guest vCPUs do...

Those should be limited to the guest memory area though, which as
mmap()ed space would already be considered initialized, I believe.  If
there's some fancy data race checking tool for valgrind then that
might be a problem, but for just the normal memcheck tool, I don't
think it should be an issue.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]