| Message ID | 20220126201438.822457-3-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | use a do-while() loop in pnv_phbX_translate_tve() | expand |
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>
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>
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; + /* 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); /* We exit the loop with TCE being the final TCE */ tce_mask = ~((1ull << tce_shift) - 1);
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(-)