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? 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;
>>  }
>