On 11/20/2017 02:00 PM, Cornelia Huck wrote: > On Mon, 20 Nov 2017 13:35:25 +0100 > Christian Borntraeger <[email protected]> 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 <[email protected]> >> --- >> 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; >> } >
