Re: [PATCH gnumach] Fix interrupt handling

[Samuel Thibault]

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.