| Message ID | 20171120123525.147663-3-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | valgrind fallout for s390x/kvm | expand |
On Mon, 20 Nov 2017 13:35:25 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing > side effects. Use valgrind annotations to properly mark > all storage changes instead of using memset or designated > initializers. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/s390-skeys-kvm.c | 12 ++++++++++- > hw/s390x/s390-stattrib-kvm.c | 7 ++++++ > target/s390x/kvm.c | 51 ++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 65 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c > index dc54ed8..0986795 100644 > --- a/hw/s390x/s390-skeys-kvm.c > +++ b/hw/s390x/s390-skeys-kvm.c > @@ -13,6 +13,9 @@ > #include "hw/s390x/storage-keys.h" > #include "sysemu/kvm.h" > #include "qemu/error-report.h" > +#ifdef CONFIG_VALGRIND_H > +#include <valgrind/memcheck.h> > +#endif > > static int kvm_s390_skeys_enabled(S390SKeysState *ss) > { > @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn, > .count = count, > .skeydata_addr = (__u64)keys > }; > + int ret; > > - return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); > + ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); > + if (!ret) { > +#ifdef CONFIG_VALGRIND_H > + VALGRIND_MAKE_MEM_DEFINED(keys, count); > +#endif This looks ugly :( Is s390x the only one hitting those side effects? If we need to sprinkle those all over the source code, it improves valgrind results but makes the code harder to read... (And no, I don't have a better idea.) > + } > + return ret; > }
On 11/20/2017 02:00 PM, Cornelia Huck wrote: > On Mon, 20 Nov 2017 13:35:25 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing >> side effects. Use valgrind annotations to properly mark >> all storage changes instead of using memset or designated >> initializers. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/s390x/s390-skeys-kvm.c | 12 ++++++++++- >> hw/s390x/s390-stattrib-kvm.c | 7 ++++++ >> target/s390x/kvm.c | 51 ++++++++++++++++++++++++++++++++++++++++---- >> 3 files changed, 65 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c >> index dc54ed8..0986795 100644 >> --- a/hw/s390x/s390-skeys-kvm.c >> +++ b/hw/s390x/s390-skeys-kvm.c >> @@ -13,6 +13,9 @@ >> #include "hw/s390x/storage-keys.h" >> #include "sysemu/kvm.h" >> #include "qemu/error-report.h" >> +#ifdef CONFIG_VALGRIND_H >> +#include <valgrind/memcheck.h> >> +#endif >> >> static int kvm_s390_skeys_enabled(S390SKeysState *ss) >> { >> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn, >> .count = count, >> .skeydata_addr = (__u64)keys >> }; >> + int ret; >> >> - return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); >> + ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); >> + if (!ret) { >> +#ifdef CONFIG_VALGRIND_H >> + VALGRIND_MAKE_MEM_DEFINED(keys, count); >> +#endif > > This looks ugly :( > > Is s390x the only one hitting those side effects? If we need to > sprinkle those all over the source code, it improves valgrind results > but makes the code harder to read... > > (And no, I don't have a better idea.) We could provide a wrapper? > >> + } >> + return ret; >> } >
On 11/20/2017 02:00 PM, Cornelia Huck wrote: > On Mon, 20 Nov 2017 13:35:25 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing >> side effects. Use valgrind annotations to properly mark >> all storage changes instead of using memset or designated >> initializers. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/s390x/s390-skeys-kvm.c | 12 ++++++++++- >> hw/s390x/s390-stattrib-kvm.c | 7 ++++++ >> target/s390x/kvm.c | 51 ++++++++++++++++++++++++++++++++++++++++---- >> 3 files changed, 65 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c >> index dc54ed8..0986795 100644 >> --- a/hw/s390x/s390-skeys-kvm.c >> +++ b/hw/s390x/s390-skeys-kvm.c >> @@ -13,6 +13,9 @@ >> #include "hw/s390x/storage-keys.h" >> #include "sysemu/kvm.h" >> #include "qemu/error-report.h" >> +#ifdef CONFIG_VALGRIND_H >> +#include <valgrind/memcheck.h> >> +#endif >> >> static int kvm_s390_skeys_enabled(S390SKeysState *ss) >> { >> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn, >> .count = count, >> .skeydata_addr = (__u64)keys >> }; >> + int ret; >> >> - return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); >> + ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); >> + if (!ret) { >> +#ifdef CONFIG_VALGRIND_H >> + VALGRIND_MAKE_MEM_DEFINED(keys, count); >> +#endif > > This looks ugly :( > > Is s390x the only one hitting those side effects? Almost nobody else uses the device attributes. And when they use it they have the same problem We papered over some bugs by zero-initializing structs but I think marking only the really changed memory will help to detect real bugs (e.g. if code reads ioctl data but the ioctl fails we will not detect this when pre-zeroing. If we need to > sprinkle those all over the source code, it improves valgrind results > but makes the code harder to read... > > (And no, I don't have a better idea.) > >> + } >> + return ret; >> } >
On Mon, 20 Nov 2017 14:11:03 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 11/20/2017 02:00 PM, Cornelia Huck wrote: > > On Mon, 20 Nov 2017 13:35:25 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing > >> side effects. Use valgrind annotations to properly mark > >> all storage changes instead of using memset or designated > >> initializers. > >> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> hw/s390x/s390-skeys-kvm.c | 12 ++++++++++- > >> hw/s390x/s390-stattrib-kvm.c | 7 ++++++ > >> target/s390x/kvm.c | 51 ++++++++++++++++++++++++++++++++++++++++---- > >> 3 files changed, 65 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c > >> index dc54ed8..0986795 100644 > >> --- a/hw/s390x/s390-skeys-kvm.c > >> +++ b/hw/s390x/s390-skeys-kvm.c > >> @@ -13,6 +13,9 @@ > >> #include "hw/s390x/storage-keys.h" > >> #include "sysemu/kvm.h" > >> #include "qemu/error-report.h" > >> +#ifdef CONFIG_VALGRIND_H > >> +#include <valgrind/memcheck.h> > >> +#endif > >> > >> static int kvm_s390_skeys_enabled(S390SKeysState *ss) > >> { > >> @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn, > >> .count = count, > >> .skeydata_addr = (__u64)keys > >> }; > >> + int ret; > >> > >> - return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); > >> + ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); > >> + if (!ret) { > >> +#ifdef CONFIG_VALGRIND_H > >> + VALGRIND_MAKE_MEM_DEFINED(keys, count); > >> +#endif > > > > This looks ugly :( > > > > Is s390x the only one hitting those side effects? > > Almost nobody else uses the device attributes. And when they use it they > have the same problem Makes sense. > We papered over some bugs by zero-initializing structs but I think marking > only the really changed memory will help to detect real bugs (e.g. if code > reads ioctl data but the ioctl fails we will not detect this when pre-zeroing. Yes, from a "finding bugs" standpoint that's definitely an improvement. Maybe a wrapper would improve the readability.
diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c index dc54ed8..0986795 100644 --- a/hw/s390x/s390-skeys-kvm.c +++ b/hw/s390x/s390-skeys-kvm.c @@ -13,6 +13,9 @@ #include "hw/s390x/storage-keys.h" #include "sysemu/kvm.h" #include "qemu/error-report.h" +#ifdef CONFIG_VALGRIND_H +#include <valgrind/memcheck.h> +#endif static int kvm_s390_skeys_enabled(S390SKeysState *ss) { @@ -35,8 +38,15 @@ static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn, .count = count, .skeydata_addr = (__u64)keys }; + int ret; - return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); + ret = kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args); + if (!ret) { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(keys, count); +#endif + } + return ret; } static int kvm_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn, diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c index 41770a7..eb2b220 100644 --- a/hw/s390x/s390-stattrib-kvm.c +++ b/hw/s390x/s390-stattrib-kvm.c @@ -18,6 +18,9 @@ #include "exec/ram_addr.h" #include "cpu.h" #include "kvm_s390x.h" +#ifdef CONFIG_VALGRIND_H +#include <valgrind/memcheck.h> +#endif Object *kvm_s390_stattrib_create(void) { @@ -54,6 +57,10 @@ static int kvm_s390_stattrib_read_helper(S390StAttribState *sa, if (r < 0) { error_report("KVM_S390_GET_CMMA_BITS failed: %s", strerror(-r)); return r; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(values, clog.count); +#endif } *start_gfn = clog.start_gfn; diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index b0439a1..d3f3ddb 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -50,6 +50,9 @@ #include "exec/memattrs.h" #include "hw/s390x/s390-virtio-ccw.h" #include "hw/s390x/s390-virtio-hcall.h" +#ifdef CONFIG_VALGRIND_H +#include <valgrind/memcheck.h> +#endif #ifndef DEBUG_KVM #define DEBUG_KVM 0 @@ -176,8 +179,13 @@ int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit) rc = kvm_s390_query_mem_limit(hw_limit); if (rc) { return rc; - } else if (*hw_limit < new_limit) { - return -E2BIG; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(hw_limit, sizeof(*hw_limit)); +#endif + if (*hw_limit < new_limit) { + return -E2BIG; + } } return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr); @@ -312,6 +320,10 @@ static int compat_disable_facilities(KVMState *s, __u64 fac_mask[], int len) if (rc) { error_report("KVM_GET_DEVICE_ATTR failed with rc %d", rc); return rc; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(&mach_attrs, sizeof(mach_attrs)); +#endif } cpu_attrs.cpuid = mach_attrs.cpuid | 0xff00000000000000UL; @@ -704,11 +716,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low) r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); if (r) { return r; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(tod_low, sizeof(tod_low)); +#endif } attr.attr = KVM_S390_VM_TOD_HIGH; attr.addr = (uint64_t)tod_high; - return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); + r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); + if (!r) { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(tod_high, sizeof(tod_high)); +#endif + } + return r; } int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low) @@ -722,6 +744,11 @@ int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low) }; r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); + if (!r) { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(>od, sizeof(gtod)); +#endif + } *tod_high = gtod.epoch_idx; *tod_low = gtod.tod; @@ -2085,6 +2112,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) cpu->irqstate_saved_size = 0; error_report("Migration of interrupt state failed"); return; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(cpu->irqstate, bytes); +#endif } cpu->irqstate_saved_size = bytes; @@ -2170,6 +2201,10 @@ static int query_cpu_subfunc(S390FeatBitmap features) rc = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); if (rc) { return rc; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop)); +#endif } /* @@ -2280,6 +2315,10 @@ static int query_cpu_feat(S390FeatBitmap features) rc = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); if (rc) { return rc; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop)); +#endif } for (i = 0; i < ARRAY_SIZE(kvm_to_feat); i++) { @@ -2328,7 +2367,7 @@ bool kvm_s390_cpu_models_supported(void) void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) { - struct kvm_s390_vm_cpu_machine prop = {}; + struct kvm_s390_vm_cpu_machine prop; struct kvm_device_attr attr = { .group = KVM_S390_VM_CPU_MODEL, .attr = KVM_S390_VM_CPU_MACHINE, @@ -2349,6 +2388,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) if (rc) { error_setg(errp, "KVM: Error querying host CPU model: %d", rc); return; + } else { +#ifdef CONFIG_VALGRIND_H + VALGRIND_MAKE_MEM_DEFINED(&prop, sizeof(prop)); +#endif } cpu_type = cpuid_type(prop.cpuid);
the KVM_GET/SET_DEVICE_ATTR calls have non-self-describing side effects. Use valgrind annotations to properly mark all storage changes instead of using memset or designated initializers. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- hw/s390x/s390-skeys-kvm.c | 12 ++++++++++- hw/s390x/s390-stattrib-kvm.c | 7 ++++++ target/s390x/kvm.c | 51 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 65 insertions(+), 5 deletions(-)