Re: [Qemu-devel] [PATCH 2/2] s390x/kvm: use valgrind annotations for kvm device attributes

[Cornelia Huck <cohuck@redhat.com>]

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.