* [PATCH] s390x/kvm: help valgrind in several places @ 2020-04-28 18:31 Christian Borntraeger 2020-04-29 7:00 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 4+ messages in thread From: Christian Borntraeger @ 2020-04-28 18:31 UTC (permalink / raw) To: qemu-devel, Cornelia Huck Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Halil Pasic, Christian Borntraeger, qemu-s390x, Richard Henderson We need some little help in the code to reduce the valgrind noise. - some designated initializers for the cpu model features and subfunctions - mark memory as defined for sida memory reads Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- target/s390x/kvm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 69881a0da0..bcd0ee0d14 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -52,6 +52,10 @@ #include "hw/s390x/s390-virtio-hcall.h" #include "hw/s390x/pv.h" +#ifdef CONFIG_VALGRIND_H +#include <valgrind/memcheck.h> +#endif + #ifndef DEBUG_KVM #define DEBUG_KVM 0 #endif @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf, error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret)); abort(); } + +#ifdef CONFIG_VALGRIND_H + if (!is_write) { + VALGRIND_MAKE_MEM_DEFINED(hostbuf, len); + } +#endif + return ret; } @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) static int query_cpu_subfunc(S390FeatBitmap features) { - struct kvm_s390_vm_cpu_subfunc prop; + struct kvm_s390_vm_cpu_subfunc prop = {}; struct kvm_device_attr attr = { .group = KVM_S390_VM_CPU_MODEL, .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC, @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = { static int query_cpu_feat(S390FeatBitmap features) { - struct kvm_s390_vm_cpu_feat prop; + struct kvm_s390_vm_cpu_feat prop = {}; struct kvm_device_attr attr = { .group = KVM_S390_VM_CPU_MODEL, .attr = KVM_S390_VM_CPU_MACHINE_FEAT, -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] s390x/kvm: help valgrind in several places 2020-04-28 18:31 [PATCH] s390x/kvm: help valgrind in several places Christian Borntraeger @ 2020-04-29 7:00 ` Philippe Mathieu-Daudé 2020-04-29 7:21 ` Christian Borntraeger 0 siblings, 1 reply; 4+ messages in thread From: Philippe Mathieu-Daudé @ 2020-04-29 7:00 UTC (permalink / raw) To: Christian Borntraeger, qemu-devel, Cornelia Huck Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Halil Pasic, qemu-s390x, Richard Henderson Hi Christian, On 4/28/20 8:31 PM, Christian Borntraeger wrote: > We need some little help in the code to reduce the valgrind noise. > - some designated initializers for the cpu model features and subfunctions ^ This could go as trivial patch while we discuss the rest. > - mark memory as defined for sida memory reads > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- I couldn't apply this patch, then figured out it targets s390-next. > target/s390x/kvm.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 69881a0da0..bcd0ee0d14 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -52,6 +52,10 @@ > #include "hw/s390x/s390-virtio-hcall.h" > #include "hw/s390x/pv.h" > > +#ifdef CONFIG_VALGRIND_H > +#include <valgrind/memcheck.h> > +#endif > + > #ifndef DEBUG_KVM > #define DEBUG_KVM 0 > #endif > @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf, > error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret)); > abort(); > } What about kvm_s390_mem_op()? > + > +#ifdef CONFIG_VALGRIND_H > + if (!is_write) { > + VALGRIND_MAKE_MEM_DEFINED(hostbuf, len); > + } > +#endif I agree with this macro usage, but think it should be widely accessible by the whole codebase (and other targets). "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h" is a better place for common helpers. If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place. > + > return ret; > } > > @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) > > static int query_cpu_subfunc(S390FeatBitmap features) > { > - struct kvm_s390_vm_cpu_subfunc prop; > + struct kvm_s390_vm_cpu_subfunc prop = {}; > struct kvm_device_attr attr = { > .group = KVM_S390_VM_CPU_MODEL, > .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC, > @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = { > > static int query_cpu_feat(S390FeatBitmap features) > { > - struct kvm_s390_vm_cpu_feat prop; > + struct kvm_s390_vm_cpu_feat prop = {}; > struct kvm_device_attr attr = { > .group = KVM_S390_VM_CPU_MODEL, > .attr = KVM_S390_VM_CPU_MACHINE_FEAT, > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] s390x/kvm: help valgrind in several places 2020-04-29 7:00 ` Philippe Mathieu-Daudé @ 2020-04-29 7:21 ` Christian Borntraeger 2020-04-29 7:32 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 4+ messages in thread From: Christian Borntraeger @ 2020-04-29 7:21 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, Cornelia Huck Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Halil Pasic, qemu-s390x, Richard Henderson On 29.04.20 09:00, Philippe Mathieu-Daudé wrote: > Hi Christian, > > On 4/28/20 8:31 PM, Christian Borntraeger wrote: >> We need some little help in the code to reduce the valgrind noise. >> - some designated initializers for the cpu model features and subfunctions > > ^ This could go as trivial patch while we discuss the rest. I can certainly split. > >> - mark memory as defined for sida memory reads >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- > > I couldn't apply this patch, then figured out it targets s390-next. > >> target/s390x/kvm.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 69881a0da0..bcd0ee0d14 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -52,6 +52,10 @@ >> #include "hw/s390x/s390-virtio-hcall.h" >> #include "hw/s390x/pv.h" >> +#ifdef CONFIG_VALGRIND_H >> +#include <valgrind/memcheck.h> >> +#endif >> + >> #ifndef DEBUG_KVM >> #define DEBUG_KVM 0 >> #endif >> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf, >> error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret)); >> abort(); >> } > > What about kvm_s390_mem_op()? I have not triggered something in here, but you are right, there should be cases where we make conditions depend of that content. Will change my testing and add something here as well. > >> + >> +#ifdef CONFIG_VALGRIND_H >> + if (!is_write) { >> + VALGRIND_MAKE_MEM_DEFINED(hostbuf, len); >> + } >> +#endif > > I agree with this macro usage, but think it should be widely accessible by the whole codebase (and other targets). > > "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h" is a better place for common helpers. > > If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place. This macro IS available for the whole codebase if you include valgrind/memcheck.h. We used it in the past (before 2.2) for kvm memory. See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark memory as initialized). The only thing that we could discuss is introducing a new global function like valgrind_make_mem_defined that would hide the ifdefs. Is there interest in such a thing? It is likely that these corner cases (valgrind not able to see that this is defined) are more likely o happen with KVM. But it would be useful for anything not only KVM. > >> + >> return ret; >> } >> @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) >> static int query_cpu_subfunc(S390FeatBitmap features) >> { >> - struct kvm_s390_vm_cpu_subfunc prop; >> + struct kvm_s390_vm_cpu_subfunc prop = {}; >> struct kvm_device_attr attr = { >> .group = KVM_S390_VM_CPU_MODEL, >> .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC, >> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = { >> static int query_cpu_feat(S390FeatBitmap features) >> { >> - struct kvm_s390_vm_cpu_feat prop; >> + struct kvm_s390_vm_cpu_feat prop = {}; >> struct kvm_device_attr attr = { >> .group = KVM_S390_VM_CPU_MODEL, >> .attr = KVM_S390_VM_CPU_MACHINE_FEAT, >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] s390x/kvm: help valgrind in several places 2020-04-29 7:21 ` Christian Borntraeger @ 2020-04-29 7:32 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 4+ messages in thread From: Philippe Mathieu-Daudé @ 2020-04-29 7:32 UTC (permalink / raw) To: Christian Borntraeger, qemu-devel, Cornelia Huck Cc: Thomas Huth, Janosch Frank, David Hildenbrand, Halil Pasic, qemu-s390x, Paolo Bonzini, Richard Henderson +Paolo On 4/29/20 9:21 AM, Christian Borntraeger wrote: > > On 29.04.20 09:00, Philippe Mathieu-Daudé wrote: >> Hi Christian, >> >> On 4/28/20 8:31 PM, Christian Borntraeger wrote: >>> We need some little help in the code to reduce the valgrind noise. >>> - some designated initializers for the cpu model features and subfunctions >> >> ^ This could go as trivial patch while we discuss the rest. > > I can certainly split. If you split then please directly include "Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>" to the it. > >> >>> - mark memory as defined for sida memory reads >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >> >> I couldn't apply this patch, then figured out it targets s390-next. >> >>> target/s390x/kvm.c | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 69881a0da0..bcd0ee0d14 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -52,6 +52,10 @@ >>> #include "hw/s390x/s390-virtio-hcall.h" >>> #include "hw/s390x/pv.h" >>> +#ifdef CONFIG_VALGRIND_H >>> +#include <valgrind/memcheck.h> >>> +#endif >>> + >>> #ifndef DEBUG_KVM >>> #define DEBUG_KVM 0 >>> #endif >>> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf, >>> error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret)); >>> abort(); >>> } >> >> What about kvm_s390_mem_op()? > > I have not triggered something in here, but you are right, there should be cases where we make conditions > depend of that content. Will change my testing and add something here as well. >> >>> + >>> +#ifdef CONFIG_VALGRIND_H >>> + if (!is_write) { >>> + VALGRIND_MAKE_MEM_DEFINED(hostbuf, len); >>> + } >>> +#endif >> >> I agree with this macro usage, but think it should be widely accessible by the whole codebase (and other targets). >> >> "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h" is a better place for common helpers. >> >> If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place. > > This macro IS available for the whole codebase if you include valgrind/memcheck.h. > We used it in the past (before 2.2) for kvm memory. > See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark memory as initialized). > > The only thing that we could discuss is introducing a new global function like > valgrind_make_mem_defined that would hide the ifdefs. > Is there interest in such a thing? > It is likely that these corner cases (valgrind not able to see that this is defined) are more likely > o happen with KVM. But it would be useful for anything not only KVM. Correct. What I wanted to say here is, if we can use this code elsewhere, then it is worth adding a global helper (generic or KVM). If you reuse this code twice, having an inlined function makes the code more readable anyway. See for example in util/coroutine-ucontext.c: static inline void valgrind_stack_deregister(CoroutineUContext *co) { VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id); } Maybe we could have something like: static inline void valgrind_define_memory(void *ptr, size_t size, bool is_defined) { if (is_defined) { VALGRIND_MAKE_MEM_DEFINED(ptr, size); } } > >> >>> + >>> return ret; >>> } >>> @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) >>> static int query_cpu_subfunc(S390FeatBitmap features) >>> { >>> - struct kvm_s390_vm_cpu_subfunc prop; >>> + struct kvm_s390_vm_cpu_subfunc prop = {}; >>> struct kvm_device_attr attr = { >>> .group = KVM_S390_VM_CPU_MODEL, >>> .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC, >>> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = { >>> static int query_cpu_feat(S390FeatBitmap features) >>> { >>> - struct kvm_s390_vm_cpu_feat prop; >>> + struct kvm_s390_vm_cpu_feat prop = {}; >>> struct kvm_device_attr attr = { >>> .group = KVM_S390_VM_CPU_MODEL, >>> .attr = KVM_S390_VM_CPU_MACHINE_FEAT, >>> >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-29 8:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 18:31 [PATCH] s390x/kvm: help valgrind in several places Christian Borntraeger 2020-04-29 7:00 ` Philippe Mathieu-Daudé 2020-04-29 7:21 ` Christian Borntraeger 2020-04-29 7:32 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.