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

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

[Daniel Henrique Barboza]

Changes from v1:
- patch 1: init 'val' union in a single statement
- all patches:
  * added Philippe's R-b
  * changed initialization format from {0} to { }
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg07234.html

Daniel Henrique Barboza (4):
  target/ppc: initialize 'val' union 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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.35.1

[PATCH v2 1/4] target/ppc: initialize 'val' union 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 '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 'val' has no impact in the logic and makes Valgrind output
more bearable.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/kvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2649 bytes --]

On Wed, Mar 30, 2022 at 09:17:14PM -0300, 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 '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 'val' has no impact in the logic and makes Valgrind output
> more bearable.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/kvm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index dc93b99189..858866ecd4 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -543,10 +543,11 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> +    /* Init 'val' to avoid "uninitialised value" Valgrind warnings */
>      union {
>          uint32_t u32;
>          uint64_t u64;
> -    } val;
> +    } val = { };
>      struct kvm_one_reg reg = {
>          .id = id,
>          .addr = (uintptr_t) &val,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 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.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 858866ecd4..42814e1b97 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2538,7 +2538,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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On Wed, Mar 30, 2022 at 09:17:15PM -0300, Daniel Henrique Barboza wrote:
> '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.

Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
one of the valgrind hinting mechanisms to inform it that
kvm_get_one_reg() writes the variable at *target?

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 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 858866ecd4..42814e1b97 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2538,7 +2538,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? */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Henrique Barboza]

On 3/30/22 22:25, David Gibson wrote:
> On Wed, Mar 30, 2022 at 09:17:15PM -0300, Daniel Henrique Barboza wrote:
>> '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.
> 
> Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
> one of the valgrind hinting mechanisms to inform it that
> kvm_get_one_reg() writes the variable at *target?

I didn't find a way of doing that looking in the memcheck helpers
(https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
good way of solving this warning because we would put stuff inside a specific
function X and all callers of X would be covered by it.

What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
tells Valgrind that the var was initialized.

This patch would then be something as follows:


diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index dc93b99189..b0e22fa283 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -56,6 +56,10 @@
  #define DEBUG_RETURN_GUEST 0
  #define DEBUG_RETURN_GDB   1
  
+#ifdef CONFIG_VALGRIND_H
+#include <valgrind/memcheck.h>
+#endif
+
  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
      KVM_CAP_LAST_INFO
  };
@@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
      CPUState *cs = CPU(cpu);
      uint64_t lpcr;
  
+#ifdef CONFIG_VALGRIND_H
+    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
+#endif
+
      kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
      /* Do we need to modify the LPCR? */


CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.

I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
situation that will keep happening in the future if we keep adding this same code pattern
(passing as reference an uninitialized var). For now, given that we have only 4 instances
to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
Valgrind that we know what we're doing, I think we're better of initializing these vars.


Thanks,


Daniel



> 
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> 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 858866ecd4..42814e1b97 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2538,7 +2538,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? */
> 

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

[Richard Henderson]

On 3/31/22 11:17, Daniel Henrique Barboza wrote:
>> Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
>> one of the valgrind hinting mechanisms to inform it that
>> kvm_get_one_reg() writes the variable at *target?
> 
> I didn't find a way of doing that looking in the memcheck helpers
> (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
> good way of solving this warning because we would put stuff inside a specific
> function X and all callers of X would be covered by it.
> 
> What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
> tells Valgrind that the var was initialized.
> 
> This patch would then be something as follows:
> 
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index dc93b99189..b0e22fa283 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -56,6 +56,10 @@
>   #define DEBUG_RETURN_GUEST 0
>   #define DEBUG_RETURN_GDB   1
> 
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>       KVM_CAP_LAST_INFO
>   };
> @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
>       CPUState *cs = CPU(cpu);
>       uint64_t lpcr;
> 
> +#ifdef CONFIG_VALGRIND_H
> +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
> +#endif
> +
>       kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
>       /* Do we need to modify the LPCR? */
> 
> 
> CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.
> 
> I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
> situation that will keep happening in the future if we keep adding this same code pattern
> (passing as reference an uninitialized var). For now, given that we have only 4 instances
> to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
> Valgrind that we know what we're doing, I think we're better of initializing these vars.

I would instead put this annotation inside kvm_get_one_reg, so that it covers all kvm 
hosts.  But it's too late to do this for 7.0.


r~

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

[Daniel Henrique Barboza]

