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

[Daniel Henrique Barboza]

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>