[PATCH 0/4] ppc: valgrind "uninitialized values" fixes

[PATCH 0/4] ppc: valgrind "uninitialized values" fixes

[Daniel Henrique Barboza]

Hi,

These are a handful of trivial fixes to make Valgrind happier when
profiling the boot of a pSeries guest. All the patches are handling a
similar case where we have something similar to this:

---
uint64_t val;

(...)

kvm_vcpu_ioctl(...., &val);
---

Valgrind does not consider that 'val' was initialized and then it keeps
complaining about every future use of 'val', or anything that got
assigned as 'val', as being an uninitialized value/data. The fix is
simple and without any side effects:: just initialize 'val'.

After this series, together with the memory leak fix sent in [1], we
don't see any more ppc/spapr related Valgrind memcheck complaints when
booting a pSeries guest.

No functional changes were made.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg06804.html

Daniel Henrique Barboza (4):
  target/ppc: initialize 'reg_val' in kvm_get_one_spr()
  target/ppc: init 'lpcr' in kvmppc_enable_cap_large_decr()
  target/ppc: init 'sregs' in kvmppc_put_books_sregs()
  target/ppc: init 'rmmu_info' in kvm_get_radix_page_info()

 target/ppc/kvm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.35.1

[PATCH 1/4] target/ppc: initialize 'reg_val' in kvm_get_one_spr()

[Daniel Henrique Barboza]

Valgrind isn't convinced that we are initializing the values we assign
to env->spr[spr] because it doesn't understand that the 'reg_val' union
is being written by the kvm_vcpu_ioctl() that follows (via struct
kvm_one_reg).

This results in Valgrind complaining about uninitialized values every
time we use env->spr in a conditional, like this instance:

==707578== Thread 1:
==707578== Conditional jump or move depends on uninitialised value(s)
==707578==    at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106)
==707578==    by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173)
==707578==    by 0xA110F7: hreg_store_msr (helper_regs.c:262)
==707578==    by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168)
==707578==    by 0xD4730F: device_transitional_reset (qdev.c:799)
==707578==    by 0xD4A11B: resettable_phase_hold (resettable.c:182)
==707578==    by 0xD49A77: resettable_assert_reset (resettable.c:60)
==707578==    by 0xD4994B: resettable_reset (resettable.c:45)
==707578==    by 0xD458BB: device_cold_reset (qdev.c:296)
==707578==    by 0x48FBC7: cpu_reset (cpu-common.c:114)
==707578==    by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38)
==707578==    by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209)
==707578==  Uninitialised value was created by a stack allocation
==707578==    at 0xB11F08: kvm_get_one_spr (kvm.c:543)

Initializing 'reg_val' has no impact in the logic and makes Valgrind
output more bearable.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index dc93b99189..ce1b926e8c 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
-    union {
+    union reg_val {
         uint32_t u32;
         uint64_t u64;
-    } val;
+    };
+    /* Init reg_val to avoid "uninitialised value" Valgrind warnings */
+    union reg_val val = {0};
     struct kvm_one_reg reg = {
         .id = id,
         .addr = (uintptr_t) &val,
-- 
2.35.1

Re: [PATCH 1/4] target/ppc: initialize 'reg_val' in kvm_get_one_spr()

[Philippe Mathieu-Daudé]

On 30/3/22 23:04, Daniel Henrique Barboza wrote:
> Valgrind isn't convinced that we are initializing the values we assign
> to env->spr[spr] because it doesn't understand that the 'reg_val' union
> is being written by the kvm_vcpu_ioctl() that follows (via struct
> kvm_one_reg).
> 
> This results in Valgrind complaining about uninitialized values every
> time we use env->spr in a conditional, like this instance:
> 
> ==707578== Thread 1:
> ==707578== Conditional jump or move depends on uninitialised value(s)
> ==707578==    at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106)
> ==707578==    by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173)
> ==707578==    by 0xA110F7: hreg_store_msr (helper_regs.c:262)
> ==707578==    by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168)
> ==707578==    by 0xD4730F: device_transitional_reset (qdev.c:799)
> ==707578==    by 0xD4A11B: resettable_phase_hold (resettable.c:182)
> ==707578==    by 0xD49A77: resettable_assert_reset (resettable.c:60)
> ==707578==    by 0xD4994B: resettable_reset (resettable.c:45)
> ==707578==    by 0xD458BB: device_cold_reset (qdev.c:296)
> ==707578==    by 0x48FBC7: cpu_reset (cpu-common.c:114)
> ==707578==    by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38)
> ==707578==    by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209)
> ==707578==  Uninitialised value was created by a stack allocation
> ==707578==    at 0xB11F08: kvm_get_one_spr (kvm.c:543)
> 
> Initializing 'reg_val' has no impact in the logic and makes Valgrind
> output more bearable.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/kvm.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index dc93b99189..ce1b926e8c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>       CPUPPCState *env = &cpu->env;
> -    union {
> +    union reg_val {
>           uint32_t u32;
>           uint64_t u64;
> -    } val;
> +    };
> +    /* Init reg_val to avoid "uninitialised value" Valgrind warnings */
> +    union reg_val val = {0};

