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

[Christian Borntraeger]

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