qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
@ 2014-10-30  9:36 Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 1/9] valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl Christian Borntraeger
                   ` (11 more replies)
  0 siblings, 12 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

This series avoids most memcheck false positives in KVM ioctls on s390x
and x86_64.

Please review and consider for 2.2 or later. Some of these things could
also be fixed in valgrind, but it will take a while until these changes
hit a release or distros.

The series is also available via signed tag:

The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03:

  Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000)

are available in the git repository at:

  git://github.com/borntraeger/qemu.git tags/memcheck

for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a:

  valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100)

----------------------------------------------------------------
valgrind/i386/s390x: memcheck false positives

Let's avoid most memcheck false positives in KVM ioctls.

----------------------------------------------------------------
Christian Borntraeger (9):
      valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl
      valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl
      valgrind/i386: avoid false positives on KVM_SET_PIT ioctl
      valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
      valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl
      valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl
      valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
      valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl
      valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl

 hw/i386/kvm/clock.c | 3 +--
 hw/i386/kvm/i8254.c | 2 +-
 kvm-all.c           | 2 +-
 target-i386/kvm.c   | 9 +++++----
 target-s390x/kvm.c  | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 1/9] valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 2/9] valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl Christian Borntraeger
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_dirty_log contains padding fields that trigger false
positives in valgrind. Let's use a designated initializer to avoid
false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 44a5e72..b951320 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -400,7 +400,7 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
-    KVMDirtyLog d;
+    KVMDirtyLog d = {};
     KVMSlot *mem;
     int ret = 0;
     hwaddr start_addr = section->offset_within_address_space;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 2/9] valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 1/9] valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 3/9] valgrind/i386: avoid false positives on KVM_SET_PIT ioctl Christian Borntraeger
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

kvm_clock_data contains pad fields. Let's use a designated
initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/i386/kvm/clock.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 1ac60d6..78f5d26 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -88,7 +88,7 @@ static void kvmclock_vm_state_change(void *opaque, int running,
     int ret;
 
     if (running) {
-        struct kvm_clock_data data;
+        struct kvm_clock_data data = {};
         uint64_t time_at_migration = kvmclock_current_nsec(s);
 
         s->clock_valid = false;
@@ -99,7 +99,6 @@ static void kvmclock_vm_state_change(void *opaque, int running,
         }
 
         data.clock = s->clock;
-        data.flags = 0;
         ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
         if (ret < 0) {
             fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret));
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 3/9] valgrind/i386: avoid false positives on KVM_SET_PIT ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 1/9] valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 2/9] valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 4/9] valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl Christian Borntraeger
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_pit_state2 contains pad fields. Let's use a designated
initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/i386/kvm/i8254.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
index 472af81..90eea10 100644
--- a/hw/i386/kvm/i8254.c
+++ b/hw/i386/kvm/i8254.c
@@ -138,7 +138,7 @@ static void kvm_pit_get(PITCommonState *pit)
 static void kvm_pit_put(PITCommonState *pit)
 {
     KVMPITState *s = KVM_PIT(pit);
-    struct kvm_pit_state2 kpit;
+    struct kvm_pit_state2 kpit = {};
     struct kvm_pit_channel_state *kchan;
     struct PITChannelState *sc;
     int i, ret;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 4/9] valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (2 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 3/9] valgrind/i386: avoid false positives on KVM_SET_PIT ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 5/9] valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl Christian Borntraeger
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_xcrs contains padding bytes. Let's use a designated
initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ddedc73..e6fe37b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1074,7 +1074,7 @@ static int kvm_put_xsave(X86CPU *cpu)
 static int kvm_put_xcrs(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    struct kvm_xcrs xcrs;
+    struct kvm_xcrs xcrs = {};
 
     if (!kvm_has_xcrs()) {
         return 0;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 5/9] valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (3 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 4/9] valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 6/9] valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl Christian Borntraeger
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_msrs contains padding bytes. Let's use a designated
initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index e6fe37b..cd7b19c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1189,7 +1189,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
     struct {
         struct kvm_msrs info;
         struct kvm_msr_entry entries[150];
-    } msr_data;
+    } msr_data = {};
     struct kvm_msr_entry *msrs = msr_data.entries;
     int n = 0, i;
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 6/9] valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (4 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 5/9] valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl Christian Borntraeger
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_msrs contains pad fields. Let's use a designated
initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index cd7b19c..6203634 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1150,7 +1150,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
     struct {
         struct kvm_msrs info;
         struct kvm_msr_entry entries[1];
-    } msr_data;
+    } msr_data = {};
     struct kvm_msr_entry *msrs = msr_data.entries;
 
     if (!has_msr_tsc_deadline) {
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (5 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 6/9] valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-11-05 10:33   ` Paolo Bonzini
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 8/9] valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl Christian Borntraeger
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_msrs contains a pad field. Lets initialize this pad
field. A designated initializer seems not appropriate here, as
struct kvm_msrs is embedded in the msr_data structure.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-i386/kvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6203634..90020cb 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1628,6 +1628,7 @@ static int kvm_get_msrs(X86CPU *cpu)
     }
 
     msr_data.info.nmsrs = n;