This should also work:

-- >8 --
@@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t 
id, int spr)
      union {
          uint32_t u32;
          uint64_t u64;
-    } val;
+    } val = { 0 };
---

Re: [PATCH 1/4] target/ppc: initialize 'reg_val' in kvm_get_one_spr()

[Daniel Henrique Barboza]

On 3/30/22 18:22, Philippe Mathieu-Daudé wrote:
> On 30/3/22 23:04, Daniel Henrique Barboza wrote:
>> Valgrind isn't convinced that we are initializing the values we assign
>> to env->spr[spr] because it doesn't understand that the 'reg_val' union
>> is being written by the kvm_vcpu_ioctl() that follows (via struct
>> kvm_one_reg).
>>
>> This results in Valgrind complaining about uninitialized values every
>> time we use env->spr in a conditional, like this instance:
>>
>> ==707578== Thread 1:
>> ==707578== Conditional jump or move depends on uninitialised value(s)
>> ==707578==    at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106)
>> ==707578==    by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173)
>> ==707578==    by 0xA110F7: hreg_store_msr (helper_regs.c:262)
>> ==707578==    by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168)
>> ==707578==    by 0xD4730F: device_transitional_reset (qdev.c:799)
>> ==707578==    by 0xD4A11B: resettable_phase_hold (resettable.c:182)
>> ==707578==    by 0xD49A77: resettable_assert_reset (resettable.c:60)
>> ==707578==    by 0xD4994B: resettable_reset (resettable.c:45)
>> ==707578==    by 0xD458BB: device_cold_reset (qdev.c:296)
>> ==707578==    by 0x48FBC7: cpu_reset (cpu-common.c:114)
>> ==707578==    by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38)
>> ==707578==    by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209)
>> ==707578==  Uninitialised value was created by a stack allocation
>> ==707578==    at 0xB11F08: kvm_get_one_spr (kvm.c:543)
>>
>> Initializing 'reg_val' has no impact in the logic and makes Valgrind
>> output more bearable.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/kvm.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index dc93b99189..ce1b926e8c 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
>>   {
>>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>>       CPUPPCState *env = &cpu->env;
>> -    union {
>> +    union reg_val {
>>           uint32_t u32;
>>           uint64_t u64;
>> -    } val;
>> +    };
>> +    /* Init reg_val to avoid "uninitialised value" Valgrind warnings */
>> +    union reg_val val = {0};
> 
> This should also work:
> 
> -- >8 --
> @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
>       union {
>           uint32_t u32;
>           uint64_t u64;
> -    } val;
> +    } val = { 0 };

Apparently it does work. I'll make a few tests and re-send.


Also, do we have an official way of spelling this zeroed struct initialization? I
see several instances of {0} and { 0 } in the code. In this series I used {0}.
./scripts/checkpatch.pl seems ok with both formats.


