Hello, Sometimes the "why" of a commit is obvious, so it doesn't need to be explained, but here it's really not and thus it definitely needs to be. We have had various pings-pongs in the past about whether to EOI before/after the interrupt, masking or not, etc. So we really need a firm explanation, recorded in the git history, why we believe the proposed way is now correct.
Samuel Damien Zammit, le dim. 01 oct. 2023 04:58:35 +0000, a ecrit: > --- > i386/i386at/interrupt.S | 19 ++++++++++++++++--- > x86_64/interrupt.S | 19 +++++++++++++++---- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S > index 8ae6b97c..ec2fc656 100644 > --- a/i386/i386at/interrupt.S > +++ b/i386/i386at/interrupt.S > @@ -98,11 +98,24 @@ ENTRY(interrupt) > outb %al,$(PIC_MASTER_OCW) /* unmask master */ > 2: > #else > - cmpl $16,%ecx /* was this a low ISA intr? */ > - jge _no_eoi /* no, must be PCI (let irq_ack handle > EOI) */ So we are now EOI-ing all irqs? The comment was saying we were leaving that up to irq_ack for IRQs above 16. Was that actually wrong/changed? > movl %ecx,(%esp) /* load irq number as 1st arg */ > + movl $IOAPIC_MASK_DISABLED,4(%esp) /* load disabled flag into 2nd > arg */ > + call EXT(ioapic_toggle) /* ioapic mask irq so upon re-entry we > dont nest */ > + movl S_IRQ,%eax > + movl %eax,(%esp) /* load irq number as 1st arg */ > call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */ > -_no_eoi: > + movl S_IRQ,%eax > + cmpl $9,%eax /* was this a high PCI interrupt? >= 9 > except 12 */ > + jge _no_unmask /* yes, dont unmask until acked */ > +_unmask: > + movl %eax,(%esp) /* load irq number as 1st arg */ > + movl $IOAPIC_MASK_ENABLED,4(%esp) /* load enabled flag into 2nd > arg */ > + call EXT(ioapic_toggle) /* ioapic unmask irq to re-enable */ > + jmp _done ? I don't see the point of masking while doing the EOI for the non-PCI irqs? The nested interrupt will just happen later. Perhaps you are just pushing the issue further for them, where it'll be even harder to reproduce and thus harder to fix. > +_no_unmask: > + cmpl $12,%eax /* was this the mouse interrupt? */ > + je _unmask /* yes, unmask */ Does IRQ #13 (FPU) not also need special care? (possibly also on x86_64?) > +_done: > #endif > > movl S_IPL,%eax > diff --git a/x86_64/interrupt.S b/x86_64/interrupt.S > index fcd5a032..85f94da6 100644 > --- a/x86_64/interrupt.S > +++ b/x86_64/interrupt.S > @@ -98,11 +98,22 @@ ENTRY(interrupt) > outb %al,$(PIC_MASTER_OCW) /* unmask master */ > 2: > #else > - cmpl $16,%ecx /* was this a low ISA intr? */ > - jge _no_eoi /* no, must be PCI (let irq_ack handle > EOI) */ > - movl %ecx,%edi /* load irq number as 1st arg */ > + movl S_IRQ,S_ARG0 /* load irq number as 1st arg */ > + movl $IOAPIC_MASK_DISABLED,S_ARG1 /* load disabled flag into 2nd > arg */ > + call EXT(ioapic_toggle) /* ioapic mask irq so upon re-entry we > dont nest */ > + movl S_IRQ,S_ARG0 /* load irq number as 1st arg */ > call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */ > -_no_eoi: > + movl S_IRQ,S_ARG0 /* load irq number as 1st arg */ > + cmpl $9,S_ARG0 /* was this a high PCI interrupt? >= 9 > except 12 */ > + jge _no_unmask /* yes, dont unmask until acked */ > +_unmask: > + movl $IOAPIC_MASK_ENABLED,S_ARG1 /* load enabled flag into 2nd > arg */ > + call EXT(ioapic_toggle) /* ioapic unmask irq to re-enable */ > + jmp _done > +_no_unmask: > + cmpl $12,S_ARG0 /* was this the mouse interrupt? */ > + je _unmask /* yes, unmask */ > +_done: > #endif
