Applied, thanks! Damien Zammit, le lun. 02 oct. 2023 03:39:13 +0000, a ecrit: > Logic for interrupts: > > - interrupt.S raises spl (thus IF cleared) > - interrupt.S EOI > - interrupt.S calls the handler > - for pure in-kernel handlers, they do whatever they want with IF > cleared. > - when a userland handler is registers, queue_intr masks the irq. > - interrupt.S lowers spl with splx_cli, thus IF still cleared > - iret, that clears the IF > > - later on, userland acks the IRQ, that unmasks the irq > > The key to this change is that all interrupts, including IPIs, are > treated the same way. Eg. the spl level is now raised before an IPI and > restored after. Also, EOI is not needed inside irq_acknowledge. > > With this change and the experimental change not to dispatch threads > direct to idle processors in the scheduler, I no longer observe kernel > faults, but an occasional hang does occur. > > --- > device/intr.c | 16 ++++++++++++++-- > i386/i386at/interrupt.S | 28 +++++++++++++--------------- > x86_64/interrupt.S | 28 +++++++++++++--------------- > 3 files changed, 40 insertions(+), 32 deletions(-) > > diff --git a/device/intr.c b/device/intr.c > index 15029440..97cfa377 100644 > --- a/device/intr.c > +++ b/device/intr.c > @@ -50,6 +50,20 @@ search_intr (struct irqdev *dev, ipc_port_t dst_port) > return NULL; > } > > + > +/* > + * Interrupt handling logic: > + * > + * interrupt.S raises spl (thus IF cleared) > + * interrupt.S EOI > + * interrupt.S calls the handler > + * - for pure in-kernel handlers, they do whatever they want with IF > cleared. > + * - when a userland handler is registered, queue_intr masks the irq. > + * interrupt.S lowers spl with splx_cli, thus IF still cleared > + * iret, that also clears the IF > + * > + * later on, (irq_acknowledge), userland acks the IRQ, that unmasks the irq > + */ > kern_return_t > irq_acknowledge (ipc_port_t receive_port) > { > @@ -76,8 +90,6 @@ irq_acknowledge (ipc_port_t receive_port) > if (ret) > return ret; > > - (*(irqtab.irqdev_ack)) (&irqtab, e->id); > - > __enable_irq (irqtab.irq[e->id]); > > return D_SUCCESS; > diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S > index 8ae6b97c..77424b43 100644 > --- a/i386/i386at/interrupt.S > +++ b/i386/i386at/interrupt.S > @@ -45,14 +45,6 @@ ENTRY(interrupt) > ret /* if so, just return */ > 1: > #endif > -#if NCPUS > 1 > - cmpl $CALL_PMAP_UPDATE,%eax /* was this a SMP pmap_update request? > */ > - je _call_single > - > - cmpl $CALL_AST_CHECK,%eax /* was this a SMP remote -> local ast > request? */ > - je _call_local_ast > -#endif > - > subl $24,%esp /* Two local variables + 4 parameters */ > movl %eax,S_IRQ /* save irq number */ > > @@ -61,6 +53,14 @@ ENTRY(interrupt) > > movl S_IRQ,%ecx /* restore irq number */ > > +#if NCPUS > 1 > + cmpl $CALL_PMAP_UPDATE,%ecx /* was this a SMP pmap_update request? > */ > + je _call_single > + > + cmpl $CALL_AST_CHECK,%ecx /* was this a SMP remote -> local ast > request? */ > + je _call_local_ast > +#endif > + > #ifndef APIC > movl $1,%eax > shll %cl,%eax /* get corresponding IRQ mask */ > @@ -98,11 +98,8 @@ 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,(%esp) /* load irq number as 1st arg */ > call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */ > -_no_eoi: > #endif > > movl S_IPL,%eax > @@ -122,6 +119,7 @@ _no_eoi: > > call *EXT(ivect)(%eax) /* call interrupt handler */ > > +_completed: > movl S_IPL,%eax /* restore previous ipl */ > movl %eax,(%esp) > call splx_cli /* restore previous ipl */ > @@ -131,14 +129,14 @@ _no_eoi: > > #if NCPUS > 1 > _call_single: > - cli /* no nested interrupts */ > call EXT(lapic_eoi) /* lapic EOI before the handler to > allow extra update */ > call EXT(pmap_update_interrupt) > - ret > + jmp _completed > > _call_local_ast: > - call EXT(ast_check) /* AST check on this cpu */ > call EXT(lapic_eoi) /* lapic EOI */ > - ret > + call EXT(ast_check) /* AST check on this cpu */ > + jmp _completed > + > #endif > END(interrupt) > diff --git a/x86_64/interrupt.S b/x86_64/interrupt.S > index fcd5a032..6fb77727 100644 > --- a/x86_64/interrupt.S > +++ b/x86_64/interrupt.S > @@ -45,14 +45,6 @@ ENTRY(interrupt) > ret /* if so, just return */ > 1: > #endif > -#if NCPUS > 1 > - cmpl $CALL_PMAP_UPDATE,%eax /* was this a SMP pmap_update request? > */ > - je _call_single > - > - cmpl $CALL_AST_CHECK,%eax /* was this a SMP remote -> local ast > request? */ > - je _call_local_ast > -#endif > - > subq $16,%rsp /* Two local variables */ > movl %eax,S_IRQ /* save irq number */ > > @@ -61,6 +53,14 @@ ENTRY(interrupt) > > movl S_IRQ,%ecx /* restore irq number */ > > +#if NCPUS > 1 > + cmpl $CALL_PMAP_UPDATE,%ecx /* was this a SMP pmap_update request? > */ > + je _call_single > + > + cmpl $CALL_AST_CHECK,%ecx /* was this a SMP remote -> local ast > request? */ > + je _call_local_ast > +#endif > + > #ifndef APIC > movl $1,%eax > shll %cl,%eax /* get corresponding IRQ mask */ > @@ -98,11 +98,8 @@ 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 */ > call EXT(ioapic_irq_eoi) /* ioapic irq specific EOI */ > -_no_eoi: > #endif > > ; > @@ -121,6 +118,7 @@ _no_eoi: > shll $1,%eax /* irq * 8 */ > call *EXT(ivect)(%rax) /* call interrupt handler */ > > +_completed: > movl S_IPL,%edi /* restore previous ipl */ > call splx_cli /* restore previous ipl */ > > @@ -129,14 +127,14 @@ _no_eoi: > > #if NCPUS > 1 > _call_single: > - cli /* no nested interrupts */ > call EXT(lapic_eoi) /* lapic EOI before the handler to > allow extra update */ > call EXT(pmap_update_interrupt) > - ret > + jmp _completed > > _call_local_ast: > - call EXT(ast_check) /* AST check on this cpu */ > call EXT(lapic_eoi) /* lapic EOI */ > - ret > + call EXT(ast_check) /* AST check on this cpu */ > + jmp _completed > + > #endif > END(interrupt) > -- > 2.40.1 > > >
-- Samuel --- Pour une évaluation indépendante, transparente et rigoureuse ! Je soutiens la Commission d'Évaluation de l'Inria.