Thanks,


Daniel


> ---

Re: [PATCH 1/4] target/ppc: initialize 'reg_val' in kvm_get_one_spr()

[Philippe Mathieu-Daudé]

On 30/3/22 23:34, Daniel Henrique Barboza wrote:
> On 3/30/22 18:22, Philippe Mathieu-Daudé wrote:
>> On 30/3/22 23:04, Daniel Henrique Barboza wrote:
>>> Valgrind isn't convinced that we are initializing the values we assign
>>> to env->spr[spr] because it doesn't understand that the 'reg_val' union
>>> is being written by the kvm_vcpu_ioctl() that follows (via struct
>>> kvm_one_reg).
>>>
>>> This results in Valgrind complaining about uninitialized values every
>>> time we use env->spr in a conditional, like this instance:
>>>
>>> ==707578== Thread 1:
>>> ==707578== Conditional jump or move depends on uninitialised value(s)
>>> ==707578==    at 0xA10A40: hreg_compute_hflags_value (helper_regs.c:106)
>>> ==707578==    by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173)
>>> ==707578==    by 0xA110F7: hreg_store_msr (helper_regs.c:262)
>>> ==707578==    by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168)
>>> ==707578==    by 0xD4730F: device_transitional_reset (qdev.c:799)
>>> ==707578==    by 0xD4A11B: resettable_phase_hold (resettable.c:182)
>>> ==707578==    by 0xD49A77: resettable_assert_reset (resettable.c:60)
>>> ==707578==    by 0xD4994B: resettable_reset (resettable.c:45)
>>> ==707578==    by 0xD458BB: device_cold_reset (qdev.c:296)
>>> ==707578==    by 0x48FBC7: cpu_reset (cpu-common.c:114)
>>> ==707578==    by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38)
>>> ==707578==    by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209)
>>> ==707578==  Uninitialised value was created by a stack allocation
>>> ==707578==    at 0xB11F08: kvm_get_one_spr (kvm.c:543)
>>>
>>> Initializing 'reg_val' has no impact in the logic and makes Valgrind
>>> output more bearable.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   target/ppc/kvm.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index dc93b99189..ce1b926e8c 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, 
>>> uint64_t id, int spr)
>>>   {
>>>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>       CPUPPCState *env = &cpu->env;
>>> -    union {
>>> +    union reg_val {
>>>           uint32_t u32;
>>>           uint64_t u64;
>>> -    } val;
>>> +    };
>>> +    /* Init reg_val to avoid "uninitialised value" Valgrind warnings */
>>> +    union reg_val val = {0};
>>
>> This should also work:
>>
>> -- >8 --
>> @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t 
>> id, int spr)
>>       union {
>>           uint32_t u32;
>>           uint64_t u64;
>> -    } val;
>> +    } val = { 0 };
> 
> Apparently it does work. I'll make a few tests and re-send.
> 
> 
> Also, do we have an official way of spelling this zeroed struct 
> initialization? I
> see several instances of {0} and { 0 } in the code. In this series I 
> used {0}.
> ./scripts/checkpatch.pl seems ok with both formats.

$ git grep -F '= {0}' | wc -l
       81
$ git grep -F '= { 0 }' | wc -l
      112
$ git grep -F '= { }' | wc -l
       61
$ git grep -F '= {}' | wc -l
      368

I am not aware of an official way. Apparently '{ }' is the way to go.

Re: [PATCH 1/4] target/ppc: initialize 'reg_val' in kvm_get_one_spr()

[Philippe Mathieu-Daudé]

