TESTED: netdde works on smp with this patch
On 7/18/25 9:13 PM, Damien Zammit wrote:
> We now have a different strategy for EOI depending on trigger mode:
> For edge triggered, the eoi comes before the handler so we don't
> miss interrupts. For level triggered, the eoi comes after the handler
> since a high interrupt line doesn't keep triggering until it has
> been eoi'd so makes sense to handle those first.
>
> To facilitate this, we store a helper struct irqinfo that
> contains the vector and the trigger mode per GSI, so we can look up
> in O(1) from memory instead of asking hardware for the trigger mode
> and vector every time we need it.
>
> TESTED: On SMP this stops failure in xhci from hanging rumpnet.
>
> When a second usb driver is executed, the second one detects a problem
> and disables the uhub0 port, which breaks the first usb driver.
> But this does not block rumpnet from serving packets, sharing irq 11.
> The usb stack can be recovered by killing both usb drivers and
> restarting one. Before this change, the behaviour was that irq 11
> became stuck and unusable for any device.
>
> Parallel compiling speed seems improved with this commit on SMP.
>
> UP+apic still works as per usual.
>
> TODO: We still need to work out a strategy to have interrupts enabled
> during the handler, so that nested interrupts that occur via code that
> is executed inside the irq handler to make the device raise a new
> interrupt, are triggered. Currently these do not work properly.
> I believe this is the remaining bug with interrupts.
>
> ---
> i386/i386/apic.h | 5 +++++
> i386/i386at/interrupt.S | 49 +++++++++++++++++++++++++++--------------
> i386/i386at/ioapic.c | 15 +++++++++++--
> x86_64/interrupt.S | 46 +++++++++++++++++++++++++-------------
> 4 files changed, 82 insertions(+), 33 deletions(-)
>
> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> index 92fb900a..a91fc9ac 100644
> --- a/i386/i386/apic.h
> +++ b/i386/i386/apic.h
> @@ -235,6 +235,11 @@ typedef struct ApicInfo {
> struct IrqOverrideData irq_override_list[MAX_IRQ_OVERRIDE];
> } ApicInfo;
>
> +struct irqinfo {
> + uint8_t trigger;
> + uint8_t vector;
> +};
> +
> int apic_data_init(void);
> void apic_add_cpu(uint16_t apic_id);
> void apic_lapic_init(ApicLocalUnit* lapic_ptr);
> diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S
> index 77424b43..4662d024 100644
> --- a/i386/i386at/interrupt.S
> +++ b/i386/i386at/interrupt.S
> @@ -61,6 +61,35 @@ ENTRY(interrupt)
> je _call_local_ast
> #endif
>
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _eoi /* no, edge: eoi before handling */
> +
> +_call_handler:
> +
> + movl S_IPL,%eax
> + movl %eax,4(%esp) /* previous ipl as 2nd arg */
> +
> + movl S_RET,%eax
> + movl %eax,8(%esp) /* return address as 3rd arg */
> +
> + movl S_REGS,%eax
> + movl %eax,12(%esp) /* address of interrupted registers as
> 4th arg */
> +
> + movl S_IRQ,%eax /* copy irq number */
> +
> + shll $2,%eax /* irq * 4 */
> + movl EXT(iunit)(%eax),%edx /* get device unit number */
> + movl %edx,(%esp) /* unit number as 1st arg */
> +
> + call *EXT(ivect)(%eax) /* call interrupt handler */
> +
> + movl S_IRQ,%ecx /* restore irq number */
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _completed /* no, edge: we are done */
> +
> +_eoi:
> #ifndef APIC
> movl $1,%eax
> shll %cl,%eax /* get corresponding IRQ mask */
> @@ -102,22 +131,10 @@ ENTRY(interrupt)
> call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */
> #endif
>
> - movl S_IPL,%eax
> - movl %eax,4(%esp) /* previous ipl as 2nd arg */
> -
> - movl S_RET,%eax
> - movl %eax,8(%esp) /* return address as 3rd arg */
> -
> - movl S_REGS,%eax
> - movl %eax,12(%esp) /* address of interrupted registers as
> 4th arg */
> -
> - movl S_IRQ,%eax /* copy irq number */
> -
> - shll $2,%eax /* irq * 4 */
> - movl EXT(iunit)(%eax),%edx /* get device unit number */
> - movl %edx,(%esp) /* unit number as 1st arg */
> -
> - call *EXT(ivect)(%eax) /* call interrupt handler */
> + movl S_IRQ,%ecx /* restore irq number */
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _call_handler /* no, edge: call handler */
>
> _completed:
> movl S_IPL,%eax /* restore previous ipl */
> diff --git a/i386/i386at/ioapic.c b/i386/i386at/ioapic.c
> index a6c0fd6a..5dc7321c 100644
> --- a/i386/i386at/ioapic.c
> +++ b/i386/i386at/ioapic.c
> @@ -128,6 +128,9 @@ interrupt_handler_fn ivect[NINTR] = {
> /* 63 */ intnull,
> };
>
> +struct irqinfo irqinfo[NINTR];
> +struct irqinfo *irq_info = &irqinfo[0];
> +
> void
> picdisable(void)
> {
> @@ -329,8 +332,7 @@ ioapic_irq_eoi(int pin)
> //}
> } else {
> volatile ApicIoUnit *ioapic = apic_get_ioapic(apic)->ioapic;
> - ioapic_read_entry(apic, pin, &entry.both);
> - ioapic->eoi.r = entry.both.vector;
> + ioapic->eoi.r = irq_info[pin].vector;
> }
>
> simple_unlock_irq(s, &ioapic_lock);
> @@ -420,6 +422,9 @@ ioapic_configure(void)
> entry.both.vector = IOAPIC_INT_BASE + gsi;
> ioapic_write_entry(apic, pin, entry.both);
>
> + irq_info[pin].vector = entry.both.vector;
> + irq_info[pin].trigger = entry.both.trigger;
> +
> /* Set initial state to masked */
> mask_irq(pin);
>
> @@ -454,6 +459,9 @@ ioapic_configure(void)
> entry.both.vector = IOAPIC_INT_BASE + gsi;
> ioapic_write_entry(apic, pin, entry.both);
>
> + irq_info[pin].vector = entry.both.vector;
> + irq_info[pin].trigger = entry.both.trigger;
> +
> /* Set initial state to masked */
> mask_irq(pin);
> }
> @@ -478,6 +486,9 @@ ioapic_configure(void)
> entry.both.vector = IOAPIC_INT_BASE + gsi;
> ioapic_write_entry(apic, pin, entry.both);
>
> + irq_info[pin + ngsis].vector = entry.both.vector;
> + irq_info[pin + ngsis].trigger = entry.both.trigger;
> +
> /* Set initial state to masked */
> mask_irq(pin + ngsis);
> }
> diff --git a/x86_64/interrupt.S b/x86_64/interrupt.S
> index 6fb77727..533b0df5 100644
> --- a/x86_64/interrupt.S
> +++ b/x86_64/interrupt.S
> @@ -61,6 +61,33 @@ ENTRY(interrupt)
> je _call_local_ast
> #endif
>
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _eoi /* no, edge: eoi before handling */
> +
> +_call_handler:
> + ;
> + movq S_IPL,S_ARG1 /* previous ipl as 2nd arg */
> +
> + ;
> + movq S_RET,S_ARG2 /* return address as 3th arg */
> +
> + ;
> + movq S_REGS,S_ARG3 /* address of interrupted registers as
> 4th arg */
> +
> + movl S_IRQ,%eax /* copy irq number */
> + shll $2,%eax /* irq * 4 */
> + movl EXT(iunit)(%rax),%edi /* get device unit number as 1st arg */
> +
> + shll $1,%eax /* irq * 8 */
> + call *EXT(ivect)(%rax) /* call interrupt handler */
> +
> + movl S_IRQ,%ecx /* restore irq number */
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _completed /* no, edge: we are done */
> +
> +_eoi:
> #ifndef APIC
> movl $1,%eax
> shll %cl,%eax /* get corresponding IRQ mask */
> @@ -102,21 +129,10 @@ ENTRY(interrupt)
> call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */
> #endif
>
> - ;
> - movq S_IPL,S_ARG1 /* previous ipl as 2nd arg */
> -
> - ;
> - movq S_RET,S_ARG2 /* return address as 3th arg */
> -
> - ;
> - movq S_REGS,S_ARG3 /* address of interrupted registers as
> 4th arg */
> -
> - movl S_IRQ,%eax /* copy irq number */
> - shll $2,%eax /* irq * 4 */
> - movl EXT(iunit)(%rax),%edi /* get device unit number as 1st arg */
> -
> - shll $1,%eax /* irq * 8 */
> - call *EXT(ivect)(%rax) /* call interrupt handler */
> + movl S_IRQ,%ecx /* restore irq number */
> + movb %cs:EXT(irqinfo)(%ecx),%al /* look up
> irq_info[irq].trigger */
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _call_handler /* no, edge: call handler after eoi */
>
> _completed:
> movl S_IPL,%edi /* restore previous ipl */
>