On 3/31/22 14:36, Richard Henderson wrote:
> On 3/31/22 11:17, Daniel Henrique Barboza wrote:
>>> Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
>>> one of the valgrind hinting mechanisms to inform it that
>>> kvm_get_one_reg() writes the variable at *target?
>>
>> I didn't find a way of doing that looking in the memcheck helpers
>> (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
>> good way of solving this warning because we would put stuff inside a specific
>> function X and all callers of X would be covered by it.
>>
>> What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
>> tells Valgrind that the var was initialized.
>>
>> This patch would then be something as follows:
>>
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index dc93b99189..b0e22fa283 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -56,6 +56,10 @@
>>   #define DEBUG_RETURN_GUEST 0
>>   #define DEBUG_RETURN_GDB   1
>>
>> +#ifdef CONFIG_VALGRIND_H
>> +#include <valgrind/memcheck.h>
>> +#endif
>> +
>>   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>       KVM_CAP_LAST_INFO
>>   };
>> @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
>>       CPUState *cs = CPU(cpu);
>>       uint64_t lpcr;
>>
>> +#ifdef CONFIG_VALGRIND_H
>> +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
>> +#endif
>> +
>>       kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
>>       /* Do we need to modify the LPCR? */
>>
>>
>> CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.
>>
>> I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
>> situation that will keep happening in the future if we keep adding this same code pattern
>> (passing as reference an uninitialized var). For now, given that we have only 4 instances
>> to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
>> Valgrind that we know what we're doing, I think we're better of initializing these vars.
> 
> I would instead put this annotation inside kvm_get_one_reg, so that it covers all kvm hosts.  But it's too late to do this for 7.0.

I wasn't planning on pushing these changes for 7.0 since they aren't fixing mem
leaks or anything really bad. It's more of a quality of life improvement when
using Valgrind.

I also tried to put this annotation in kvm_get_one_reg() and it didn't solve the
warning. I didn't find a way of telling Valgrind "consider that every time this
function is called with parameter X it initializes X". That would be a good solution
to put in the common KVM files and fix the problem for everybody.


Daniel



> 
> 
> r~

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

On Thu, Mar 31, 2022 at 03:46:57PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 3/31/22 14:36, Richard Henderson wrote:
> > On 3/31/22 11:17, Daniel Henrique Barboza wrote:
> > > > Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
> > > > one of the valgrind hinting mechanisms to inform it that
> > > > kvm_get_one_reg() writes the variable at *target?
> > > 
> > > I didn't find a way of doing that looking in the memcheck helpers
> > > (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
> > > good way of solving this warning because we would put stuff inside a specific
> > > function X and all callers of X would be covered by it.
> > > 
> > > What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
> > > tells Valgrind that the var was initialized.
> > > 
> > > This patch would then be something as follows:
> > > 
> > > 
> > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > > index dc93b99189..b0e22fa283 100644
> > > --- a/target/ppc/kvm.c
> > > +++ b/target/ppc/kvm.c
> > > @@ -56,6 +56,10 @@
> > >   #define DEBUG_RETURN_GUEST 0
> > >   #define DEBUG_RETURN_GDB   1
> > > 
> > > +#ifdef CONFIG_VALGRIND_H
> > > +#include <valgrind/memcheck.h>
> > > +#endif
> > > +
> > >   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> > >       KVM_CAP_LAST_INFO
> > >   };
> > > @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
> > >       CPUState *cs = CPU(cpu);
> > >       uint64_t lpcr;
> > > 
> > > +#ifdef CONFIG_VALGRIND_H
> > > +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
> > > +#endif
> > > +
> > >       kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
> > >       /* Do we need to modify the LPCR? */
> > > 
> > > 
> > > CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.
> > > 
> > > I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
> > > situation that will keep happening in the future if we keep adding this same code pattern
> > > (passing as reference an uninitialized var). For now, given that we have only 4 instances
> > > to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
> > > Valgrind that we know what we're doing, I think we're better of initializing these vars.
> > 
> > I would instead put this annotation inside kvm_get_one_reg, so that it covers all kvm hosts.  But it's too late to do this for 7.0.
> 
> I wasn't planning on pushing these changes for 7.0 since they aren't fixing mem
> leaks or anything really bad. It's more of a quality of life improvement when
> using Valgrind.
> 
> I also tried to put this annotation in kvm_get_one_reg() and it didn't solve the
> warning.

That's weird, I'm pretty sure that should work.  I'd double check to
make sure you had all the parameters right (e.g. could you have marked
the pointer itself as initialized, rather than the memory it points
to).

> I didn't find a way of telling Valgrind "consider that every time this
> function is called with parameter X it initializes X". That would be a good solution
> to put in the common KVM files and fix the problem for everybody.
> 
> 
> Daniel
> 
> 
> 
> > 
> > 
> > r~
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Henrique Barboza]

