On Mon, 20 Nov 2017 14:11:03 +0100
Christian Borntraeger <borntrae...@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 <borntrae...@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 <borntrae...@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.
