From: David Woodhouse <dwmw@amazon.co.uk>

The interrupt from timer 0 in legacy mode is supposed to go to IRQ 0 on
the i8259 and IRQ 2 on the I/O APIC. The generic x86 GSI handling can't
cope with IRQ numbers differing between the two chips (despite it also
being the case for PCI INTx routing), so add a special case for the HPET.

IRQ 2 isn't valid on the i8259; it's the cascade IRQ and would be
interpreted as spurious interrupt on the secondary PIC. So we can fix
up all attempts to deliver IRQ2, to actually deliver to IRQ0 on the PIC.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
It ain't ever so pretty, but it's prettier than the INTx routing hack
that I just documented and at least this one doesn't rely on guest
behaviour.

Do we have tests for HPET interrupt delivery that can be extended to
cover this?


 hw/i386/x86.c   | 19 +++++++++++++++----
 hw/timer/hpet.c |  5 ++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index a88a126123..0d2c74f2d9 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -602,13 +602,24 @@ DeviceState *cpu_get_current_apic(void)
 void gsi_handler(void *opaque, int n, int level)
 {
     GSIState *s = opaque;
-
+    int i8259_pin = n;
     trace_x86_gsi_interrupt(n, level);
     switch (n) {
-    case 0 ... ISA_NUM_IRQS - 1:
-        if (s->i8259_irq[n]) {
+    case 2:
+        /*
+         * Special case for HPET legacy mode, which is defined as routing HPET
+         * timer 0 to IRQ2 of the I/O APIC and IRQ0 of the i8259 PIC. Since
+         * IRQ2 on the i8259 is the cascade, it isn't otherwise valid so we
+         * handle it via this special case.
+         */
+        i8259_pin = 0;
+        /* fall through */
+    case 0:
+    case 1:
+    case 3 ... ISA_NUM_IRQS - 1:
+        if (s->i8259_irq[i8259_pin]) {
             /* Under KVM, Kernel will forward to both PIC and IOAPIC */
-            qemu_set_irq(s->i8259_irq[n], level);
+            qemu_set_irq(s->i8259_irq[i8259_pin], level);
         }
         /* fall through */
     case ISA_NUM_IRQS ... IOAPIC_NUM_PINS - 1:
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 6998094233..9f740ffdee 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -196,8 +196,11 @@ static void update_irq(struct HPETTimer *timer, int set)
         /* if LegacyReplacementRoute bit is set, HPET specification requires
          * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
          * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
+         *
+         * There is a special case in the x86 gsi_handler() which converts
+         * IRQ2 into IRQ0 for the i8259 PIC and makes this work correctly.
          */
-        route = (timer->tn == 0) ? 0 : RTC_ISA_IRQ;
+        route = (timer->tn == 0) ? 2 : RTC_ISA_IRQ;
     } else {
         route = timer_int_route(timer);
     }
-- 
2.34.1

On Wed, 2023-08-30 at 21:20 +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The interrupt from timer 0 in legacy mode is supposed to go to IRQ 0 on
> the i8259 and IRQ 2 on the I/O APIC. The generic x86 GSI handling can't
> cope with IRQ numbers differing between the two chips (despite it also
> being the case for PCI INTx routing), so add a special case for the HPET.
> 
> IRQ 2 isn't valid on the i8259; it's the cascade IRQ and would be
> interpreted as spurious interrupt on the secondary PIC. So we can fix
> up all attempts to deliver IRQ2, to actually deliver to IRQ0 on the PIC.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> It ain't ever so pretty, but it's prettier than the INTx routing hack
> that I just documented and at least this one doesn't rely on guest
> behaviour.


I haven't come up with a better way of doing it, and nobody seemed to
care. Shall I post an identical patch without the [RFC] to see if it
elicits more of a response?

> 
> Do we have tests for HPET interrupt delivery that can be extended to
> cover this?
> 
> 
>  hw/i386/x86.c   | 19 +++++++++++++++----
>  hw/timer/hpet.c |  5 ++++-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index a88a126123..0d2c74f2d9 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -602,13 +602,24 @@ DeviceState *cpu_get_current_apic(void)
>  void gsi_handler(void *opaque, int n, int level)
>  {
>      GSIState *s = opaque;
> -
> +    int i8259_pin = n;
>      trace_x86_gsi_interrupt(n, level);
>      switch (n) {
> -    case 0 ... ISA_NUM_IRQS - 1:
> -        if (s->i8259_irq[n]) {
> +    case 2:
> +        /*
> +         * Special case for HPET legacy mode, which is defined as routing HPET
> +         * timer 0 to IRQ2 of the I/O APIC and IRQ0 of the i8259 PIC. Since
> +         * IRQ2 on the i8259 is the cascade, it isn't otherwise valid so we
> +         * handle it via this special case.
> +         */
> +        i8259_pin = 0;
> +        /* fall through */
> +    case 0:
> +    case 1:
> +    case 3 ... ISA_NUM_IRQS - 1:
> +        if (s->i8259_irq[i8259_pin]) {
>              /* Under KVM, Kernel will forward to both PIC and IOAPIC */
> -            qemu_set_irq(s->i8259_irq[n], level);
> +            qemu_set_irq(s->i8259_irq[i8259_pin], level);
>          }
>          /* fall through */
>      case ISA_NUM_IRQS ... IOAPIC_NUM_PINS - 1:
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 6998094233..9f740ffdee 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -196,8 +196,11 @@ static void update_irq(struct HPETTimer *timer, int set)
>          /* if LegacyReplacementRoute bit is set, HPET specification requires
>           * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
>           * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
> +         *
> +         * There is a special case in the x86 gsi_handler() which converts
> +         * IRQ2 into IRQ0 for the i8259 PIC and makes this work correctly.
>           */
> -        route = (timer->tn == 0) ? 0 : RTC_ISA_IRQ;
> +        route = (timer->tn == 0) ? 2 : RTC_ISA_IRQ;
>      } else {
>          route = timer_int_route(timer);
>      }