[PATCH v2 0/2] use a do-while() loop in pnv_phbX_translate_tve()

[PATCH v2 0/2] use a do-while() loop in pnv_phbX_translate_tve()

[Daniel Henrique Barboza]

Hi,

This second version implemented Matheus' suggestion of using a
do-while() loop instead of initializing variables.

v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg05475.html

Daniel Henrique Barboza (2):
  ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve()
  ppc/pnv: use a do-while() loop in pnv_phb4_translate_tve()

 hw/pci-host/pnv_phb3.c |  4 ++--
 hw/pci-host/pnv_phb4.c | 12 ++++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.34.1

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

[Daniel Henrique Barboza]

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);
 
         /* We exit the loop with TCE being the final TCE */
         tce_mask = ~((1ull << tce_shift) - 1);
-- 
2.34.1

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

[Daniel Henrique Barboza]

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;
+
         /* 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);
-- 
2.34.1

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

[Matheus K. Ferst]

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. 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?

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>

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>

Re: [PATCH v2 2/2] ppc/pnv: use a do-while() loop in pnv_phb4_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:
>> 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>

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>

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

[Matheus K. Ferst]

On 27/01/2022 09:09, Daniel Henrique Barboza wrote:
> 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 

That holds for "while ((lev--) >= 0)", where the last iteration has 
"lev=-1", but "while ((--lev) >= 0)" would make lev=0 in the last 
execution of this loop. I guess we need to either decrement lev 
elsewhere or adjust its use inside the loop (e.g.: if (lev > 0 && !(tce 
& 3)))

> and we'll
> not going to check !(tce &3), so even if 'tce' has a bogus value it's fine.
> 

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>

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]

On Thu, 27 Jan 2022, Daniel Henrique Barboza wrote:
> 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?

When you're at it, you could also drop the extra () around lev--. I don't 
think we need that and does not add to readability IMO.

I don't really know what's the original problem but if it's a while with 
missing init then often what you want is for (init, while clause, inc) 
instead but not sure that's applicable in this case so just disregard 
this comment if not relevant.

Regards,
BALATON Zoltan

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

[Daniel Henrique Barboza]

On 1/27/22 09:28, Matheus K. Ferst wrote:
> On 27/01/2022 09:09, Daniel Henrique Barboza wrote:
>> 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 
> 
> That holds for "while ((lev--) >= 0)", where the last iteration has "lev=-1", but "while ((--lev) >= 0)" would make lev=0 in the last execution of this loop. I guess we need to either decrement lev elsewhere or adjust its use inside the loop (e.g.: if (lev > 0 && !(tce & 3)))

Oh, I mentioned that considering that I screwed up and the decrement should
be done at the start of the loop.

Changing the logic to accommodate the lev decrement at the end seems too much
trouble. I'd rather keep the same original logic.


Daniel

> 
>> and we'll
>> not going to check !(tce &3), so even if 'tce' has a bogus value it's fine.
>>
> 
> 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>