[v2] target/ppc: Fix 64-bit decrementer

[PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Cédric Le Goater 2 years, 7 months ago

The current way the mask is built can overflow with a 64-bit decrementer.
Use sextract64() instead.

Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 v2: replaced MAKE_64BIT_MASK by sextract64

 hw/ppc/ppc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 7375bf4fa910..4f14464c9220 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     bool negative;
 
     /* Truncate value to decr_width and sign extend for simplicity */
-    value &= ((1ULL << nr_bits) - 1);
+    value = sextract64(value, 0, nr_bits);
     negative = !!(value & (1ULL << (nr_bits - 1)));
     if (negative) {
         value |= (0xFFFFFFFFULL << nr_bits);
-- 
2.31.1

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Peter Maydell 2 years, 7 months ago

On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
>
> The current way the mask is built can overflow with a 64-bit decrementer.
> Use sextract64() instead.
>
> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
>  v2: replaced MAKE_64BIT_MASK by sextract64
>
>  hw/ppc/ppc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 7375bf4fa910..4f14464c9220 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>      bool negative;
>
>      /* Truncate value to decr_width and sign extend for simplicity */
> -    value &= ((1ULL << nr_bits) - 1);
> +    value = sextract64(value, 0, nr_bits);
>      negative = !!(value & (1ULL << (nr_bits - 1)));
>      if (negative) {
>          value |= (0xFFFFFFFFULL << nr_bits);

I think these lines that say "if negative then force all the
high bits to one" are also no longer required. That is, this
entire section of code:
    value &= ((1ULL << nr_bits) - 1);
    negative = !!(value & (1ULL << (nr_bits - 1)));
    if (negative) {
        value |= (0xFFFFFFFFULL << nr_bits);
    }

is an open-coded sign-extension, which can all be replaced with
the single line
    value = sextract64(value, 0, nr_bits);

(and also: if nr_bits is 64 then the "<< nr_bits"
is undefined behaviour.)

thanks
-- PMM

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Cédric Le Goater 2 years, 7 months ago

On 9/14/21 11:19 AM, Peter Maydell wrote:
> On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The current way the mask is built can overflow with a 64-bit decrementer.
>> Use sextract64() instead.
>>
>> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
>> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  v2: replaced MAKE_64BIT_MASK by sextract64
>>
>>  hw/ppc/ppc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index 7375bf4fa910..4f14464c9220 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>>      bool negative;
>>
>>      /* Truncate value to decr_width and sign extend for simplicity */
>> -    value &= ((1ULL << nr_bits) - 1);
>> +    value = sextract64(value, 0, nr_bits);
>>      negative = !!(value & (1ULL << (nr_bits - 1)));
>>      if (negative) {
>>          value |= (0xFFFFFFFFULL << nr_bits);
> 
> I think these lines that say "if negative then force all the
> high bits to one" are also no longer required. That is, this
> entire section of code:
>     value &= ((1ULL << nr_bits) - 1);
>     negative = !!(value & (1ULL << (nr_bits - 1)));
>     if (negative) {
>         value |= (0xFFFFFFFFULL << nr_bits);
>     }
> 
> is an open-coded sign-extension, which can all be replaced with
> the single line
>     value = sextract64(value, 0, nr_bits);

'negative' is used for more tests afterwards but you are right. I will respin 
with more changes. 

I am reluctant in changing too much because this is common code for PPC32
and PPC64. But, hey, I will do my best with the images I have.

> (and also: if nr_bits is 64 then the "<< nr_bits"
> is undefined behaviour.)

That's the initial issue raised by the new little PPC FPGA softcore called 
microwatt as it's using a 64bit decrementer.

Thanks,

C. 

> 
> thanks
> -- PMM
>

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Peter Maydell 2 years, 7 months ago

On Tue, 14 Sept 2021 at 10:47, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 9/14/21 11:19 AM, Peter Maydell wrote:
> > On Tue, 14 Sept 2021 at 09:56, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> The current way the mask is built can overflow with a 64-bit decrementer.
> >> Use sextract64() instead.
> >>
> >> Cc: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
> >> Fixes: a8dafa525181 ("target/ppc: Implement large decrementer support for TCG")
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >>  v2: replaced MAKE_64BIT_MASK by sextract64
> >>
> >>  hw/ppc/ppc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> >> index 7375bf4fa910..4f14464c9220 100644
> >> --- a/hw/ppc/ppc.c
> >> +++ b/hw/ppc/ppc.c
> >> @@ -876,7 +876,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
> >>      bool negative;
> >>
> >>      /* Truncate value to decr_width and sign extend for simplicity */
> >> -    value &= ((1ULL << nr_bits) - 1);
> >> +    value = sextract64(value, 0, nr_bits);
> >>      negative = !!(value & (1ULL << (nr_bits - 1)));
> >>      if (negative) {
> >>          value |= (0xFFFFFFFFULL << nr_bits);
> >
> > I think these lines that say "if negative then force all the
> > high bits to one" are also no longer required. That is, this
> > entire section of code:
> >     value &= ((1ULL << nr_bits) - 1);
> >     negative = !!(value & (1ULL << (nr_bits - 1)));
> >     if (negative) {
> >         value |= (0xFFFFFFFFULL << nr_bits);
> >     }
> >
> > is an open-coded sign-extension, which can all be replaced with
> > the single line
> >     value = sextract64(value, 0, nr_bits);
>
> 'negative' is used for more tests afterwards but you are right. I will respin
> with more changes.

After the sign-extension you can set 'negative' with
  negative = ((target_long)value) < 0;

PS: passing a negative value into muldiv64() to set the next
timer event (as the existing code does in some cases) seems a bit
odd; this probably ends up with an arithmetic overflow and setting
the next timeout to something unintended. But that's a separate
issue from the bug you're dealing with here.

-- PMM

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Richard Henderson 2 years, 7 months ago

On 9/14/21 2:47 AM, Cédric Le Goater wrote:
> On 9/14/21 11:19 AM, Peter Maydell wrote:
>>>       /* Truncate value to decr_width and sign extend for simplicity */
>>> -    value &= ((1ULL << nr_bits) - 1);
>>> +    value = sextract64(value, 0, nr_bits);
>>>       negative = !!(value & (1ULL << (nr_bits - 1)));
>>>       if (negative) {
>>>           value |= (0xFFFFFFFFULL << nr_bits);
>>
>> I think these lines that say "if negative then force all the
>> high bits to one" are also no longer required. That is, this
>> entire section of code:
>>      value &= ((1ULL << nr_bits) - 1);
>>      negative = !!(value & (1ULL << (nr_bits - 1)));
>>      if (negative) {
>>          value |= (0xFFFFFFFFULL << nr_bits);
>>      }
>>
>> is an open-coded sign-extension, which can all be replaced with
>> the single line
>>      value = sextract64(value, 0, nr_bits);
> 
> 'negative' is used for more tests afterwards but you are right. I will respin
> with more changes.

After the sign-extension, negative needs no complicated expression.

   value = sextract64(value, 0, nr_bits);
   negative = (target_long)value < 0;


r~

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Cédric Le Goater 2 years, 7 months ago

On 9/14/21 3:21 PM, Richard Henderson wrote:
> On 9/14/21 2:47 AM, Cédric Le Goater wrote:
>> On 9/14/21 11:19 AM, Peter Maydell wrote:
>>>>       /* Truncate value to decr_width and sign extend for simplicity */
>>>> -    value &= ((1ULL << nr_bits) - 1);
>>>> +    value = sextract64(value, 0, nr_bits);
>>>>       negative = !!(value & (1ULL << (nr_bits - 1)));
>>>>       if (negative) {
>>>>           value |= (0xFFFFFFFFULL << nr_bits);
>>>
>>> I think these lines that say "if negative then force all the
>>> high bits to one" are also no longer required. That is, this
>>> entire section of code:
>>>      value &= ((1ULL << nr_bits) - 1);
>>>      negative = !!(value & (1ULL << (nr_bits - 1)));
>>>      if (negative) {
>>>          value |= (0xFFFFFFFFULL << nr_bits);
>>>      }
>>>
>>> is an open-coded sign-extension, which can all be replaced with
>>> the single line
>>>      value = sextract64(value, 0, nr_bits);
>>
>> 'negative' is used for more tests afterwards but you are right. I will respin
>> with more changes.
> 
> After the sign-extension, negative needs no complicated expression.
> 
>   value = sextract64(value, 0, nr_bits);
>   negative = (target_long)value < 0;

Yes. I was wondering about the 'target_ulong' type used for the value 
and decr variables. The code has below : 

    ...
    if ((value < 3) ||
    ...

and then another sign calculation on a target_ulong

       ...
       && !(decr & (1ULL << (nr_bits - 1))))) {
       ...
 
We should introduce intermediate 'int64_t' variables to extract the 
sign values from the target_ulong. That would be cleaner.

Thanks,

C.

Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

Posted by Richard Henderson 2 years, 7 months ago

On 9/14/21 6:43 AM, Cédric Le Goater wrote:
> and then another sign calculation on a target_ulong
> 
>         ...
>         && !(decr & (1ULL << (nr_bits - 1))))) {

I was wondering if that was supposed to be an unsigned test for a "small" value (i.e. decr 
< MAKE_64BIT_MASK(0, nr_bits)?  Certainly decr should never be negative, since the 
decrementer never increments, and I can't figure out what it's supposed to mean otherwise.

> We should introduce intermediate 'int64_t' variables to extract the
> sign values from the target_ulong. That would be cleaner.

Yes it would.  The underflow test becomes easier for certain.

r~