Re: [PATCH v2 2/2] ppc/pnv: use a do-while() loop in pnv_phb4_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:
>> pnv_phb4_translate_tve() is quite similar to pnv_phb3_translate_tve(),
>> and that includes the fact that 'taddr' can be considered uninitialized
>> when throwing the "TCE access fault" error because, in theory, the loop
>> that sets 'taddr' can be skippable due to 'lev' being an signed int.
>>
>> No one complained about this specific case yet, but since we took the
>> time to handle the same situtation in pnv_phb3_translate_tve(), let's
>> replicate it here as well.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index a78add75b0..88a1479831 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1261,13 +1261,21 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
>>           /* Top level table base address */
>>           base = tta << 12;
>>
>> +        /*
>> +         * There were reports of compilers complaining about 'taddr'
>> +         * being used uninitialized in pnv_phb3_translate_tve(), and
>> +         * the same scenario is happening here. Initialize 'taddr'
>> +         * just in case.
>> +         */
>> +        taddr = base;
>> +
> 
> Do we still need this initialization?

Nah, that was a spill from the previous patch that I forgot to remove.

Thanks,

Daniel

> 
>>           /* Total shift to first level */
>>           sh = tbl_shift * lev + tce_shift;
>>
>>           /* TODO: Limit to support IO page sizes */
>>
>>           /* 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,
>> @@ -1288,7 +1296,7 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
>>               }
>>               sh -= tbl_shift;
>>               base = tce & ~0xfffull;
>> -        }
>> +        } while ((lev--) >= 0);
> 
> The same comments from the other patch apply here, this changes the number of iterations in this loop.
> 
> 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>