* [PATCH 0/3] Reduce the number of Valgrind reports in unit tests. @ 2019-07-30 16:01 Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length Andrey Shinkevich ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-30 16:01 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, den, vsementsov, andrey.shinkevich Running unit tests under the Valgrind may help to detect QEMU memory issues (suggested by Denis V. Lunev). Some of the Valgrind reports relate to the unit test code itself. Let's eliminate the detected memory issues to ease locating critical ones. Andrey Shinkevich (3): test-throttle: Fix uninitialized use of burst_length tests: Fix uninitialized byte in test_visitor_in_fuzz i386/kvm: initialize struct at full before ioctl call target/i386/kvm.c | 3 +++ tests/test-string-input-visitor.c | 8 +++----- tests/test-throttle.c | 2 ++ 3 files changed, 8 insertions(+), 5 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length 2019-07-30 16:01 [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich @ 2019-07-30 16:01 ` Andrey Shinkevich 2019-08-13 12:19 ` Alberto Garcia 2019-07-30 16:01 ` [PATCH 2/3] tests: Fix uninitialized byte in test_visitor_in_fuzz Andrey Shinkevich ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-30 16:01 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, den, vsementsov, andrey.shinkevich ThrottleState::cfg of the static variable 'ts' is reassigned with the local one in the do_test_accounting() and then is passed to the throttle_account() with uninitialized member LeakyBucket::burst_length. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- tests/test-throttle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test-throttle.c b/tests/test-throttle.c index a288122..ebf3aeb 100644 --- a/tests/test-throttle.c +++ b/tests/test-throttle.c @@ -557,6 +557,8 @@ static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */ BucketType index; int i; + throttle_config_init(&cfg); + for (i = 0; i < 3; i++) { BucketType index = to_test[is_ops][i]; cfg.buckets[index].avg = avg; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length 2019-07-30 16:01 ` [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length Andrey Shinkevich @ 2019-08-13 12:19 ` Alberto Garcia 0 siblings, 0 replies; 25+ messages in thread From: Alberto Garcia @ 2019-08-13 12:19 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block Cc: kvm, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, den, vsementsov, andrey.shinkevich On Tue 30 Jul 2019 06:01:36 PM CEST, Andrey Shinkevich wrote: > ThrottleState::cfg of the static variable 'ts' is reassigned with the > local one in the do_test_accounting() and then is passed to the > throttle_account() with uninitialized member LeakyBucket::burst_length. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/3] tests: Fix uninitialized byte in test_visitor_in_fuzz 2019-07-30 16:01 [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length Andrey Shinkevich @ 2019-07-30 16:01 ` Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich 2019-08-13 12:02 ` [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 3 siblings, 0 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-30 16:01 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, den, vsementsov, andrey.shinkevich One byte in the local buffer stays uninitialized, at least with the first iteration, because of the double decrement in the test_visitor_in_fuzz(). This is what Valgrind does not like and not critical for the test itself. So, reduce the number of the memory issues reports. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- tests/test-string-input-visitor.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index 34b54df..5418e08 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -444,16 +444,14 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data, char buf[10000]; for (i = 0; i < 100; i++) { - unsigned int j; + unsigned int j, k; j = g_test_rand_int_range(0, sizeof(buf) - 1); buf[j] = '\0'; - if (j != 0) { - for (j--; j != 0; j--) { - buf[j - 1] = (char)g_test_rand_int_range(0, 256); - } + for (k = 0; k != j; k++) { + buf[k] = (char)g_test_rand_int_range(0, 256); } v = visitor_input_test_init(data, buf); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:01 [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 2/3] tests: Fix uninitialized byte in test_visitor_in_fuzz Andrey Shinkevich @ 2019-07-30 16:01 ` Andrey Shinkevich 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé ` (2 more replies) 2019-08-13 12:02 ` [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 3 siblings, 3 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-30 16:01 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, den, vsementsov, andrey.shinkevich Not the whole structure is initialized before passing it to the KVM. Reduce the number of Valgrind reports. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- target/i386/kvm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/target/i386/kvm.c b/target/i386/kvm.c index dbbb137..ed57e31 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) return 0; } + memset(&msr_data, 0, sizeof(msr_data)); msr_data.info.nmsrs = 1; msr_data.entries[0].index = MSR_IA32_TSC; env->tsc_valid = !runstate_is_running(); @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) if (has_xsave) { env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); } max_nested_state_len = kvm_max_nested_state_length(); @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) return 0; } + memset(&dbgregs, 0, sizeof(dbgregs)); for (i = 0; i < 4; i++) { dbgregs.db[i] = env->dr[i]; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich @ 2019-07-30 16:44 ` Philippe Mathieu-Daudé 2019-07-30 17:05 ` Christian Borntraeger 2019-07-30 19:22 ` Paolo Bonzini 2019-07-30 16:46 ` Peter Maydell 2019-07-30 19:20 ` Paolo Bonzini 2 siblings, 2 replies; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2019-07-30 16:44 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block Cc: vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, pbonzini, rth On 7/30/19 6:01 PM, Andrey Shinkevich wrote: > Not the whole structure is initialized before passing it to the KVM. > Reduce the number of Valgrind reports. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > target/i386/kvm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index dbbb137..ed57e31 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) > return 0; > } > > + memset(&msr_data, 0, sizeof(msr_data)); I wonder the overhead of this one... > msr_data.info.nmsrs = 1; > msr_data.entries[0].index = MSR_IA32_TSC; > env->tsc_valid = !runstate_is_running(); > @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > > if (has_xsave) { > env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); > + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); OK > } > > max_nested_state_len = kvm_max_nested_state_length(); > @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) > return 0; > } > > + memset(&dbgregs, 0, sizeof(dbgregs)); OK > for (i = 0; i < 4; i++) { > dbgregs.db[i] = env->dr[i]; > } We could remove 'dbgregs.flags = 0;' Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé @ 2019-07-30 17:05 ` Christian Borntraeger 2019-07-30 17:14 ` Philippe Mathieu-Daudé 2019-07-31 9:05 ` Christophe de Dinechin 2019-07-30 19:22 ` Paolo Bonzini 1 sibling, 2 replies; 25+ messages in thread From: Christian Borntraeger @ 2019-07-30 17:05 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Andrey Shinkevich, qemu-devel, qemu-block Cc: vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, pbonzini, rth On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: > On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >> Not the whole structure is initialized before passing it to the KVM. >> Reduce the number of Valgrind reports. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> target/i386/kvm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >> index dbbb137..ed57e31 100644 >> --- a/target/i386/kvm.c >> +++ b/target/i386/kvm.c >> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >> return 0; >> } >> >> + memset(&msr_data, 0, sizeof(msr_data)); > > I wonder the overhead of this one... Cant we use designated initializers like in commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 Author: Christian Borntraeger <borntraeger@de.ibm.com> AuthorDate: Thu Oct 30 09:23:41 2014 +0100 Commit: Paolo Bonzini <pbonzini@redhat.com> CommitDate: Mon Dec 15 12:21:01 2014 +0100 valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl and others? This should minimize the impact. > >> msr_data.info.nmsrs = 1; >> msr_data.entries[0].index = MSR_IA32_TSC; >> env->tsc_valid = !runstate_is_running(); >> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >> >> if (has_xsave) { >> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); > > OK > >> } >> >> max_nested_state_len = kvm_max_nested_state_length(); >> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >> return 0; >> } >> >> + memset(&dbgregs, 0, sizeof(dbgregs)); > > OK > >> for (i = 0; i < 4; i++) { >> dbgregs.db[i] = env->dr[i]; >> } > > We could remove 'dbgregs.flags = 0;' > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 17:05 ` Christian Borntraeger @ 2019-07-30 17:14 ` Philippe Mathieu-Daudé 2019-07-30 17:47 ` Christian Borntraeger 2019-07-31 9:05 ` Christophe de Dinechin 1 sibling, 1 reply; 25+ messages in thread From: Philippe Mathieu-Daudé @ 2019-07-30 17:14 UTC (permalink / raw) To: Christian Borntraeger, Andrey Shinkevich, qemu-devel, qemu-block Cc: vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, pbonzini, rth On 7/30/19 7:05 PM, Christian Borntraeger wrote: > On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>> Not the whole structure is initialized before passing it to the KVM. >>> Reduce the number of Valgrind reports. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> target/i386/kvm.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>> index dbbb137..ed57e31 100644 >>> --- a/target/i386/kvm.c >>> +++ b/target/i386/kvm.c >>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>> return 0; >>> } >>> >>> + memset(&msr_data, 0, sizeof(msr_data)); >> >> I wonder the overhead of this one... > > Cant we use designated initializers like in > > commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 > Author: Christian Borntraeger <borntraeger@de.ibm.com> > AuthorDate: Thu Oct 30 09:23:41 2014 +0100 > Commit: Paolo Bonzini <pbonzini@redhat.com> > CommitDate: Mon Dec 15 12:21:01 2014 +0100 > > valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl > > and others? Is the compiler smart enough to figure out it doesn't need to zeroes in case env->tsc_valid is true and the function returns? > > This should minimize the impact. >> >>> msr_data.info.nmsrs = 1; >>> msr_data.entries[0].index = MSR_IA32_TSC; >>> env->tsc_valid = !runstate_is_running(); >>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>> >>> if (has_xsave) { >>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >> >> OK >> >>> } >>> >>> max_nested_state_len = kvm_max_nested_state_length(); >>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>> return 0; >>> } >>> >>> + memset(&dbgregs, 0, sizeof(dbgregs)); >> >> OK >> >>> for (i = 0; i < 4; i++) { >>> dbgregs.db[i] = env->dr[i]; >>> } >> >> We could remove 'dbgregs.flags = 0;' >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 17:14 ` Philippe Mathieu-Daudé @ 2019-07-30 17:47 ` Christian Borntraeger 0 siblings, 0 replies; 25+ messages in thread From: Christian Borntraeger @ 2019-07-30 17:47 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Andrey Shinkevich, qemu-devel, qemu-block Cc: vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, pbonzini, rth On 30.07.19 19:14, Philippe Mathieu-Daudé wrote: > On 7/30/19 7:05 PM, Christian Borntraeger wrote: >> On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >>> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>>> Not the whole structure is initialized before passing it to the KVM. >>>> Reduce the number of Valgrind reports. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> target/i386/kvm.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>> index dbbb137..ed57e31 100644 >>>> --- a/target/i386/kvm.c >>>> +++ b/target/i386/kvm.c >>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>> return 0; >>>> } >>>> >>>> + memset(&msr_data, 0, sizeof(msr_data)); >>> >>> I wonder the overhead of this one... >> >> Cant we use designated initializers like in >> >> commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 >> Author: Christian Borntraeger <borntraeger@de.ibm.com> >> AuthorDate: Thu Oct 30 09:23:41 2014 +0100 >> Commit: Paolo Bonzini <pbonzini@redhat.com> >> CommitDate: Mon Dec 15 12:21:01 2014 +0100 >> >> valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl >> >> and others? > > Is the compiler smart enough to figure out it doesn't need to zeroes in > case env->tsc_valid is true and the function returns? Good question, we would need to double check with objdump. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 17:05 ` Christian Borntraeger 2019-07-30 17:14 ` Philippe Mathieu-Daudé @ 2019-07-31 9:05 ` Christophe de Dinechin 2019-07-31 12:32 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Christophe de Dinechin @ 2019-07-31 9:05 UTC (permalink / raw) To: qemu-devel Cc: Philippe Mathieu-Daudé, Andrey Shinkevich, qemu-block, vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, pbonzini, den, rth Christian Borntraeger writes: > On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>> Not the whole structure is initialized before passing it to the KVM. >>> Reduce the number of Valgrind reports. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> target/i386/kvm.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>> index dbbb137..ed57e31 100644 >>> --- a/target/i386/kvm.c >>> +++ b/target/i386/kvm.c >>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>> return 0; >>> } >>> >>> + memset(&msr_data, 0, sizeof(msr_data)); >> >> I wonder the overhead of this one... > > Cant we use designated initializers like in > > commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 > Author: Christian Borntraeger <borntraeger@de.ibm.com> > AuthorDate: Thu Oct 30 09:23:41 2014 +0100 > Commit: Paolo Bonzini <pbonzini@redhat.com> > CommitDate: Mon Dec 15 12:21:01 2014 +0100 > > valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl > > and others? > > This should minimize the impact. Oh, when you talked about using designated initializers, I thought you were talking about fully initializing the struct, like so: diff --git a/target/i386/kvm.c b/target/i386/kvm.c index dbbb13772a..3533870c43 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -180,19 +180,20 @@ static int kvm_get_tsc(CPUState *cs) { X86CPU *cpu = X86_CPU(cs); CPUX86State *env = &cpu->env; - struct { - struct kvm_msrs info; - struct kvm_msr_entry entries[1]; - } msr_data; int ret; if (env->tsc_valid) { return 0; } - msr_data.info.nmsrs = 1; - msr_data.entries[0].index = MSR_IA32_TSC; - env->tsc_valid = !runstate_is_running(); + struct { + struct kvm_msrs info; + struct kvm_msr_entry entries[1]; + } msr_data = { + .info = { .nmsrs = 1 }, + .entries = { [0] = { .index = MSR_IA32_TSC } } + }; + env->tsc_valid = !runstate_is_running(); ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data); if (ret < 0) { This gives the compiler maximum opportunities to flag mistakes like initializing the same thing twice, and make it easier (read no smart optimizations) to initialize in one go. Moving the declaration past the 'if' also addresses Philippe's concern. >> >>> msr_data.info.nmsrs = 1; >>> msr_data.entries[0].index = MSR_IA32_TSC; >>> env->tsc_valid = !runstate_is_running(); >>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>> >>> if (has_xsave) { >>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >> >> OK >> >>> } >>> >>> max_nested_state_len = kvm_max_nested_state_length(); >>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>> return 0; >>> } >>> >>> + memset(&dbgregs, 0, sizeof(dbgregs)); >> >> OK >> >>> for (i = 0; i < 4; i++) { >>> dbgregs.db[i] = env->dr[i]; >>> } >> >> We could remove 'dbgregs.flags = 0;' >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> -- Cheers, Christophe de Dinechin (IRC c3d) ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 9:05 ` Christophe de Dinechin @ 2019-07-31 12:32 ` Paolo Bonzini 2019-07-31 14:10 ` Andrey Shinkevich 0 siblings, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2019-07-31 12:32 UTC (permalink / raw) To: Christophe de Dinechin, qemu-devel Cc: Philippe Mathieu-Daudé, Andrey Shinkevich, qemu-block, vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, rth On 31/07/19 11:05, Christophe de Dinechin wrote: > > Christian Borntraeger writes: > >> On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >>> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>>> Not the whole structure is initialized before passing it to the KVM. >>>> Reduce the number of Valgrind reports. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> target/i386/kvm.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>> index dbbb137..ed57e31 100644 >>>> --- a/target/i386/kvm.c >>>> +++ b/target/i386/kvm.c >>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>> return 0; >>>> } >>>> >>>> + memset(&msr_data, 0, sizeof(msr_data)); >>> >>> I wonder the overhead of this one... >> >> Cant we use designated initializers like in >> >> commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 >> Author: Christian Borntraeger <borntraeger@de.ibm.com> >> AuthorDate: Thu Oct 30 09:23:41 2014 +0100 >> Commit: Paolo Bonzini <pbonzini@redhat.com> >> CommitDate: Mon Dec 15 12:21:01 2014 +0100 >> >> valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl >> >> and others? >> >> This should minimize the impact. > > Oh, when you talked about using designated initializers, I thought you > were talking about fully initializing the struct, like so: Yeah, that would be good too. For now I'm applying Andrey's series though. Paolo > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index dbbb13772a..3533870c43 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -180,19 +180,20 @@ static int kvm_get_tsc(CPUState *cs) > { > X86CPU *cpu = X86_CPU(cs); > CPUX86State *env = &cpu->env; > - struct { > - struct kvm_msrs info; > - struct kvm_msr_entry entries[1]; > - } msr_data; > int ret; > > if (env->tsc_valid) { > return 0; > } > > - msr_data.info.nmsrs = 1; > - msr_data.entries[0].index = MSR_IA32_TSC; > - env->tsc_valid = !runstate_is_running(); > + struct { > + struct kvm_msrs info; > + struct kvm_msr_entry entries[1]; > + } msr_data = { > + .info = { .nmsrs = 1 }, > + .entries = { [0] = { .index = MSR_IA32_TSC } } > + }; > + env->tsc_valid = !runstate_is_running(); > > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data); > if (ret < 0) { > > > This gives the compiler maximum opportunities to flag mistakes like > initializing the same thing twice, and make it easier (read no smart > optimizations) to initialize in one go. Moving the declaration past the > 'if' also addresses Philippe's concern. > >>> >>>> msr_data.info.nmsrs = 1; >>>> msr_data.entries[0].index = MSR_IA32_TSC; >>>> env->tsc_valid = !runstate_is_running(); >>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>> >>>> if (has_xsave) { >>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >>> >>> OK >>> >>>> } >>>> >>>> max_nested_state_len = kvm_max_nested_state_length(); >>>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>>> return 0; >>>> } >>>> >>>> + memset(&dbgregs, 0, sizeof(dbgregs)); >>> >>> OK >>> >>>> for (i = 0; i < 4; i++) { >>>> dbgregs.db[i] = env->dr[i]; >>>> } >>> >>> We could remove 'dbgregs.flags = 0;' >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> > > > -- > Cheers, > Christophe de Dinechin (IRC c3d) > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 12:32 ` Paolo Bonzini @ 2019-07-31 14:10 ` Andrey Shinkevich 0 siblings, 0 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-31 14:10 UTC (permalink / raw) To: Paolo Bonzini, Christophe de Dinechin, qemu-devel Cc: Philippe Mathieu-Daudé, qemu-block, Vladimir Sementsov-Ogievskiy, berto, ehabkost, kvm, mtosatti, mdroth, armbru, Denis Lunev, rth On 31/07/2019 15:32, Paolo Bonzini wrote: > On 31/07/19 11:05, Christophe de Dinechin wrote: >> >> Christian Borntraeger writes: >> >>> On 30.07.19 18:44, Philippe Mathieu-Daudé wrote: >>>> On 7/30/19 6:01 PM, Andrey Shinkevich wrote: >>>>> Not the whole structure is initialized before passing it to the KVM. >>>>> Reduce the number of Valgrind reports. >>>>> >>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>>> --- >>>>> target/i386/kvm.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>>> index dbbb137..ed57e31 100644 >>>>> --- a/target/i386/kvm.c >>>>> +++ b/target/i386/kvm.c >>>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>>> return 0; >>>>> } >>>>> >>>>> + memset(&msr_data, 0, sizeof(msr_data)); >>>> >>>> I wonder the overhead of this one... >>> >>> Cant we use designated initializers like in >>> >>> commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86 >>> Author: Christian Borntraeger <borntraeger@de.ibm.com> >>> AuthorDate: Thu Oct 30 09:23:41 2014 +0100 >>> Commit: Paolo Bonzini <pbonzini@redhat.com> >>> CommitDate: Mon Dec 15 12:21:01 2014 +0100 >>> >>> valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl >>> >>> and others? >>> >>> This should minimize the impact. >> >> Oh, when you talked about using designated initializers, I thought you >> were talking about fully initializing the struct, like so: > > Yeah, that would be good too. For now I'm applying Andrey's series though. > > Paolo > Thank you. As Philippe wrote, 'dbgregs.flags = 0;' is unnecessary with 'memset(0)'. Andrey >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >> index dbbb13772a..3533870c43 100644 >> --- a/target/i386/kvm.c >> +++ b/target/i386/kvm.c >> @@ -180,19 +180,20 @@ static int kvm_get_tsc(CPUState *cs) >> { >> X86CPU *cpu = X86_CPU(cs); >> CPUX86State *env = &cpu->env; >> - struct { >> - struct kvm_msrs info; >> - struct kvm_msr_entry entries[1]; >> - } msr_data; >> int ret; >> >> if (env->tsc_valid) { >> return 0; >> } >> >> - msr_data.info.nmsrs = 1; >> - msr_data.entries[0].index = MSR_IA32_TSC; >> - env->tsc_valid = !runstate_is_running(); >> + struct { >> + struct kvm_msrs info; >> + struct kvm_msr_entry entries[1]; >> + } msr_data = { >> + .info = { .nmsrs = 1 }, >> + .entries = { [0] = { .index = MSR_IA32_TSC } } >> + }; >> + env->tsc_valid = !runstate_is_running(); >> >> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data); >> if (ret < 0) { >> >> >> This gives the compiler maximum opportunities to flag mistakes like >> initializing the same thing twice, and make it easier (read no smart >> optimizations) to initialize in one go. Moving the declaration past the >> 'if' also addresses Philippe's concern. >> >>>> >>>>> msr_data.info.nmsrs = 1; >>>>> msr_data.entries[0].index = MSR_IA32_TSC; >>>>> env->tsc_valid = !runstate_is_running(); >>>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>>> >>>>> if (has_xsave) { >>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >>>> >>>> OK >>>> >>>>> } >>>>> >>>>> max_nested_state_len = kvm_max_nested_state_length(); >>>>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>>>> return 0; >>>>> } >>>>> >>>>> + memset(&dbgregs, 0, sizeof(dbgregs)); >>>> >>>> OK >>>> >>>>> for (i = 0; i < 4; i++) { >>>>> dbgregs.db[i] = env->dr[i]; >>>>> } >>>> >>>> We could remove 'dbgregs.flags = 0;' >>>> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> >> >> >> -- >> Cheers, >> Christophe de Dinechin (IRC c3d) >> > -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-07-30 17:05 ` Christian Borntraeger @ 2019-07-30 19:22 ` Paolo Bonzini 1 sibling, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2019-07-30 19:22 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Andrey Shinkevich, qemu-devel, qemu-block Cc: vsementsov, berto, ehabkost, kvm, mtosatti, mdroth, armbru, den, rth On 30/07/19 18:44, Philippe Mathieu-Daudé wrote: >> +++ b/target/i386/kvm.c >> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >> return 0; >> } >> >> + memset(&msr_data, 0, sizeof(msr_data)); > I wonder the overhead of this one... > There is just one MSR in the struct so it is okay. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé @ 2019-07-30 16:46 ` Peter Maydell 2019-07-30 17:09 ` Christian Borntraeger 2019-07-30 19:20 ` Paolo Bonzini 2 siblings, 1 reply; 25+ messages in thread From: Peter Maydell @ 2019-07-30 16:46 UTC (permalink / raw) To: Andrey Shinkevich Cc: QEMU Developers, Qemu-block, Vladimir Sementsov-Ogievskiy, Alberto Garcia, Eduardo Habkost, kvm-devel, Marcelo Tosatti, Michael Roth, Markus Armbruster, Denis V. Lunev, Paolo Bonzini, Richard Henderson On Tue, 30 Jul 2019 at 17:05, Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> wrote: > > Not the whole structure is initialized before passing it to the KVM. > Reduce the number of Valgrind reports. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Does it even make sense to try to valgrind a KVM-enabled run of QEMU? As soon as we run the guest it will make modifications to memory which Valgrind can't track; and I don't think Valgrind supports the KVM_RUN ioctl anyway... thanks -- PMM ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:46 ` Peter Maydell @ 2019-07-30 17:09 ` Christian Borntraeger 0 siblings, 0 replies; 25+ messages in thread From: Christian Borntraeger @ 2019-07-30 17:09 UTC (permalink / raw) To: Peter Maydell, Andrey Shinkevich Cc: QEMU Developers, Qemu-block, Vladimir Sementsov-Ogievskiy, Alberto Garcia, Eduardo Habkost, kvm-devel, Marcelo Tosatti, Michael Roth, Markus Armbruster, Denis V. Lunev, Paolo Bonzini, Richard Henderson On 30.07.19 18:46, Peter Maydell wrote: > On Tue, 30 Jul 2019 at 17:05, Andrey Shinkevich > <andrey.shinkevich@virtuozzo.com> wrote: >> >> Not the whole structure is initialized before passing it to the KVM. >> Reduce the number of Valgrind reports. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > > Does it even make sense to try to valgrind a KVM-enabled run > of QEMU? As soon as we run the guest it will make modifications > to memory which Valgrind can't track; and I don't think > Valgrind supports the KVM_RUN ioctl anyway... As long as we do not care about the guest memory, it does make sense and it does find bugs. See also https://www.linux-kvm.org/page/KVM_Forum_2014 https://www.linux-kvm.org/images/d/d2/03x07-Valgrind.pdf Unfortunately I wasnt able to follow up on those. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-07-30 16:46 ` Peter Maydell @ 2019-07-30 19:20 ` Paolo Bonzini 2019-07-31 7:24 ` Christian Borntraeger 2 siblings, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2019-07-30 19:20 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, den, vsementsov, Christian Borntraeger On 30/07/19 18:01, Andrey Shinkevich wrote: > Not the whole structure is initialized before passing it to the KVM. > Reduce the number of Valgrind reports. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Christian, is this the right fix? It's not expensive so it wouldn't be an issue, just checking if there's any better alternative. Paolo > --- > target/i386/kvm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index dbbb137..ed57e31 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) > return 0; > } > > + memset(&msr_data, 0, sizeof(msr_data)); > msr_data.info.nmsrs = 1; > msr_data.entries[0].index = MSR_IA32_TSC; > env->tsc_valid = !runstate_is_running(); > @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > > if (has_xsave) { > env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); > + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); > } > > max_nested_state_len = kvm_max_nested_state_length(); > @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) > return 0; > } > > + memset(&dbgregs, 0, sizeof(dbgregs)); > for (i = 0; i < 4; i++) { > dbgregs.db[i] = env->dr[i]; > } > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-30 19:20 ` Paolo Bonzini @ 2019-07-31 7:24 ` Christian Borntraeger 2019-07-31 12:04 ` Andrey Shinkevich 0 siblings, 1 reply; 25+ messages in thread From: Christian Borntraeger @ 2019-07-31 7:24 UTC (permalink / raw) To: Paolo Bonzini, Andrey Shinkevich, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, den, vsementsov On 30.07.19 21:20, Paolo Bonzini wrote: > On 30/07/19 18:01, Andrey Shinkevich wrote: >> Not the whole structure is initialized before passing it to the KVM. >> Reduce the number of Valgrind reports. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > > Christian, is this the right fix? It's not expensive so it wouldn't be > an issue, just checking if there's any better alternative. I think all of these variants are valid with pros and cons 1. teach valgrind about this: Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files) knowledge about which parts are actually touched. 2. use designated initializers 3. use memset 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined > > Paolo > >> --- >> target/i386/kvm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >> index dbbb137..ed57e31 100644 >> --- a/target/i386/kvm.c >> +++ b/target/i386/kvm.c >> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >> return 0; >> } >> >> + memset(&msr_data, 0, sizeof(msr_data)); >> msr_data.info.nmsrs = 1; >> msr_data.entries[0].index = MSR_IA32_TSC; >> env->tsc_valid = !runstate_is_running(); >> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >> >> if (has_xsave) { >> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >> } >> >> max_nested_state_len = kvm_max_nested_state_length(); >> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >> return 0; >> } >> >> + memset(&dbgregs, 0, sizeof(dbgregs)); >> for (i = 0; i < 4; i++) { >> dbgregs.db[i] = env->dr[i]; >> } >> -- >> 1.8.3.1 >> > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 7:24 ` Christian Borntraeger @ 2019-07-31 12:04 ` Andrey Shinkevich 2019-07-31 12:28 ` Christian Borntraeger 0 siblings, 1 reply; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-31 12:04 UTC (permalink / raw) To: Christian Borntraeger, Paolo Bonzini, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 31/07/2019 10:24, Christian Borntraeger wrote: > > > On 30.07.19 21:20, Paolo Bonzini wrote: >> On 30/07/19 18:01, Andrey Shinkevich wrote: >>> Not the whole structure is initialized before passing it to the KVM. >>> Reduce the number of Valgrind reports. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> >> Christian, is this the right fix? It's not expensive so it wouldn't be >> an issue, just checking if there's any better alternative. > > I think all of these variants are valid with pros and cons > 1. teach valgrind about this: > Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files) > knowledge about which parts are actually touched. > 2. use designated initializers > 3. use memset > 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined > Thank you all very much for taking part in the discussion. Also, one may use the Valgrind technology to suppress the unwanted reports by adding the Valgrind specific format file valgrind.supp to the QEMU project. The file content is extendable for future needs. All the cases we like to suppress will be recounted in that file. A case looks like the stack fragments. For instance, from QEMU block: { hw/block/hd-geometry.c Memcheck:Cond fun:guess_disk_lchs fun:hd_geometry_guess fun:blkconf_geometry ... fun:device_set_realized fun:property_set_bool fun:object_property_set fun:object_property_set_qobject fun:object_property_set_bool } The number of suppressed cases are reported by the Valgrind with every run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)" Andrey >> >> Paolo >> >>> --- >>> target/i386/kvm.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>> index dbbb137..ed57e31 100644 >>> --- a/target/i386/kvm.c >>> +++ b/target/i386/kvm.c >>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>> return 0; >>> } >>> >>> + memset(&msr_data, 0, sizeof(msr_data)); >>> msr_data.info.nmsrs = 1; >>> msr_data.entries[0].index = MSR_IA32_TSC; >>> env->tsc_valid = !runstate_is_running(); >>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>> >>> if (has_xsave) { >>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >>> } >>> >>> max_nested_state_len = kvm_max_nested_state_length(); >>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>> return 0; >>> } >>> >>> + memset(&dbgregs, 0, sizeof(dbgregs)); >>> for (i = 0; i < 4; i++) { >>> dbgregs.db[i] = env->dr[i]; >>> } >>> -- >>> 1.8.3.1 >>> >> >> > -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 12:04 ` Andrey Shinkevich @ 2019-07-31 12:28 ` Christian Borntraeger 2019-07-31 12:43 ` Christian Borntraeger 0 siblings, 1 reply; 25+ messages in thread From: Christian Borntraeger @ 2019-07-31 12:28 UTC (permalink / raw) To: Andrey Shinkevich, Paolo Bonzini, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 31.07.19 14:04, Andrey Shinkevich wrote: > On 31/07/2019 10:24, Christian Borntraeger wrote: >> >> >> On 30.07.19 21:20, Paolo Bonzini wrote: >>> On 30/07/19 18:01, Andrey Shinkevich wrote: >>>> Not the whole structure is initialized before passing it to the KVM. >>>> Reduce the number of Valgrind reports. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> >>> Christian, is this the right fix? It's not expensive so it wouldn't be >>> an issue, just checking if there's any better alternative. >> >> I think all of these variants are valid with pros and cons >> 1. teach valgrind about this: >> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files) >> knowledge about which parts are actually touched. >> 2. use designated initializers >> 3. use memset >> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined >> > > Thank you all very much for taking part in the discussion. > Also, one may use the Valgrind technology to suppress the unwanted > reports by adding the Valgrind specific format file valgrind.supp to the > QEMU project. The file content is extendable for future needs. > All the cases we like to suppress will be recounted in that file. > A case looks like the stack fragments. For instance, from QEMU block: > > { > hw/block/hd-geometry.c > Memcheck:Cond > fun:guess_disk_lchs > fun:hd_geometry_guess > fun:blkconf_geometry > ... > fun:device_set_realized > fun:property_set_bool > fun:object_property_set > fun:object_property_set_qobject > fun:object_property_set_bool > } > > The number of suppressed cases are reported by the Valgrind with every > run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)" > > Andrey Yes, indeed that would be another variant. How performance critical are the fixed locations? That might have an impact on what is the best solution. From a cleanliness approach doing 1 (adding the ioctl definition to valgrind) is certainly the most beautiful way. I did that in the past, look for example at https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=c2baee9b7bf043702c130de0771a4df439fcf403 or https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=00a31dd3d1e7101b331c2c83fca6c666ba35d910 for examples. > >>> >>> Paolo >>> >>>> --- >>>> target/i386/kvm.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>> index dbbb137..ed57e31 100644 >>>> --- a/target/i386/kvm.c >>>> +++ b/target/i386/kvm.c >>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>> return 0; >>>> } >>>> >>>> + memset(&msr_data, 0, sizeof(msr_data)); >>>> msr_data.info.nmsrs = 1; >>>> msr_data.entries[0].index = MSR_IA32_TSC; >>>> env->tsc_valid = !runstate_is_running(); >>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>> >>>> if (has_xsave) { >>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); >>>> } >>>> >>>> max_nested_state_len = kvm_max_nested_state_length(); >>>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu) >>>> return 0; >>>> } >>>> >>>> + memset(&dbgregs, 0, sizeof(dbgregs)); >>>> for (i = 0; i < 4; i++) { >>>> dbgregs.db[i] = env->dr[i]; >>>> } >>>> -- >>>> 1.8.3.1 >>>> >>> >>> >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 12:28 ` Christian Borntraeger @ 2019-07-31 12:43 ` Christian Borntraeger 2019-07-31 13:03 ` Paolo Bonzini 2019-07-31 14:11 ` Andrey Shinkevich 0 siblings, 2 replies; 25+ messages in thread From: Christian Borntraeger @ 2019-07-31 12:43 UTC (permalink / raw) To: Andrey Shinkevich, Paolo Bonzini, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 31.07.19 14:28, Christian Borntraeger wrote: > > > On 31.07.19 14:04, Andrey Shinkevich wrote: >> On 31/07/2019 10:24, Christian Borntraeger wrote: >>> >>> >>> On 30.07.19 21:20, Paolo Bonzini wrote: >>>> On 30/07/19 18:01, Andrey Shinkevich wrote: >>>>> Not the whole structure is initialized before passing it to the KVM. >>>>> Reduce the number of Valgrind reports. >>>>> >>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> >>>> Christian, is this the right fix? It's not expensive so it wouldn't be >>>> an issue, just checking if there's any better alternative. >>> >>> I think all of these variants are valid with pros and cons >>> 1. teach valgrind about this: >>> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files) >>> knowledge about which parts are actually touched. >>> 2. use designated initializers >>> 3. use memset >>> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined >>> >> >> Thank you all very much for taking part in the discussion. >> Also, one may use the Valgrind technology to suppress the unwanted >> reports by adding the Valgrind specific format file valgrind.supp to the >> QEMU project. The file content is extendable for future needs. >> All the cases we like to suppress will be recounted in that file. >> A case looks like the stack fragments. For instance, from QEMU block: >> >> { >> hw/block/hd-geometry.c >> Memcheck:Cond >> fun:guess_disk_lchs >> fun:hd_geometry_guess >> fun:blkconf_geometry >> ... >> fun:device_set_realized >> fun:property_set_bool >> fun:object_property_set >> fun:object_property_set_qobject >> fun:object_property_set_bool >> } >> >> The number of suppressed cases are reported by the Valgrind with every >> run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)" >> >> Andrey > > Yes, indeed that would be another variant. How performance critical are > the fixed locations? That might have an impact on what is the best solution. > From a cleanliness approach doing 1 (adding the ioctl definition to valgrind) > is certainly the most beautiful way. I did that in the past, look for example at > > https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=c2baee9b7bf043702c130de0771a4df439fcf403 > or > https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=00a31dd3d1e7101b331c2c83fca6c666ba35d910 > > for examples. > > >> >>>> >>>> Paolo >>>> >>>>> --- >>>>> target/i386/kvm.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>>> index dbbb137..ed57e31 100644 >>>>> --- a/target/i386/kvm.c >>>>> +++ b/target/i386/kvm.c >>>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>>> return 0; >>>>> } >>>>> >>>>> + memset(&msr_data, 0, sizeof(msr_data)); >>>>> msr_data.info.nmsrs = 1; >>>>> msr_data.entries[0].index = MSR_IA32_TSC; >>>>> env->tsc_valid = !runstate_is_running(); >>>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>>> >>>>> if (has_xsave) { >>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); This is memsetting 4k? Yet another variant would be to use the RUNNING_ON_VALGRIND macro from valgrind/valgrind.h to only memset for valgrind. But just using MAKE_MEM_DEFINED from memcheck.h is simpler. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 12:43 ` Christian Borntraeger @ 2019-07-31 13:03 ` Paolo Bonzini 2019-07-31 14:11 ` Andrey Shinkevich 1 sibling, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2019-07-31 13:03 UTC (permalink / raw) To: Christian Borntraeger, Andrey Shinkevich, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 31/07/19 14:43, Christian Borntraeger wrote: >>>>>> if (has_xsave) { >>>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); > This is memsetting 4k? > Yet another variant would be to use the RUNNING_ON_VALGRIND macro from > valgrind/valgrind.h to only memset for valgrind. But just using MAKE_MEM_DEFINED > from memcheck.h is simpler. > Yes, it's 4k but only at initialization time and I actually prefer not to have potentially uninitialized host data in there. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call 2019-07-31 12:43 ` Christian Borntraeger 2019-07-31 13:03 ` Paolo Bonzini @ 2019-07-31 14:11 ` Andrey Shinkevich 1 sibling, 0 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-07-31 14:11 UTC (permalink / raw) To: Christian Borntraeger, Paolo Bonzini, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 31/07/2019 15:43, Christian Borntraeger wrote: > > > On 31.07.19 14:28, Christian Borntraeger wrote: >> >> >> On 31.07.19 14:04, Andrey Shinkevich wrote: >>> On 31/07/2019 10:24, Christian Borntraeger wrote: >>>> >>>> >>>> On 30.07.19 21:20, Paolo Bonzini wrote: >>>>> On 30/07/19 18:01, Andrey Shinkevich wrote: >>>>>> Not the whole structure is initialized before passing it to the KVM. >>>>>> Reduce the number of Valgrind reports. >>>>>> >>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>>> >>>>> Christian, is this the right fix? It's not expensive so it wouldn't be >>>>> an issue, just checking if there's any better alternative. >>>> >>>> I think all of these variants are valid with pros and cons >>>> 1. teach valgrind about this: >>>> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files) >>>> knowledge about which parts are actually touched. >>>> 2. use designated initializers >>>> 3. use memset >>>> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined >>>> >>> >>> Thank you all very much for taking part in the discussion. >>> Also, one may use the Valgrind technology to suppress the unwanted >>> reports by adding the Valgrind specific format file valgrind.supp to the >>> QEMU project. The file content is extendable for future needs. >>> All the cases we like to suppress will be recounted in that file. >>> A case looks like the stack fragments. For instance, from QEMU block: >>> >>> { >>> hw/block/hd-geometry.c >>> Memcheck:Cond >>> fun:guess_disk_lchs >>> fun:hd_geometry_guess >>> fun:blkconf_geometry >>> ... >>> fun:device_set_realized >>> fun:property_set_bool >>> fun:object_property_set >>> fun:object_property_set_qobject >>> fun:object_property_set_bool >>> } >>> >>> The number of suppressed cases are reported by the Valgrind with every >>> run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)" >>> >>> Andrey >> >> Yes, indeed that would be another variant. How performance critical are >> the fixed locations? That might have an impact on what is the best solution. >> From a cleanliness approach doing 1 (adding the ioctl definition to valgrind) >> is certainly the most beautiful way. I did that in the past, look for example at >> >> https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=c2baee9b7bf043702c130de0771a4df439fcf403 >> or >> https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=00a31dd3d1e7101b331c2c83fca6c666ba35d910 >> >> for examples. >> >> >>> >>>>> >>>>> Paolo >>>>> >>>>>> --- >>>>>> target/i386/kvm.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c >>>>>> index dbbb137..ed57e31 100644 >>>>>> --- a/target/i386/kvm.c >>>>>> +++ b/target/i386/kvm.c >>>>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> + memset(&msr_data, 0, sizeof(msr_data)); >>>>>> msr_data.info.nmsrs = 1; >>>>>> msr_data.entries[0].index = MSR_IA32_TSC; >>>>>> env->tsc_valid = !runstate_is_running(); >>>>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>>>> >>>>>> if (has_xsave) { >>>>>> env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); >>>>>> + memset(env->xsave_buf, 0, sizeof(struct kvm_xsave)); > > This is memsetting 4k? > Yet another variant would be to use the RUNNING_ON_VALGRIND macro from > valgrind/valgrind.h to only memset for valgrind. But just using MAKE_MEM_DEFINED > from memcheck.h is simpler. > So, on this assumption, the code would look like #ifdef CONFIG_VALGRIND_H #include <valgrind/memcheck.h> #endif #ifdef CONFIG_VALGRIND_H VALGRIND_MAKE_MEM_DEFINED(&msr_data, sizeof(msr_data)); #endif etc. Andrey -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/3] Reduce the number of Valgrind reports in unit tests. 2019-07-30 16:01 [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich ` (2 preceding siblings ...) 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich @ 2019-08-13 12:02 ` Andrey Shinkevich 2019-08-13 12:05 ` Paolo Bonzini 3 siblings, 1 reply; 25+ messages in thread From: Andrey Shinkevich @ 2019-08-13 12:02 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, pbonzini, Denis Lunev, Vladimir Sementsov-Ogievskiy PINGING... On 30/07/2019 19:01, Andrey Shinkevich wrote: > Running unit tests under the Valgrind may help to detect QEMU memory issues > (suggested by Denis V. Lunev). Some of the Valgrind reports relate to the > unit test code itself. Let's eliminate the detected memory issues to ease > locating critical ones. > > Andrey Shinkevich (3): > test-throttle: Fix uninitialized use of burst_length > tests: Fix uninitialized byte in test_visitor_in_fuzz > i386/kvm: initialize struct at full before ioctl call > > target/i386/kvm.c | 3 +++ > tests/test-string-input-visitor.c | 8 +++----- > tests/test-throttle.c | 2 ++ > 3 files changed, 8 insertions(+), 5 deletions(-) > -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/3] Reduce the number of Valgrind reports in unit tests. 2019-08-13 12:02 ` [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich @ 2019-08-13 12:05 ` Paolo Bonzini 2019-08-13 12:08 ` Andrey Shinkevich 0 siblings, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2019-08-13 12:05 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 13/08/19 14:02, Andrey Shinkevich wrote: > PINGING... I thought I had already said I queued the series? Paolo > On 30/07/2019 19:01, Andrey Shinkevich wrote: >> Running unit tests under the Valgrind may help to detect QEMU memory issues >> (suggested by Denis V. Lunev). Some of the Valgrind reports relate to the >> unit test code itself. Let's eliminate the detected memory issues to ease >> locating critical ones. >> >> Andrey Shinkevich (3): >> test-throttle: Fix uninitialized use of burst_length >> tests: Fix uninitialized byte in test_visitor_in_fuzz >> i386/kvm: initialize struct at full before ioctl call >> >> target/i386/kvm.c | 3 +++ >> tests/test-string-input-visitor.c | 8 +++----- >> tests/test-throttle.c | 2 ++ >> 3 files changed, 8 insertions(+), 5 deletions(-) >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/3] Reduce the number of Valgrind reports in unit tests. 2019-08-13 12:05 ` Paolo Bonzini @ 2019-08-13 12:08 ` Andrey Shinkevich 0 siblings, 0 replies; 25+ messages in thread From: Andrey Shinkevich @ 2019-08-13 12:08 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, qemu-block Cc: kvm, berto, mdroth, armbru, ehabkost, rth, mtosatti, Denis Lunev, Vladimir Sementsov-Ogievskiy On 13/08/2019 15:05, Paolo Bonzini wrote: > On 13/08/19 14:02, Andrey Shinkevich wrote: >> PINGING... > > I thought I had already said I queued the series? > > Paolo > Thank you Paolo. I am clear now. Andrey >> On 30/07/2019 19:01, Andrey Shinkevich wrote: >>> Running unit tests under the Valgrind may help to detect QEMU memory issues >>> (suggested by Denis V. Lunev). Some of the Valgrind reports relate to the >>> unit test code itself. Let's eliminate the detected memory issues to ease >>> locating critical ones. >>> >>> Andrey Shinkevich (3): >>> test-throttle: Fix uninitialized use of burst_length >>> tests: Fix uninitialized byte in test_visitor_in_fuzz >>> i386/kvm: initialize struct at full before ioctl call >>> >>> target/i386/kvm.c | 3 +++ >>> tests/test-string-input-visitor.c | 8 +++----- >>> tests/test-throttle.c | 2 ++ >>> 3 files changed, 8 insertions(+), 5 deletions(-) >>> >> > -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-08-13 12:37 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-30 16:01 [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 1/3] test-throttle: Fix uninitialized use of burst_length Andrey Shinkevich 2019-08-13 12:19 ` Alberto Garcia 2019-07-30 16:01 ` [PATCH 2/3] tests: Fix uninitialized byte in test_visitor_in_fuzz Andrey Shinkevich 2019-07-30 16:01 ` [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call Andrey Shinkevich 2019-07-30 16:44 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-07-30 17:05 ` Christian Borntraeger 2019-07-30 17:14 ` Philippe Mathieu-Daudé 2019-07-30 17:47 ` Christian Borntraeger 2019-07-31 9:05 ` Christophe de Dinechin 2019-07-31 12:32 ` Paolo Bonzini 2019-07-31 14:10 ` Andrey Shinkevich 2019-07-30 19:22 ` Paolo Bonzini 2019-07-30 16:46 ` Peter Maydell 2019-07-30 17:09 ` Christian Borntraeger 2019-07-30 19:20 ` Paolo Bonzini 2019-07-31 7:24 ` Christian Borntraeger 2019-07-31 12:04 ` Andrey Shinkevich 2019-07-31 12:28 ` Christian Borntraeger 2019-07-31 12:43 ` Christian Borntraeger 2019-07-31 13:03 ` Paolo Bonzini 2019-07-31 14:11 ` Andrey Shinkevich 2019-08-13 12:02 ` [PATCH 0/3] Reduce the number of Valgrind reports in unit tests Andrey Shinkevich 2019-08-13 12:05 ` Paolo Bonzini 2019-08-13 12:08 ` Andrey Shinkevich
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).