Re: [PATCH gnumach] interrupt: Mask, eoi, unmask

[Samuel Thibault]

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