[PATCH v1 1/2] target/ppc: Fix decrementer time underflow and infinite timer loop

[PATCH v1 1/2] target/ppc: Fix decrementer time underflow and infinite timer loop

[Nicholas Piggin]

It is possible to store a very large value to the decrementer that it
does not raise the decrementer exception so the timer is scheduled, but
the next time value wraps and is treated as in the past.

This can occur if (u64)-1 is stored on a zero-triggered exception, or
(u64)-1 is stored twice on an underflow-triggered exception, for
example.

If such a value is set in DECAR, it gets stored to the decrementer by
the timer function, which then immediately causes another timer, which
hangs QEMU.

Clamp the decrementer to the implemented width, and use that as the
value for the timer calculation, effectively preventing this overflow.

Reported-by: sdicaro@DDCI.com
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
sdicaro@DDCI.com debugged and reported this, I just changed their fix
to extract variable bits so it works with large decrementer. So most
of the credit goes to them.

Thanks,
Nick

 hw/ppc/ppc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 4e816c68c7..d80b0adc6c 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -798,6 +798,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     int64_t signed_decr;
 
     /* Truncate value to decr_width and sign extend for simplicity */
+    value = extract64(value, 0, nr_bits);
+    decr = extract64(decr, 0, nr_bits);
     signed_value = sextract64(value, 0, nr_bits);
     signed_decr = sextract64(decr, 0, nr_bits);
 
-- 
2.40.1

[PATCH v1 2/2] target/ppc: Decrementer fix BookE semantics

[Nicholas Piggin]

The decrementer store function has logic that short-cuts the timer if a
very small value is stored (0, 1, or 2) and raises an interrupt
directly. There are two problem with this on BookE.

First is that BookE says a decrementer interrupt should not be raised
on a store of 0, only of a decrement from 1. Second is that raising
the irq directly will bypass the auto-reload logic in the booke decr
timer function, breaking autoreload when 1 or 2 is stored.

Fix this by removing that small-value special case. It makes this
tricky logic even more difficult to reason about, and it hardly matters
for performance.

Cc: sdicaro@DDCI.com
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
These were some booke decrementer corner case issues I saw, probably
a little less important than the first patch so could go later.

Thanks,
Nick

 hw/ppc/ppc.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index d80b0adc6c..1b1220c423 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -811,11 +811,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     }
 
     /*
-     * Going from 2 -> 1, 1 -> 0 or 0 -> -1 is the event to generate a DEC
-     * interrupt.
-     *
-     * If we get a really small DEC value, we can assume that by the time we
-     * handled it we should inject an interrupt already.
+     * Going from 1 -> 0 or 0 -> -1 is the event to generate a DEC interrupt.
      *
      * On MSB level based DEC implementations the MSB always means the interrupt
      * is pending, so raise it on those.
@@ -823,8 +819,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
      * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
      * an edge interrupt, so raise it here too.
      */
