Re: [RFC PATCH] hw/timer/hpet: fix IRQ routing in legacy support mode

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3596 bytes --]

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);
>      }


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]