On 30/3/22 23:38, Philippe Mathieu-Daudé wrote:
> On 30/3/22 23:34, Daniel Henrique Barboza wrote:
>> On 3/30/22 18:22, Philippe Mathieu-Daudé wrote:
>>> On 30/3/22 23:04, Daniel Henrique Barboza wrote:
>>>> Valgrind isn't convinced that we are initializing the values we assign
>>>> to env->spr[spr] because it doesn't understand that the 'reg_val' union
>>>> is being written by the kvm_vcpu_ioctl() that follows (via struct
>>>> kvm_one_reg).
>>>>
>>>> This results in Valgrind complaining about uninitialized values every
>>>> time we use env->spr in a conditional, like this instance:
>>>>
>>>> ==707578== Thread 1:
>>>> ==707578== Conditional jump or move depends on uninitialised value(s)
>>>> ==707578==    at 0xA10A40: hreg_compute_hflags_value 
>>>> (helper_regs.c:106)
>>>> ==707578==    by 0xA10C9F: hreg_compute_hflags (helper_regs.c:173)
>>>> ==707578==    by 0xA110F7: hreg_store_msr (helper_regs.c:262)
>>>> ==707578==    by 0xA051A3: ppc_cpu_reset (cpu_init.c:7168)
>>>> ==707578==    by 0xD4730F: device_transitional_reset (qdev.c:799)
>>>> ==707578==    by 0xD4A11B: resettable_phase_hold (resettable.c:182)
>>>> ==707578==    by 0xD49A77: resettable_assert_reset (resettable.c:60)
>>>> ==707578==    by 0xD4994B: resettable_reset (resettable.c:45)
>>>> ==707578==    by 0xD458BB: device_cold_reset (qdev.c:296)
>>>> ==707578==    by 0x48FBC7: cpu_reset (cpu-common.c:114)
>>>> ==707578==    by 0x97B5EB: spapr_reset_vcpu (spapr_cpu_core.c:38)
>>>> ==707578==    by 0x97BABB: spapr_cpu_core_reset (spapr_cpu_core.c:209)
>>>> ==707578==  Uninitialised value was created by a stack allocation
>>>> ==707578==    at 0xB11F08: kvm_get_one_spr (kvm.c:543)
>>>>
>>>> Initializing 'reg_val' has no impact in the logic and makes Valgrind
>>>> output more bearable.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>   target/ppc/kvm.c | 6 ++++--
>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index dc93b99189..ce1b926e8c 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -543,10 +543,12 @@ static void kvm_get_one_spr(CPUState *cs, 
>>>> uint64_t id, int spr)
>>>>   {
>>>>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>       CPUPPCState *env = &cpu->env;
>>>> -    union {
>>>> +    union reg_val {
>>>>           uint32_t u32;
>>>>           uint64_t u64;
>>>> -    } val;
>>>> +    };
>>>> +    /* Init reg_val to avoid "uninitialised value" Valgrind 
>>>> warnings */
>>>> +    union reg_val val = {0};
>>>
>>> This should also work:
>>>
>>> -- >8 --
>>> @@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, 
>>> uint64_t id, int spr)
>>>       union {
>>>           uint32_t u32;
>>>           uint64_t u64;
>>> -    } val;
>>> +    } val = { 0 };
>>
>> Apparently it does work. I'll make a few tests and re-send.
>>
>>
>> Also, do we have an official way of spelling this zeroed struct 
>> initialization? I
>> see several instances of {0} and { 0 } in the code. In this series I 
>> used {0}.
>> ./scripts/checkpatch.pl seems ok with both formats.
> 
> $ git grep -F '= {0}' | wc -l
>        81
> $ git grep -F '= { 0 }' | wc -l
>       112
> $ git grep -F '= { }' | wc -l
>        61
> $ git grep -F '= {}' | wc -l
>       368
> 
> I am not aware of an official way. Apparently '{ }' is the way to go.

I meant '{}' but personally I prefer '{ }' for readability.

[PATCH 2/4] target/ppc: init 'lpcr' in kvmppc_enable_cap_large_decr()

[Daniel Henrique Barboza]

'lpcr' is used as an input of kvm_get_one_reg(). Valgrind doesn't
understand that and it returns warnings as such for this function:

