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

[Matheus K. Ferst]

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?

>           /* 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>