[RFC PATCH 0/1] add Valgrind hint in kvm_get_one_reg()

[RFC PATCH 0/1] add Valgrind hint in kvm_get_one_reg()

[Daniel Henrique Barboza]

Hi,

Valgrind is not happy with how we're using KVM functions that receives a
parameter via reference and write them. This results in a lot of
complaints about uninitialized values when using these functions
because, as default, Valgrind doesn't know that the variable is being
initialized in the function.

This is the overall pattern that Valgrind does not like:

---
uint64_t val;
(...)
kvm_get_one_reg(...., &val);

if (val) {...}
---

Valgrind complains that the 'if' clause is using an uninitialized
variable.

A quick fix is to init 'val' and be done with it. The drawback is that
every single caller of kvm_get_one_reg() must also be bothered with
initializing these variables to avoid the warnings.

David suggested in [1] that, instead, we should add a Valgrind hint in
the common KVM functions to fix this issue for everyone. This is what
this patch accomplishes. kvm_get_one_reg() has 20+ callers so I believe
this extra boilerplate is worth the benefits.

There are more common instances of KVM functions that Valgrind complains
about. If we're good with the approach taken here we can think about
adding this hint for more functions.


[1] https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg07351.html

Daniel Henrique Barboza (1):
  kvm-all.c: hint Valgrind that kvm_get_one_reg() inits memory

 accel/kvm/kvm-all.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

-- 
2.35.1

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

[Daniel Henrique Barboza]

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.

This patch uses the memcheck VALGRING_MAKE_MEM_DEFINED() to mark the
'target' variable as initialized if the ioctl is successful.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 accel/kvm/kvm-all.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 5f1377ca04..d9acba23c7 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -53,6 +53,10 @@
 #include <sys/eventfd.h>
 #endif
 
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
+
 /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
  * need to use the real host PAGE_SIZE, as that's what KVM will use.
  */
@@ -3504,6 +3508,19 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
     if (r) {
         trace_kvm_failed_reg_get(id, strerror(-r));
     }
+
+#ifdef CONFIG_VALGRIND_H
+    if (r == 0) {
+        switch (id & KVM_REG_SIZE_MASK) {
+        case KVM_REG_SIZE_U32:
+            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint32_t));
+            break;
+        case KVM_REG_SIZE_U64:
+            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint64_t));
+            break;
+        }
+    }
+#endif
     return r;
 }
 
-- 
2.35.1

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

[Peter Maydell]

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.

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

thanks
-- PMM

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

[Daniel Henrique Barboza]

On 4/5/22 11:30, 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 don't know if Valgrind is capable of doing that. Guess it's worth a look.

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

At least in the hosts I have access to, I wasn't able to get a pSeries guest
booting up to prompt with Valgrind + TCG. It was painfully slow. Valgrind + KVM
is slow but doable. Granted, vCPUs reads/writes can't be profiled with it when
using KVM, but for everything else is alright.


Thanks,


Daniel


> 
> thanks
> -- PMM

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

[David Gibson]

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

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

[David Gibson]

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

On Tue, Apr 05, 2022 at 10:04:39AM -0300, Daniel Henrique Barboza 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.
> 
> This patch uses the memcheck VALGRING_MAKE_MEM_DEFINED() to mark the
> 'target' variable as initialized if the ioctl is successful.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  accel/kvm/kvm-all.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 5f1377ca04..d9acba23c7 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -53,6 +53,10 @@
>  #include <sys/eventfd.h>
>  #endif
>  
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>  /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>   * need to use the real host PAGE_SIZE, as that's what KVM will use.
>   */
> @@ -3504,6 +3508,19 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>      if (r) {
>          trace_kvm_failed_reg_get(id, strerror(-r));
>      }
> +
> +#ifdef CONFIG_VALGRIND_H
> +    if (r == 0) {
> +        switch (id & KVM_REG_SIZE_MASK) {
> +        case KVM_REG_SIZE_U32:
> +            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint32_t));
> +            break;
> +        case KVM_REG_SIZE_U64:
> +            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint64_t));
> +            break;
> +        }
> +    }
> +#endif
>      return 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

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