On Thu, 27 Jan 2022, Daniel Henrique Barboza wrote:
On 1/27/22 08:41, Matheus K. Ferst wrote:
On 26/01/2022 17:14, Daniel Henrique Barboza wrote:
The 'taddr' variable is left unintialized, being set only inside the
"while ((lev--) >= 0)" loop where we get the TCE address. The 'lev' var
is an int32_t that is being initiliazed by the GETFIELD() macro, which
returns an uint64_t.
For a human reader this means that 'lev' will always be positive or zero.
But some compilers may beg to differ. 'lev' being an int32_t can in theory
be set as negative, and the "while ((lev--) >= 0)" loop might never be
reached, and 'taddr' will be left unitialized. This can cause phb3_error()
to use 'taddr' uninitialized down below:
if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) {
phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr);
A quick way of fixing it is to use a do/while() loop. This will keep the
same semanting as the existing while() loop does and the compiler will
understand that 'taddr' will be initialized at least once.
Suggested-by: Matheus K. Ferst <matheus.fe...@eldorado.org.br>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573
Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
---
hw/pci-host/pnv_phb3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 7fb35dc031..39a6184419 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -792,7 +792,7 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace
*ds, hwaddr addr,
sh = tbl_shift * lev + tce_shift;
/* TODO: Multi-level untested */
- while ((lev--) >= 0) {
+ do {
/* Grab the TCE address */
taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) <<
3);
if (dma_memory_read(&address_space_memory, taddr, &tce,
@@ -813,7 +813,7 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace
*ds, hwaddr addr,
}
sh -= tbl_shift;
base = tce & ~0xfffull;
- }
+ } while ((lev--) >= 0);
This changes the number of iterations in this loop.
ooofff
We'd need "while ((--lev) >= 0)" to keep it the same, but then we would be
checking "!(tce & 3)" for the last iteration. Is that a problem?
When you're at it, you could also drop the extra () around lev--. I don't
think we need that and does not add to readability IMO.
I don't really know what's the original problem but if it's a while with
missing init then often what you want is for (init, while clause, inc)
instead but not sure that's applicable in this case so just disregard
this comment if not relevant.
Regards,
BALATON Zoltan