On 4/1/22 00:40, David Gibson wrote:
> On Thu, Mar 31, 2022 at 03:46:57PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 3/31/22 14:36, Richard Henderson wrote:
>>> On 3/31/22 11:17, Daniel Henrique Barboza wrote:
>>>>> Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
>>>>> one of the valgrind hinting mechanisms to inform it that
>>>>> kvm_get_one_reg() writes the variable at *target?
>>>>
>>>> I didn't find a way of doing that looking in the memcheck helpers
>>>> (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
>>>> good way of solving this warning because we would put stuff inside a specific
>>>> function X and all callers of X would be covered by it.
>>>>
>>>> What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
>>>> tells Valgrind that the var was initialized.
>>>>
>>>> This patch would then be something as follows:
>>>>
>>>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index dc93b99189..b0e22fa283 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -56,6 +56,10 @@
>>>>    #define DEBUG_RETURN_GUEST 0
>>>>    #define DEBUG_RETURN_GDB   1
>>>>
>>>> +#ifdef CONFIG_VALGRIND_H
>>>> +#include <valgrind/memcheck.h>
>>>> +#endif
>>>> +
>>>>    const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>>>        KVM_CAP_LAST_INFO
>>>>    };
>>>> @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
>>>>        CPUState *cs = CPU(cpu);
>>>>        uint64_t lpcr;
>>>>
>>>> +#ifdef CONFIG_VALGRIND_H
>>>> +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
>>>> +#endif
>>>> +
>>>>        kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
>>>>        /* Do we need to modify the LPCR? */
>>>>
>>>>
>>>> CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.
>>>>
>>>> I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
>>>> situation that will keep happening in the future if we keep adding this same code pattern
>>>> (passing as reference an uninitialized var). For now, given that we have only 4 instances
>>>> to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
>>>> Valgrind that we know what we're doing, I think we're better of initializing these vars.
>>>
>>> I would instead put this annotation inside kvm_get_one_reg, so that it covers all kvm hosts.  But it's too late to do this for 7.0.
>>
>> I wasn't planning on pushing these changes for 7.0 since they aren't fixing mem
>> leaks or anything really bad. It's more of a quality of life improvement when
>> using Valgrind.
>>
>> I also tried to put this annotation in kvm_get_one_reg() and it didn't solve the
>> warning.
> 
> That's weird, I'm pretty sure that should work.  I'd double check to
> make sure you had all the parameters right (e.g. could you have marked
> the pointer itself as initialized, rather than the memory it points
> to).


You're right. I got confused with different setups here and there and thought that
it didn't work.

I sent a patch to kvm-all.c that tries to do that:


https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00507.html


As for this series, for now I'm willing to take it since it improves the situation with
simple initializations. We can reconsider it if we make good progress through the common
code. At any rate these are 7.1 patches, so we have time.



Thanks,


Daniel



> 
>> I didn't find a way of telling Valgrind "consider that every time this
>> function is called with parameter X it initializes X". That would be a good solution
>> to put in the common KVM files and fix the problem for everybody.
>>
>>
>> Daniel
>>
>>
>>
>>>
>>>
>>> r~
>>
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5093 bytes --]

On Thu, Mar 31, 2022 at 02:17:42PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 3/30/22 22:25, David Gibson wrote:
> > On Wed, Mar 30, 2022 at 09:17:15PM -0300, Daniel Henrique Barboza wrote:
> > > '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.
> > 
> > Hmm... this is seeming a bit like whack-a-mole.  Could we instead use
> > one of the valgrind hinting mechanisms to inform it that
> > kvm_get_one_reg() writes the variable at *target?
> 
> I didn't find a way of doing that looking in the memcheck helpers
> (https://valgrind.org/docs/manual/mc-manual.html section 4.7). That would be a
> good way of solving this warning because we would put stuff inside a specific
> function X and all callers of X would be covered by it.
> 
> What I did find instead is a memcheck macro called VALGRIND_MAKE_MEM_DEFINED that
> tells Valgrind that the var was initialized.

I think that's the one I was thinking of.

> This patch would then be something as follows:
> 
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index dc93b99189..b0e22fa283 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -56,6 +56,10 @@
>  #define DEBUG_RETURN_GUEST 0
>  #define DEBUG_RETURN_GDB   1
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>      KVM_CAP_LAST_INFO
>  };
> @@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
>      CPUState *cs = CPU(cpu);
>      uint64_t lpcr;
> +#ifdef CONFIG_VALGRIND_H
> +    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
> +#endif
> +
>      kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
>      /* Do we need to modify the LPCR? */

The macro call should only go after the get_one_reg, of course.

> CONFIG_VALGRIND_H needs 'valgrind-devel´ installed.

Right.. better would probably be to make a wrapper macro defined as a
no-op in the !CONFIG_VALGRIND_H case, so you don't need the ifdefs at
the point you use it.
> 
> I agree that this "Valgrind is complaining about variable initialization" is a whack-a-mole
> situation that will keep happening in the future if we keep adding this same code pattern
> (passing as reference an uninitialized var). For now, given that we have only 4 instances
> to fix it in ppc code (as far as I'm aware of), and we don't have a better way of telling
> Valgrind that we know what we're doing, I think we're better of
> initializing these vars.

Hmm... still feels like it would be better to put the
MAKE_MEM_DEFINED inside kvm_get_one_reg().  I think the difficulty
with that is that it handles both 32-bit and 64-bit registers and I'm
not sure if there's an easy way to work out exactly how many bits
*have* been initialized.

> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> > 
> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > 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 858866ecd4..42814e1b97 100644
> > > --- a/target/ppc/kvm.c
> > > +++ b/target/ppc/kvm.c
> > > @@ -2538,7 +2538,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? */
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 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.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 42814e1b97..9a2ee761d2 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -851,7 +851,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 = { };
     int i;
 
     sregs.pvr = env->spr[SPR_PVR];
-- 
2.35.1

[PATCH v2 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)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 9a2ee761d2..a3130013b3 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 = { };
     int i;
 
     if (!kvm_check_extension(s, KVM_CAP_PPC_MMU_RADIX)) {
-- 
2.35.1