Re: [PATCH v2] hw/timer/hpet: Fix wrong HPET interrupts

[Paolo Bonzini <pbonzini@redhat.com>]

On 7/13/24 13:54, TaiseiIto wrote:
> Before this commit, there are 3 problems about HPET timer interrupts. First,
> HPET periodic timers cause a too early interrupt before HPET main counter
> value reaches a value written its comparator value register. Second,
> disabled HPET timers whose comparator value register is not
> 0xffffffffffffffff cause wrong interrupts. Third, enabled HPET timers whose
> comparator value register is 0xffffffffffffffff don't cause any interrupts.
> About the first one, for example, an HPET driver writes 0x00000000aaaaaaaa
> to an HPET periodic timer comparator value register. As a result, the
> register becomes 0xffffffffaaaaaaaa because writing to the higher 32 bits of
> the register doesn't affect itself in periodic mode. (see
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function.) And "timer->period"
> which means interrupt period in periodic mode becomes 0xaaaaaaaa. Next, the
> HPET driver sets the HPET_CFG_ENABLE flag to start the main counter. The
> comparator value register (0xffffffffaaaaaaaa) indicate the next interrupt
> time. The period (0xaaaaaaaa) is added to the comparator value register at
> "hpet_timer" function because "hpet_time_after64" function returns true when
> the main counter is small. So, the first interrupt is planned when the main
> counter is 0x0000000055555554, but the first interrupt should occur when the
> main counter is 0x00000000aaaaaaaa. To solve this problem, I fix
> "case HPET_TN_CMP + 4" of "hpet_ram_write" function to ensure that writings
> to higher 32 bits of a comparator value register reflect itself even if in
> periodic mode. About the other two problems, it was decided by comparator
> value whether each timer is enabled, but it should be decided by
> "timer_enabled" function which confirm "HPET_TN_ENABLE" flag. To solve these
> problems, I fix the code to decide correctly whether each timer is enabled.
> After this commit, the 3 problems are solved. First, HPET periodic timers
> cause the first interrupt when the main counter value reaches a value
> written its comparator value register. Second, disabled HPET timers never
> cause any interrupt. Third, enabled HPET timers cause interrupts correctly
> even if an HPET driver writes 0xffffffffffffffff to its comparator value
> register.
> 
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
> 
> Changes in v2:
> - Reflect writings to higher 32 bits of a comparator value register rather
>    than clearing these bits.
> - Fix wrong indents.
> - Link to v1: https://lore.kernel.org/qemu-devel/TY0PR0101MB4285838139BC56DEC3D1CCFDA4CE2@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
> 
>   hw/timer/hpet.c | 24 +++++++++++++++---------
>   1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..4b6352e257 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -552,6 +552,10 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>                   timer->period =
>                       (timer->period & 0xffffffff00000000ULL) | new_val;
>               }
> +            /*
> +             * FIXME: on a 64-bit write, HPET_TN_SETVAL should apply to the
> +             * high bits part as well.
> +             */
>               timer->config &= ~HPET_TN_SETVAL;
>               if (hpet_enabled(s)) {
>                   hpet_set_timer(timer);
> @@ -562,20 +566,22 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>               if (!timer_is_periodic(timer)
>                   || (timer->config & HPET_TN_SETVAL)) {
>                   timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;
> -            } else {
> +            }
> +            if (timer_is_periodic(timer)) {
>                   /*
>                    * FIXME: Clamp period to reasonable min value?
>                    * Clamp period to reasonable max value
>                    */
> -                new_val &= (timer->config & HPET_TN_32BIT ? ~0u : ~0ull) >> 1;
> +                new_val = MIN(new_val, ~0u >> 1);
> +                timer->cmp = (timer->cmp & 0xffffffffULL) | new_val << 32;

This seems wrong to me.  The comparator must be reset using 
HPET_TN_SETVAL, otherwise it just keeps running.  From the specification:

"If software wants to change the periodic rate, it should write a new 
value to the comparator value register.  At the point when the timer’s 
comparator indicates a match, this new value will be added to derive the 
next matching point. So as to avoid race conditions where the new value 
is written just as a match occurs, either the main counter should be 
halted or the comparator disabled when the new periodic rate is written".

The sentence "at the point when the timer’s comparator indicates a 
match" indicates that the comparator register is not written.

I suspect you're hitting the other issue, and HPET_TN_SETVAL is cleared 
incorrectly by a 64-bit write.  I'll try sending out a patch to fix that.

> -                    if ((&s->timer[i])->cmp != ~0ULL) {
> +                    if (hpet_enabled(s)) {
>                           hpet_set_timer(&s->timer[i]);
>                       }
>                   }

This is incorrect too.  "hpet_enabled(s)" is always true here; I think 
the bug is that timer_enabled() should only affect generation of 
interrupts.  Are you perhaps using level-triggered interrupts?  If so, 
it makes sense that you want the timer to be running - and then when 
setting HPET_TN_ENABLE the interrupt will fire.

However, we're making progress; you're just finding more bugs.

I have some ideas on what needs to be fixed next, and I placed them at a 
branch "hpet" of https://gitlab.com/bonzini/qemu.git.  Unfortunately, I 
don't have a way to test them (writing testcases is on my list but I 
don't have time right now) and as the branch proceeds things get more 
experimental; but let me know if it helps or if you can make small 
changes to the patches that fix your testcase.

Paolo