+    msr_data.info.pad = 0;
     ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
     if (ret < 0) {
         return ret;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 8/9] valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (6 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 9/9] valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl Christian Borntraeger
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_vcpu_events contains reserved fields. Let's use a
designated initializer to avoid false positives in valgrind.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 90020cb..34db720 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1854,7 +1854,7 @@ static int kvm_put_apic(X86CPU *cpu)
 static int kvm_put_vcpu_events(X86CPU *cpu, int level)
 {
     CPUX86State *env = &cpu->env;
-    struct kvm_vcpu_events events;
+    struct kvm_vcpu_events events= {};
 
     if (!kvm_has_vcpu_events()) {
         return 0;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH 9/9] valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (7 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 8/9] valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl Christian Borntraeger
@ 2014-10-30  9:36 ` Christian Borntraeger
  2014-10-30  9:46 ` [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Peter Maydell
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:36 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: Christian Borntraeger, qemu-devel

struct kvm_fpu contains an alignment padding on s390x. Let's use a
designated initializer to avoid false positives from valgrind/memcheck.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 5b10a25..ec79ad4 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -208,7 +208,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     CPUS390XState *env = &cpu->env;
     struct kvm_sregs sregs;
     struct kvm_regs regs;
-    struct kvm_fpu fpu;
+    struct kvm_fpu fpu = {};
     int r;
     int i;
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (8 preceding siblings ...)
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 9/9] valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl Christian Borntraeger
@ 2014-10-30  9:46 ` Peter Maydell
  2014-10-30  9:50   ` Christian Borntraeger
  2014-10-30 13:03 ` Paolo Bonzini
  2014-11-13 14:34 ` Paolo Bonzini
  11 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2014-10-30  9:46 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: Paolo Bonzini, qemu-devel

On 30 October 2014 09:36, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> This series avoids most memcheck false positives in KVM ioctls on s390x
> and x86_64.
>
> Please review and consider for 2.2 or later. Some of these things could
> also be fixed in valgrind, but it will take a while until these changes
> hit a release or distros.

Are you planning to submit the valgrind fixes as well? These definitely
seem like valgrind bugs that we're having to work around here (though
the workarounds are pretty simple so they're not a huge deal).

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:46 ` [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Peter Maydell
@ 2014-10-30  9:50   ` Christian Borntraeger
  2014-10-30  9:58     ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30  9:50 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, qemu-devel

Am 30.10.2014 10:46, schrieb Peter Maydell:
> On 30 October 2014 09:36, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> This series avoids most memcheck false positives in KVM ioctls on s390x
>> and x86_64.
>>
>> Please review and consider for 2.2 or later. Some of these things could
>> also be fixed in valgrind, but it will take a while until these changes
>> hit a release or distros.
> 
> Are you planning to submit the valgrind fixes as well? These definitely
> seem like valgrind bugs that we're having to work around here (though
> the workarounds are pretty simple so they're not a huge deal).

Yes, I will try to get some of this fixed in valgrind as well. This will take a little longer though because the code changes are bigger than just 1 line of code. Given that valgrind has around 1 release/year, this patch set is certainly
a nice band-aid that is useful for todays development.

Christian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:50   ` Christian Borntraeger
@ 2014-10-30  9:58     ` Peter Maydell
  2014-10-30 11:01       ` Christian Borntraeger
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2014-10-30  9:58 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: Paolo Bonzini, qemu-devel

On 30 October 2014 09:50, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Yes, I will try to get some of this fixed in valgrind as well. This will
> take a little longer though because the code changes are bigger than just
> 1 line of code. Given that valgrind has around 1 release/year, this patch
> set is certainly a nice band-aid that is useful for todays development.

I guess these patches also mean we're not going to get valgrind warnings
if we forget to initialize a necessary field in the struct, but I
suppose we can live with that.

-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:58     ` Peter Maydell
@ 2014-10-30 11:01       ` Christian Borntraeger
  0 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30 11:01 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, qemu-devel

Am 30.10.2014 10:58, schrieb Peter Maydell:
> On 30 October 2014 09:50, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> Yes, I will try to get some of this fixed in valgrind as well. This will
>> take a little longer though because the code changes are bigger than just
>> 1 line of code. Given that valgrind has around 1 release/year, this patch
>> set is certainly a nice band-aid that is useful for todays development.
> 
> I guess these patches also mean we're not going to get valgrind warnings
> if we forget to initialize a necessary field in the struct.

Yes, thats correct. The alternative would be to not use an designated initializer and instead memset reserved and pad field.

> but I suppose we can live with that.
> 
> -- PMM
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (9 preceding siblings ...)
  2014-10-30  9:46 ` [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Peter Maydell
@ 2014-10-30 13:03 ` Paolo Bonzini
  2014-10-30 13:20   ` Christian Borntraeger
  2014-11-13 14:34 ` Paolo Bonzini
  11 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2014-10-30 13:03 UTC (permalink / raw)
  To: qemu-devel

On 10/30/2014 10:36 AM, Christian Borntraeger wrote:
> Some of these things could
> also be fixed in valgrind, but it will take a while until these changes
> hit a release or distros.

Ok, it's sensible to have it fixed in QEMU if it's temporary.  Which
could not be fixed in valgrind?

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30 13:03 ` Paolo Bonzini
@ 2014-10-30 13:20   ` Christian Borntraeger
  2014-11-03 12:27     ` Paolo Bonzini
  0 siblings, 1 reply; 21+ messages in thread
From: Christian Borntraeger @ 2014-10-30 13:20 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: qemu-devel

Am 30.10.2014 14:03, schrieb Paolo Bonzini:
> On 10/30/2014 10:36 AM, Christian Borntraeger wrote:
>> Some of these things could
>> also be fixed in valgrind, but it will take a while until these changes
>> hit a release or distros.
> 
> Ok, it's sensible to have it fixed in QEMU if it's temporary.  Which
> could not be fixed in valgrind?

This is a tricky question. A typical annotation in valgrind for an more complex ioctl looks like

   case VKI_SIOCGMIIREG:         /* get hardware entry registers */
      PRE_MEM_RASCIIZ( "ioctl(SIOCGIFMIIREG)",
                     (Addr)((struct vki_ifreq *)ARG3)->vki_ifr_name );
      PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)",
                     (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id,
                     sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id) );
      PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)",
                     (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num,
                     sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num) );
      PRE_MEM_WRITE( "ioctl(SIOCGIFMIIREG)", ARG3, 
                     sizeof(struct vki_ifreq));
      break;

