Re: [Qemu-devel] [PATCH 3/3] i386/kvm: initialize struct at full before ioctl call

[Christophe de Dinechin]

Christian Borntraeger writes:

> On 30.07.19 18:44, Philippe Mathieu-Daudé wrote:
>> On 7/30/19 6:01 PM, Andrey Shinkevich wrote:
>>> Not the whole structure is initialized before passing it to the KVM.
>>> Reduce the number of Valgrind reports.
>>>
>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>> ---
>>>  target/i386/kvm.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>> index dbbb137..ed57e31 100644
>>> --- a/target/i386/kvm.c
>>> +++ b/target/i386/kvm.c
>>> @@ -190,6 +190,7 @@ static int kvm_get_tsc(CPUState *cs)
>>>          return 0;
>>>      }
>>>
>>> +    memset(&msr_data, 0, sizeof(msr_data));
>>
>> I wonder the overhead of this one...
>
> Cant we use designated initializers like in
>
> commit bdfc8480c50a53d91aa9a513d23a84de0d5fbc86
> Author:     Christian Borntraeger <address@hidden>
> AuthorDate: Thu Oct 30 09:23:41 2014 +0100
> Commit:     Paolo Bonzini <address@hidden>
> CommitDate: Mon Dec 15 12:21:01 2014 +0100
>
>     valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl
>
> and others?
>
> This should minimize the impact.

Oh, when you talked about using designated initializers, I thought you
were talking about fully initializing the struct, like so:

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index dbbb13772a..3533870c43 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -180,19 +180,20 @@ static int kvm_get_tsc(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
-    struct {
-        struct kvm_msrs info;
-        struct kvm_msr_entry entries[1];
-    } msr_data;
     int ret;

     if (env->tsc_valid) {
         return 0;
     }

-    msr_data.info.nmsrs = 1;
-    msr_data.entries[0].index = MSR_IA32_TSC;
-    env->tsc_valid = !runstate_is_running();
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[1];
+    } msr_data = {
+        .info = { .nmsrs =  1 },
+        .entries = { [0] = { .index = MSR_IA32_TSC } }
+    };
+     env->tsc_valid = !runstate_is_running();

     ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
     if (ret < 0) {


This gives the compiler maximum opportunities to flag mistakes like
initializing the same thing twice, and make it easier (read no smart
optimizations) to initialize in one go. Moving the declaration past the
'if' also addresses Philippe's concern.

>>
>>>      msr_data.info.nmsrs = 1;
>>>      msr_data.entries[0].index = MSR_IA32_TSC;
>>>      env->tsc_valid = !runstate_is_running();
>>> @@ -1706,6 +1707,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>
>>>      if (has_xsave) {
>>>          env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
>>> +        memset(env->xsave_buf, 0, sizeof(struct kvm_xsave));
>>
>> OK
>>
>>>      }
>>>
>>>      max_nested_state_len = kvm_max_nested_state_length();
>>> @@ -3477,6 +3479,7 @@ static int kvm_put_debugregs(X86CPU *cpu)
>>>          return 0;
>>>      }
>>>
>>> +    memset(&dbgregs, 0, sizeof(dbgregs));
>>
>> OK
>>
>>>      for (i = 0; i < 4; i++) {
>>>          dbgregs.db[i] = env->dr[i];
>>>      }
>>
>> We could remove 'dbgregs.flags = 0;'
>>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>


--
Cheers,
Christophe de Dinechin (IRC c3d)