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

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 31/07/2019 10:24, Christian Borntraeger wrote:
> 
> 
> On 30.07.19 21:20, Paolo Bonzini wrote:
>> On 30/07/19 18:01, 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 <andrey.shinkevich@virtuozzo.com>
>>
>> Christian, is this the right fix?  It's not expensive so it wouldn't be
>> an issue, just checking if there's any better alternative.
> 
> I think all of these variants are valid with pros and cons
> 1. teach valgrind about this:
> Add to coregrind/m_syswrap/syswrap-linux.c (and the relevant header files)
> knowledge about which parts are actually touched.
> 2. use designated initializers
> 3. use memset
> 3. use a valgrind callback VG_USERREQ__MAKE_MEM_DEFINED to tell that this memory is defined
> 

Thank you all very much for taking part in the discussion.
Also, one may use the Valgrind technology to suppress the unwanted 
reports by adding the Valgrind specific format file valgrind.supp to the 
QEMU project. The file content is extendable for future needs.
All the cases we like to suppress will be recounted in that file.
A case looks like the stack fragments. For instance, from QEMU block:

{
    hw/block/hd-geometry.c
    Memcheck:Cond
    fun:guess_disk_lchs
    fun:hd_geometry_guess
    fun:blkconf_geometry
    ...
    fun:device_set_realized
    fun:property_set_bool
    fun:object_property_set
    fun:object_property_set_qobject
    fun:object_property_set_bool
}

The number of suppressed cases are reported by the Valgrind with every 
run: "ERROR SUMMARY: 5 errors from 3 contexts (suppressed: 0 from 0)"

Andrey

>>
>> Paolo
>>
>>> ---
>>>   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));
>>>       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));
>>>       }
>>>   
>>>       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));
>>>       for (i = 0; i < 4; i++) {
>>>           dbgregs.db[i] = env->dr[i];
>>>       }
>>> -- 
>>> 1.8.3.1
>>>
>>
>>
> 

-- 
With the best regards,
Andrey Shinkevich