This scheme works fine as long as the ioctl is unchanged.
So any ioctl that has padding and no flags this should be doable.

For all KVM ioctls with reserved fields that might become used on certain flags, we have two options:

a: we would instruct valgrind to not check the reserved fields
Whenever we start using them, we would still not check those field

b: we would instruct valgrind to not check the reserved fields if flags has a certain value (e.g. 0), otherwise all reserved fields would be checked.
Whenever we start using the reserved fields, valgrind would complain unless we write all. So in that case we have to modify valgrind again

In essence a will cause false negatives, b will cause false positives

I think b is preferred

Christian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30 13:20   ` Christian Borntraeger
@ 2014-11-03 12:27     ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-03 12:27 UTC (permalink / raw)
  To: Christian Borntraeger, Peter Maydell; +Cc: qemu-devel



On 30/10/2014 14:20, Christian Borntraeger wrote:
> Am 30.10.2014 14:03, schrieb Paolo Bonzini:
>> On 10/30/2014 10:36 AM, Christian Borntraeger wrote:
>>> Some of these things could
>>> also be fixed in valgrind, but it will take a while until these changes
>>> hit a release or distros.
>>
>> Ok, it's sensible to have it fixed in QEMU if it's temporary.  Which
>> could not be fixed in valgrind?
> 
> This is a tricky question. A typical annotation in valgrind for an more complex ioctl looks like
> 
>    case VKI_SIOCGMIIREG:         /* get hardware entry registers */
>       PRE_MEM_RASCIIZ( "ioctl(SIOCGIFMIIREG)",
>                      (Addr)((struct vki_ifreq *)ARG3)->vki_ifr_name );
>       PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)",
>                      (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id,
>                      sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id) );
>       PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)",
>                      (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num,
>                      sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num) );
>       PRE_MEM_WRITE( "ioctl(SIOCGIFMIIREG)", ARG3, 
>                      sizeof(struct vki_ifreq));
>       break;
> 
> This scheme works fine as long as the ioctl is unchanged.
> So any ioctl that has padding and no flags this should be doable.
> 
> For all KVM ioctls with reserved fields that might become used on certain flags, we have two options:
> 
> a: we would instruct valgrind to not check the reserved fields
> Whenever we start using them, we would still not check those field
> 
> b: we would instruct valgrind to not check the reserved fields if flags has a certain value (e.g. 0), otherwise all reserved fields would be checked.
> Whenever we start using the reserved fields, valgrind would complain unless we write all. So in that case we have to modify valgrind again
> 
> In essence a will cause false negatives, b will cause false positives
> 
> I think b is preferred

I agree.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
  2014-10-30  9:36 ` [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl Christian Borntraeger
@ 2014-11-05 10:33   ` Paolo Bonzini
  2014-11-05 10:37     ` Paolo Bonzini
  0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-05 10:33 UTC (permalink / raw)
  To: Christian Borntraeger, Peter Maydell; +Cc: qemu-devel

On 30/10/2014 10:36, Christian Borntraeger wrote:
> struct kvm_msrs contains a pad field. Lets initialize this pad
> field. A designated initializer seems not appropriate here, as
> struct kvm_msrs is embedded in the msr_data structure.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

What about this:

    msr_data.info = (struct kvm_msrs) {
        .nmsrs = n
    };

?  It would also be applicable to other uses of kvm_msrs.

Also, you're missing one occurrence in kvm_put_msr_feature_control.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
  2014-11-05 10:33   ` Paolo Bonzini
@ 2014-11-05 10:37     ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-05 10:37 UTC (permalink / raw)
  To: Christian Borntraeger, Peter Maydell; +Cc: qemu-devel



On 05/11/2014 11:33, Paolo Bonzini wrote:
> On 30/10/2014 10:36, Christian Borntraeger wrote:
>> struct kvm_msrs contains a pad field. Lets initialize this pad
>> field. A designated initializer seems not appropriate here, as
>> struct kvm_msrs is embedded in the msr_data structure.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> What about this:
> 
>     msr_data.info = (struct kvm_msrs) {
>         .nmsrs = n
>     };
> 
> ?  It would also be applicable to other uses of kvm_msrs.

Also, KVM_SET_MSRS has to deal with a reserved field in struct
kvm_msr_entry.  Currently you handle it with a relatively large memset
produced by the designated initializer "= {}" in kvm_put_msrs.  However,
you could set it in kvm_msr_entry_set, and avoid the memset.

Paolo

> Also, you're missing one occurrence in kvm_put_msr_feature_control.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
                   ` (10 preceding siblings ...)
  2014-10-30 13:03 ` Paolo Bonzini
@ 2014-11-13 14:34 ` Paolo Bonzini
  2014-11-13 19:07   ` Christian Borntraeger
  11 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-13 14:34 UTC (permalink / raw)
  To: Christian Borntraeger, Peter Maydell; +Cc: qemu-devel

On 30/10/2014 10:36, Christian Borntraeger wrote:
> This series avoids most memcheck false positives in KVM ioctls on s390x
> and x86_64.
> 
> Please review and consider for 2.2 or later. Some of these things could
> also be fixed in valgrind, but it will take a while until these changes
> hit a release or distros.
> 
> The series is also available via signed tag:
> 
> The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03:
> 
>   Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000)
> 
> are available in the git repository at:
> 
>   git://github.com/borntraeger/qemu.git tags/memcheck
> 
> for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a:
> 
>   valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100)
> 
> ----------------------------------------------------------------
> valgrind/i386/s390x: memcheck false positives
> 
> Let's avoid most memcheck false positives in KVM ioctls.
> 
> ----------------------------------------------------------------
> Christian Borntraeger (9):
>       valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl
>       valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl
>       valgrind/i386: avoid false positives on KVM_SET_PIT ioctl
>       valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
>       valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl
>       valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl
>       valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
>       valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl
>       valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl
> 
>  hw/i386/kvm/clock.c | 3 +--
>  hw/i386/kvm/i8254.c | 2 +-
>  kvm-all.c           | 2 +-
>  target-i386/kvm.c   | 9 +++++----
>  target-s390x/kvm.c  | 2 +-
>  5 files changed, 9 insertions(+), 9 deletions(-)
> 
> 
> 

Christian, when you respin can you add another one in
kvm_irqchip_add_adapter_route?  Coverity reports kroute.pad as
uninitialized.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives
  2014-11-13 14:34 ` Paolo Bonzini
@ 2014-11-13 19:07   ` Christian Borntraeger
  0 siblings, 0 replies; 21+ messages in thread
From: Christian Borntraeger @ 2014-11-13 19:07 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: qemu-devel

Am 13.11.2014 um 15:34 schrieb Paolo Bonzini:
> On 30/10/2014 10:36, Christian Borntraeger wrote:
>> This series avoids most memcheck false positives in KVM ioctls on s390x
>> and x86_64.
>>
>> Please review and consider for 2.2 or later. Some of these things could
>> also be fixed in valgrind, but it will take a while until these changes
>> hit a release or distros.
>>
>> The series is also available via signed tag:
>>
>> The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03:
>>
>>   Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000)
>>
>> are available in the git repository at:
>>
>>   git://github.com/borntraeger/qemu.git tags/memcheck
>>
>> for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a:
>>
>>   valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100)
>>
>> ----------------------------------------------------------------
>> valgrind/i386/s390x: memcheck false positives
>>
>> Let's avoid most memcheck false positives in KVM ioctls.
>>
>> ----------------------------------------------------------------
>> Christian Borntraeger (9):
>>       valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_PIT ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl
>>       valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
>>       valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl
>>       valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl
>>
>>  hw/i386/kvm/clock.c | 3 +--
>>  hw/i386/kvm/i8254.c | 2 +-
>>  kvm-all.c           | 2 +-
>>  target-i386/kvm.c   | 9 +++++----
>>  target-s390x/kvm.c  | 2 +-
>>  5 files changed, 9 insertions(+), 9 deletions(-)
>>
>>
>>
> 
> Christian, when you respin can you add another one in
> kvm_irqchip_add_adapter_route?  Coverity reports kroute.pad as
> uninitialized.

OK, will do. It will take probably some more days, but it will happen :-)

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2014-11-13 19:07 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-30  9:36 [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 1/9] valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 2/9] valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 3/9] valgrind/i386: avoid false positives on KVM_SET_PIT ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 4/9] valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 5/9] valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 6/9] valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl Christian Borntraeger
2014-11-05 10:33   ` Paolo Bonzini
2014-11-05 10:37     ` Paolo Bonzini
2014-10-30  9:36 ` [Qemu-devel] [PATCH 8/9] valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl Christian Borntraeger
2014-10-30  9:36 ` [Qemu-devel] [PATCH 9/9] valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl Christian Borntraeger
2014-10-30  9:46 ` [Qemu-devel] [PATCH 0/9] valgrind/i386/s390x: memcheck false positives Peter Maydell
2014-10-30  9:50   ` Christian Borntraeger
2014-10-30  9:58     ` Peter Maydell
2014-10-30 11:01       ` Christian Borntraeger
2014-10-30 13:03 ` Paolo Bonzini
2014-10-30 13:20   ` Christian Borntraeger
2014-11-03 12:27     ` Paolo Bonzini
2014-11-13 14:34 ` Paolo Bonzini
2014-11-13 19:07   ` Christian Borntraeger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).