Hello,
Please do test without apic too, your change currently breaks it.
This also needs to update the comment above irq_acknowledge().
Damien Zammit, le ven. 18 juil. 2025 11:13:01 +0000, a ecrit:
> 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.
"Makes sense" is not explanatory enough :)
The current state of the code is indeed odd for level-triggered: we eoi
the interrupt, then (for user-level intrs) mask it on the io apic, and
then let user handle it. I don't know the hardware details about apics,
but it looks to me like for level-triggered irqs, since we eoi the intr
before masking it on the io apic, the io apic could raise the interrupt
again there, which doesn't stack because we have IF cleared, but could
get that interrupt immediately after queueing the previous for userland
or such.
So we'd rather indeed eoi *after* masking, I am convinced of this. I'm
however not convinced by it explaining fixing the issues below.
> 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.
I'm surprised that this change fixes it. I fear we are just hiding
a bug rather than solving it... Does the usb stack properly ack its
interrupts?
Do the interrupts *always* happen on the BSP?
If a user-land driver does not behave correctly concerning interrupts,
yes, the interrupt will become unusable, that is expected: as long as
the interrupt has not been confirmed to be processed by userland, we do
not want to allow it again, because the hardware will just keep the
level high, and interrupts will pile on.
> Parallel compiling speed seems improved with this commit on SMP.
I'm also surprised by this. Does it not help also on UP?
> 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.
AIUI, this can be fixed by calling eoi in queue_intr after disabling the
irq but before actually queueing the irq. But let's get your current
change in first for a start.
> ---
> 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;
> +};
Please also declare the irqinfo array. Even if the assembly won't use
that declaration, better be explicit about it for information.
> 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 */
The %cs: override shouldn't be needed.
The index seems wrong: AIUI it should be (%ecx,2), to properly offset by
two times the irq number.
> + testb $1,%al /* was this a level triggered
> interrupt? */
> + jz _eoi /* no, edge: eoi before handling */
Since the non-apic case will want eoi first, better keep the source code
in that order.
> 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];
Why the irq_info intermediate? You should be able to just directly use
irqinfo[pin].
Samuel
