[Qemu-devel] [RFC/PATCH] qemu: Use valgrind annotations to mark kvm guest memory as defined

[Qemu-devel] [RFC/PATCH] qemu: Use valgrind annotations to mark kvm guest memory as defined

[Christian Borntraeger]

valgrind with kvm produces a big amount of false positives regarding
"Conditional jump or move depends on uninitialised value(s)". This
happens because the guest memory is allocated with qemu_vmalloc which
boils down posix_memalign etc. This function is (correctly) considered
by valgrind as returning undefined memory.

Since valgrind is based on jitting code, it will not be able to see
changes made by the guest to guest memory if this is done by KVM_RUN,
thus keeping most of the guest memory undefined.

Now lots of places in qemu will then use guest memory to change behaviour.
To avoid the flood of these messages, lets declare the whole guest
memory as defined. This will reduce the noise and allows us to see real
problems.

In the future we might want to make this conditional, since there
is actually something that we can use those false positives for:
These messages will point to code that depends on guest memory, so
we can use these backtraces to actually make an audit that is focussed
only at those code places. For normal development we dont want to
see those messages, though.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 configure |    3 ++-
 kvm-all.c |    7 +++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index 280726c..90df0d8 100755
--- a/configure
+++ b/configure
@@ -2900,11 +2900,12 @@ if compile_prog "-Werror" "" ; then
 fi
 
 ########################################
-# check if we have valgrind/valgrind.h
+# check if we have valgrind/valgrind.h and valgrind/memcheck.h
 
 valgrind_h=no
 cat > $TMPC << EOF
 #include <valgrind/valgrind.h>
+#include <valgrind/memcheck.h>
 int main(void) {
   return 0;
 }
diff --git a/kvm-all.c b/kvm-all.c
index 2148b20..33d16cf 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -39,6 +39,10 @@
 #include <sys/eventfd.h>
 #endif
 
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
+
 /* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */
 #define PAGE_SIZE TARGET_PAGE_SIZE
 
@@ -1687,6 +1691,9 @@ void *kvm_vmalloc(ram_addr_t size)
 
 void kvm_setup_guest_memory(void *start, size_t size)
 {
+#ifdef CONFIG_VALGRIND_H
+    VALGRIND_MAKE_MEM_DEFINED(start, size);
+#endif
     if (!kvm_has_sync_mmu()) {
         int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
-- 
1.7.0.4

Re: [Qemu-devel] [RFC/PATCH] qemu: Use valgrind annotations to mark kvm guest memory as defined

[Christian Borntraeger]

On 10/08/12 15:11, Christian Borntraeger wrote:
> valgrind with kvm produces a big amount of false positives regarding
> "Conditional jump or move depends on uninitialised value(s)". This
> happens because the guest memory is allocated with qemu_vmalloc which
> boils down posix_memalign etc. This function is (correctly) considered
> by valgrind as returning undefined memory.
> 
> Since valgrind is based on jitting code, it will not be able to see
> changes made by the guest to guest memory if this is done by KVM_RUN,
> thus keeping most of the guest memory undefined.
> 
> Now lots of places in qemu will then use guest memory to change behaviour.
> To avoid the flood of these messages, lets declare the whole guest
> memory as defined. This will reduce the noise and allows us to see real
> problems.
> 
> In the future we might want to make this conditional, since there
> is actually something that we can use those false positives for:
> These messages will point to code that depends on guest memory, so
> we can use these backtraces to actually make an audit that is focussed
> only at those code places. For normal development we dont want to
> see those messages, though.


Any opinion about this patch?


 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  configure |    3 ++-
>  kvm-all.c |    7 +++++++
>  2 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/configure b/configure
> index 280726c..90df0d8 100755
> --- a/configure
> +++ b/configure
> @@ -2900,11 +2900,12 @@ if compile_prog "-Werror" "" ; then
>  fi
> 
>  ########################################
> -# check if we have valgrind/valgrind.h
> +# check if we have valgrind/valgrind.h and valgrind/memcheck.h
> 
>  valgrind_h=no
>  cat > $TMPC << EOF
>  #include <valgrind/valgrind.h>
> +#include <valgrind/memcheck.h>
>  int main(void) {
>    return 0;
>  }
> diff --git a/kvm-all.c b/kvm-all.c
> index 2148b20..33d16cf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -39,6 +39,10 @@
>  #include <sys/eventfd.h>
>  #endif
> 
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>  /* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */
>  #define PAGE_SIZE TARGET_PAGE_SIZE
> 
> @@ -1687,6 +1691,9 @@ void *kvm_vmalloc(ram_addr_t size)
> 
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
> +#ifdef CONFIG_VALGRIND_H
> +    VALGRIND_MAKE_MEM_DEFINED(start, size);
> +#endif
>      if (!kvm_has_sync_mmu()) {
>          int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
> 

Re: [Qemu-devel] [RFC/PATCH] qemu: Use valgrind annotations to mark kvm guest memory as defined

[Avi Kivity]

On 08/21/2012 12:03 PM, Christian Borntraeger wrote:
> On 10/08/12 15:11, Christian Borntraeger wrote:
>> valgrind with kvm produces a big amount of false positives regarding
>> "Conditional jump or move depends on uninitialised value(s)". This
>> happens because the guest memory is allocated with qemu_vmalloc which
>> boils down posix_memalign etc. This function is (correctly) considered
>> by valgrind as returning undefined memory.
>> 
>> Since valgrind is based on jitting code, it will not be able to see
>> changes made by the guest to guest memory if this is done by KVM_RUN,
>> thus keeping most of the guest memory undefined.
>> 
>> Now lots of places in qemu will then use guest memory to change behaviour.
>> To avoid the flood of these messages, lets declare the whole guest
>> memory as defined. This will reduce the noise and allows us to see real
>> problems.
>> 
>> In the future we might want to make this conditional, since there
>> is actually something that we can use those false positives for:
>> These messages will point to code that depends on guest memory, so
>> we can use these backtraces to actually make an audit that is focussed
>> only at those code places. For normal development we dont want to
>> see those messages, though.
> 
> 
> Any opinion about this patch?

Missed it, sorry.  Now applied to uq/master, thanks.


-- 
error compiling committee.c: too many arguments to function