==55240== Thread 1:
==55240== Conditional jump or move depends on uninitialised value(s)
==55240==    at 0xB011E4: kvmppc_enable_cap_large_decr (kvm.c:2546)
==55240==    by 0x92F28F: cap_large_decr_cpu_apply (spapr_caps.c:523)
==55240==    by 0x930C37: spapr_caps_cpu_apply (spapr_caps.c:921)
==55240==    by 0x955D3B: spapr_reset_vcpu (spapr_cpu_core.c:73)
==55240==    by 0x95612B: spapr_cpu_core_reset (spapr_cpu_core.c:209)
==55240==    by 0x95619B: spapr_cpu_core_reset_handler (spapr_cpu_core.c:218)
==55240==    by 0xD3605F: qemu_devices_reset (reset.c:69)
==55240==    by 0x92112B: spapr_machine_reset (spapr.c:1641)
==55240==    by 0x4FBD63: qemu_system_reset (runstate.c:444)
==55240==    by 0x62812B: qdev_machine_creation_done (machine.c:1247)
==55240==    by 0x5064C3: qemu_machine_creation_done (vl.c:2725)
==55240==    by 0x5065DF: qmp_x_exit_preconfig (vl.c:2748)
==55240==  Uninitialised value was created by a stack allocation
==55240==    at 0xB01158: kvmppc_enable_cap_large_decr (kvm.c:2540)

