Re: [PATCH v2 1/2] ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve()

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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.ferst@eldorado.org.br>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@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?


I don't think that's a problem because then (lev >= 0) will not be true and we'll
not going to check !(tce &3), so even if 'tce' has a bogus value it's fine.


Daniel






> 
> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>