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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 18, 2024 at 01:10:44PM +0000, 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 the code to
> clear the higher 32 bits of comparator value registers of periodic mode
> timers when HPET starts the main counter. 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 0xffffffff to its
> comparator value register.
> 
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>

I didn't get this patch previously. Donnu why.
Tagged now.

> ---
>  hw/timer/hpet.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 01efe4885d..2dcefa7049 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -599,8 +599,12 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
>                  s->hpet_offset =
>                      ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>                  for (i = 0; i < s->num_timers; i++) {
> -                    if ((&s->timer[i])->cmp != ~0ULL) {
> -                        hpet_set_timer(&s->timer[i]);
> +                    HPETTimer *timer = &s->timer[i];
> +                    if (timer_enabled(timer)) {
> +                        if (timer_is_periodic(timer)) {
> +                            timer->cmp &= 0xffffffffULL;
> +                        }
> +                        hpet_set_timer(timer);
>                      }
>                  }
>              } else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
> -- 
> 2.34.1
> 
>