Message ID | 6c1f2ced5b329a86a5c6846fb8530f2f1a845a44.camel@infradead.org |
---|---|
State | New |
Headers | show |
Series | [RFC] hw/timer/hpet: fix IRQ routing in legacy support mode | expand |
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); > }
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); }