Re: [PATCH]: gnumach - simplify interrupt handling

[Samuel Thibault]

Hello,

Damien Zammit, le sam. 09 nov. 2019 15:46:33 +1100, a ecrit:
> The first patch combines all spls 1-7 into a single spl that disables all 
> interrupts.
> It boots with just this patch.

I have applied this one, thanks!

> The second patch removes all concept of a cached PIC mask, and allows the PIC 
> to remain
> unmasked for all interrupts individually all the time (except when doing an 
> EOI).

Had you tried your remove-picmask patch with netdde? I'm getting a
hang at network configuration here, with the e1000 qemu nic model. The
gnumach dde patch makes use of mask_irq/unmask_irq, it probably needs to
be fixed to properly mask/unmask irqs for userland netdde. As mentioned
in the device.defs patch, the very presence of an device_intr_enable RPC
is actually odd. I don't think we want to let userland disable/enable
irqs and userland should rather manage its own interrupt notification
masking. That said, there is possibly irq acknowledgment which needs to
be done somehow. We need to sort that out, be it for netdde or rump.

> diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S
> index cdb385c..8a4cd73 100644
> --- a/i386/i386at/interrupt.S
> +++ b/i386/i386at/interrupt.S
> @@ -42,11 +42,18 @@ ENTRY(interrupt)
>       cli                             /* XXX no more nested interrupts */
>       popl    %eax                    /* restore irq number */
>       movl    %eax,%ecx               /* copy irq number */
> +     movb    $0xff,%al               /* mask for all interrupts */
> +     outb    %al,$(PIC_MASTER_OCW)   /* mask master out */
> +     outb    %al,$(PIC_SLAVE_OCW)    /* mask slave out */
> +     movl    %ecx,%eax               /* restore eax */
>       movb    $(NON_SPEC_EOI),%al     /* non-specific EOI */
>       outb    %al,$(PIC_MASTER_ICW)   /* ack interrupt to master */
>       cmpl    $8,%ecx                 /* do we need to ack slave? */
>       jl      1f                      /* no, skip it */
>       outb    %al,$(PIC_SLAVE_ICW)
>  1:
> +     movb    $0,%al                  /* empty mask */
> +     outb    %al,$(PIC_MASTER_OCW)   /* unmask master */
> +     outb    %al,$(PIC_SLAVE_OCW)    /* unmask slave */
>       ret                             /* return */

Is this really needed? Since we have the interrupt flag cleared, we
won't get interrupted anyway.

> diff --git a/linux/dev/drivers/block/ahci.c b/linux/dev/drivers/block/ahci.c
> index 3cb003c..59eb5bc 100644
> --- a/linux/dev/drivers/block/ahci.c
> +++ b/linux/dev/drivers/block/ahci.c

Please do not include unrelated changes :)

> diff --git a/linux/dev/include/asm-i386/system.h 
> b/linux/dev/include/asm-i386/system.h
> index f26a33e..41eb65a 100644
> --- a/linux/dev/include/asm-i386/system.h
> +++ b/linux/dev/include/asm-i386/system.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_SYSTEM_H
>  #define __ASM_SYSTEM_H
>  
> +#include <i386/ipl.h> /* curr_ipl, splx */
> +
>  #include <asm/segment.h>
>  
>  /*
> @@ -223,10 +225,8 @@ static inline unsigned long __xchg(unsigned long x, void 
> * ptr, int size)
>  #define mb()  __asm__ __volatile__ (""   : : :"memory")
>  #define __sti() __asm__ __volatile__ ("sti": : :"memory")
>  #define __cli() __asm__ __volatile__ ("cli": : :"memory")
> -#define __save_flags(x) \
> -__asm__ __volatile__("pushf ; pop %0" : "=r" (x): /* no input */ :"memory")
> -#define __restore_flags(x) \
> -__asm__ __volatile__("push %0 ; popf": /* no output */ :"g" (x):"memory")
> +#define __save_flags(x) (x = ((curr_ipl > 0) ? 0 : (1 << 9)))

Better move EFLAGS_IF_SHIFT definition here and use it here.

Samuel