Init 'lpcr' to avoid this warning.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index ce1b926e8c..9fb13b23d8 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2539,7 +2539,7 @@ int kvmppc_get_cap_large_decr(void)
 int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
 {
     CPUState *cs = CPU(cpu);
-    uint64_t lpcr;
+    uint64_t lpcr = 0;
 
     kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
     /* Do we need to modify the LPCR? */
-- 
2.35.1

[PATCH 3/4] target/ppc: init 'sregs' in kvmppc_put_books_sregs()

[Daniel Henrique Barboza]

Init 'sregs' to avoid Valgrind complaints about uninitialized bytes
from kvmppc_put_books_sregs():

==54059== Thread 3:
==54059== Syscall param ioctl(generic) points to uninitialised byte(s)
==54059==    at 0x55864E4: ioctl (in /usr/lib64/libc.so.6)
==54059==    by 0xD1FA23: kvm_vcpu_ioctl (kvm-all.c:3053)
==54059==    by 0xAFB18B: kvmppc_put_books_sregs (kvm.c:891)
==54059==    by 0xAFB47B: kvm_arch_put_registers (kvm.c:949)
==54059==    by 0xD1EDA7: do_kvm_cpu_synchronize_post_init (kvm-all.c:2766)
==54059==    by 0x481AF3: process_queued_cpu_work (cpus-common.c:343)
==54059==    by 0x4EF247: qemu_wait_io_event_common (cpus.c:412)
==54059==    by 0x4EF343: qemu_wait_io_event (cpus.c:436)
==54059==    by 0xD21E83: kvm_vcpu_thread_fn (kvm-accel-ops.c:54)
==54059==    by 0xFFEBF3: qemu_thread_start (qemu-thread-posix.c:556)
==54059==    by 0x54E6DC3: start_thread (in /usr/lib64/libc.so.6)
==54059==    by 0x5596C9F: clone (in /usr/lib64/libc.so.6)
==54059==  Address 0x799d1cc is on thread 3's stack
==54059==  in frame #2, created by kvmppc_put_books_sregs (kvm.c:851)
==54059==  Uninitialised value was created by a stack allocation
==54059==    at 0xAFAEB0: kvmppc_put_books_sregs (kvm.c:851)

This happens because Valgrind does not consider the 'sregs'
initialization done by kvm_vcpu_ioctl() at the end of the function.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 9fb13b23d8..657e735f9d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -852,7 +852,7 @@ static int kvm_put_vpa(CPUState *cs)
 int kvmppc_put_books_sregs(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
-    struct kvm_sregs sregs;
+    struct kvm_sregs sregs = {0};
     int i;
 
     sregs.pvr = env->spr[SPR_PVR];
-- 
2.35.1

[PATCH 4/4] target/ppc: init 'rmmu_info' in kvm_get_radix_page_info()

[Daniel Henrique Barboza]

Init the struct to avoid Valgrind complaints about unitialized bytes,
such as this one:

==39549== Syscall param ioctl(generic) points to uninitialised byte(s)
==39549==    at 0x55864E4: ioctl (in /usr/lib64/libc.so.6)
==39549==    by 0xD1F7EF: kvm_vm_ioctl (kvm-all.c:3035)
==39549==    by 0xAF8F5B: kvm_get_radix_page_info (kvm.c:276)
==39549==    by 0xB00533: kvmppc_host_cpu_class_init (kvm.c:2369)
==39549==    by 0xD3DCE7: type_initialize (object.c:366)
==39549==    by 0xD3FACF: object_class_foreach_tramp (object.c:1071)
==39549==    by 0x502757B: g_hash_table_foreach (in /usr/lib64/libglib-2.0.so.0.7000.5)
==39549==    by 0xD3FC1B: object_class_foreach (object.c:1093)
==39549==    by 0xB0141F: kvm_ppc_register_host_cpu_type (kvm.c:2613)
==39549==    by 0xAF87E7: kvm_arch_init (kvm.c:157)
==39549==    by 0xD1E2A7: kvm_init (kvm-all.c:2595)
==39549==    by 0x8E6E93: accel_init_machine (accel-softmmu.c:39)
==39549==  Address 0x1fff00e208 is on thread 1's stack
==39549==  in frame #2, created by kvm_get_radix_page_info (kvm.c:267)
==39549==  Uninitialised value was created by a stack allocation
==39549==    at 0xAF8EE8: kvm_get_radix_page_info (kvm.c:267)

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 657e735f9d..f22cf75ade 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -267,7 +267,7 @@ struct ppc_radix_page_info *kvm_get_radix_page_info(void)
 {
     KVMState *s = KVM_STATE(current_accel());
     struct ppc_radix_page_info *radix_page_info;
-    struct kvm_ppc_rmmu_info rmmu_info;
+    struct kvm_ppc_rmmu_info rmmu_info = {0};
     int i;
 
     if (!kvm_check_extension(s, KVM_CAP_PPC_MMU_RADIX)) {
-- 
2.35.1

Re: [PATCH 0/4] ppc: valgrind "uninitialized values" fixes

[Philippe Mathieu-Daudé]

On 30/3/22 23:04, Daniel Henrique Barboza wrote:
> Hi,
> 
> These are a handful of trivial fixes to make Valgrind happier when
> profiling the boot of a pSeries guest. All the patches are handling a
> similar case where we have something similar to this:
> 
> ---
> uint64_t val;
> 
> (...)
> 
> kvm_vcpu_ioctl(...., &val);

Which is a false positive.

> ---
> 
> Valgrind does not consider that 'val' was initialized and then it keeps
> complaining about every future use of 'val', or anything that got
> assigned as 'val', as being an uninitialized value/data. The fix is
> simple and without any side effects:: just initialize 'val'.

I gave a try to some definition but the result is rather ugly =)

-- >8 --
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index d9359859d4..85429930fb 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -194,4 +194,10 @@ extern void QEMU_NORETURN QEMU_ERROR("code path is 
reachable")
  #define QEMU_DISABLE_CFI
  #endif

+#ifdef QEMU_STATIC_ANALYSIS
+#define QEMU_UNNECESSARY_INIT(values...) = values
+#else
+#define QEMU_UNNECESSARY_INIT(values...)
+#endif
+
  #endif /* COMPILER_H */
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index dc93b99189..97aec29694 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -546,7 +546,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t 
id, int spr)
      union {
          uint32_t u32;
          uint64_t u64;
-    } val;
+    } val QEMU_UNNECESSARY_INIT({ });
      struct kvm_one_reg reg = {
          .id = id,
          .addr = (uintptr_t) &val,
---

Being able to use valgrind is more valuable that these unnecessary
init, so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>