[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/4] target/ppc: init 'lpcr' in kvmppc_enable_cap_large_de

From: Daniel Henrique Barboza
Subject: Re: [PATCH v2 2/4] target/ppc: init 'lpcr' in kvmppc_enable_cap_large_decr()
Date: Tue, 5 Apr 2022 16:37:11 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0

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 
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_GDB   1

+#include <valgrind/memcheck.h>
   const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
@@ -2539,6 +2543,10 @@ int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int 
       CPUState *cs = CPU(cpu);
       uint64_t lpcr;

+    VALGRIND_MAKE_MEM_DEFINED(lpcr, sizeof(uint64_t));
       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 
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 
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

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

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

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


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.



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 
to put in the common KVM files and fix the problem for everybody.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]