-    if ((value < 3) ||
-        ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
+    if (((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
         ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
           && signed_decr >= 0)) {
         (*raise_excp)(cpu);
-- 
2.40.1

Re: [PATCH v1 1/2] target/ppc: Fix decrementer time underflow and infinite timer loop

[Daniel Henrique Barboza]

On 5/30/23 10:12, Nicholas Piggin wrote:
> It is possible to store a very large value to the decrementer that it
> does not raise the decrementer exception so the timer is scheduled, but
> the next time value wraps and is treated as in the past.
> 
> This can occur if (u64)-1 is stored on a zero-triggered exception, or
> (u64)-1 is stored twice on an underflow-triggered exception, for
> example.
> 
> If such a value is set in DECAR, it gets stored to the decrementer by
> the timer function, which then immediately causes another timer, which
> hangs QEMU.
> 
> Clamp the decrementer to the implemented width, and use that as the
> value for the timer calculation, effectively preventing this overflow.
> 
> Reported-by: sdicaro@DDCI.com
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

And queued.


Daniel

> sdicaro@DDCI.com debugged and reported this, I just changed their fix
> to extract variable bits so it works with large decrementer. So most
> of the credit goes to them.
> 
> Thanks,
> Nick
> 
>   hw/ppc/ppc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 4e816c68c7..d80b0adc6c 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -798,6 +798,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>       int64_t signed_decr;
>   
>       /* Truncate value to decr_width and sign extend for simplicity */
> +    value = extract64(value, 0, nr_bits);
> +    decr = extract64(decr, 0, nr_bits);
>       signed_value = sextract64(value, 0, nr_bits);
>       signed_decr = sextract64(decr, 0, nr_bits);
>   

Re: [PATCH v1 2/2] target/ppc: Decrementer fix BookE semantics

[Daniel Henrique Barboza]

On 5/30/23 10:12, Nicholas Piggin wrote:
> The decrementer store function has logic that short-cuts the timer if a
> very small value is stored (0, 1, or 2) and raises an interrupt
> directly. There are two problem with this on BookE.
> 
> First is that BookE says a decrementer interrupt should not be raised
> on a store of 0, only of a decrement from 1. Second is that raising
> the irq directly will bypass the auto-reload logic in the booke decr
> timer function, breaking autoreload when 1 or 2 is stored.
> 
> Fix this by removing that small-value special case. It makes this
> tricky logic even more difficult to reason about, and it hardly matters
> for performance.
> 
> Cc: sdicaro@DDCI.com
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

And queued.


Daniel


> These were some booke decrementer corner case issues I saw, probably
> a little less important than the first patch so could go later.
> 
> Thanks,
> Nick
> 
>   hw/ppc/ppc.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index d80b0adc6c..1b1220c423 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -811,11 +811,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>       }
>   
>       /*
> -     * Going from 2 -> 1, 1 -> 0 or 0 -> -1 is the event to generate a DEC
> -     * interrupt.
> -     *
> -     * If we get a really small DEC value, we can assume that by the time we
> -     * handled it we should inject an interrupt already.
> +     * Going from 1 -> 0 or 0 -> -1 is the event to generate a DEC interrupt.
>        *
>        * On MSB level based DEC implementations the MSB always means the interrupt
>        * is pending, so raise it on those.
> @@ -823,8 +819,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
>        * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
>        * an edge interrupt, so raise it here too.
>        */
> -    if ((value < 3) ||
> -        ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
> +    if (((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) ||
>           ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < 0
>             && signed_decr >= 0)) {
>           (*raise_excp)(cpu);

Re: [PATCH v1 1/2] target/ppc: Fix decrementer time underflow and infinite timer loop

[Michael Tokarev]

30.05.2023 16:12, Nicholas Piggin wrote:
> It is possible to store a very large value to the decrementer that it
> does not raise the decrementer exception so the timer is scheduled, but
> the next time value wraps and is treated as in the past.
> 
> This can occur if (u64)-1 is stored on a zero-triggered exception, or
> (u64)-1 is stored twice on an underflow-triggered exception, for
> example.
> 
> If such a value is set in DECAR, it gets stored to the decrementer by
> the timer function, which then immediately causes another timer, which
> hangs QEMU.
> 
> Clamp the decrementer to the implemented width, and use that as the
> value for the timer calculation, effectively preventing this overflow.
> 
> Reported-by: sdicaro@DDCI.com
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> sdicaro@DDCI.com debugged and reported this, I just changed their fix
> to extract variable bits so it works with large decrementer. So most
> of the credit goes to them.
> 
> Thanks,
> Nick
> 
>   hw/ppc/ppc.c | 2 ++
>   1 file changed, 2 insertions(+)

Is it a -stable material?  From the description it smells like it is.

Thanks,

/mjt

Re: [PATCH v1 1/2] target/ppc: Fix decrementer time underflow and infinite timer loop

[Daniel Henrique Barboza]

On 6/7/23 06:26, Michael Tokarev wrote:
> 30.05.2023 16:12, Nicholas Piggin wrote:
>> It is possible to store a very large value to the decrementer that it
>> does not raise the decrementer exception so the timer is scheduled, but
>> the next time value wraps and is treated as in the past.
>>
>> This can occur if (u64)-1 is stored on a zero-triggered exception, or
>> (u64)-1 is stored twice on an underflow-triggered exception, for
>> example.
>>
>> If such a value is set in DECAR, it gets stored to the decrementer by
>> the timer function, which then immediately causes another timer, which
>> hangs QEMU.
>>
>> Clamp the decrementer to the implemented width, and use that as the
>> value for the timer calculation, effectively preventing this overflow.
>>
>> Reported-by: sdicaro@DDCI.com
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> sdicaro@DDCI.com debugged and reported this, I just changed their fix
>> to extract variable bits so it works with large decrementer. So most
>> of the credit goes to them.
>>
>> Thanks,
>> Nick
>>
>>   hw/ppc/ppc.c | 2 ++
>>   1 file changed, 2 insertions(+)
> 
> Is it a -stable material?  From the description it smells like it is.

Feel free to pick it for -stable.  Thanks,

Daniel

> 
> Thanks,
> 
> /mjt
>