Index | Thread | Search

From:
hshoexer <hshoexer@yerbouti.franken.de>
Subject:
Re: un-ifdef i8259 on amd64
To:
tech@openbsd.org
Date:
Thu, 15 Jan 2026 12:52:32 +0100

Download raw body.

Thread
On Thu, Jan 15, 2026 at 11:05:53AM +0100, Stefan Fritsch wrote:
> Hi,
> 
> I don't think we need different code variants for the legacy PIC on amd64. 
> It just makes the code difficult to read.  Just keep the default variant 
> and remove lots of #ifdefs
> 
> always defined:
>         ICU_HARDWARE_MASK
> 
> never defined:
>         ICU_SPECIAL_MASK_MODE
>         AUTO_EOI_1
>         AUTO_EOI_2
>         PIC_MASKDELAY
>         MASKDELAY
>         REORDER_IRQ
> 
> ok?

Looks good to me.  I cleaned up that code some time ago and came
up with the same diff.

ok hshoexer

> 
> Cheers,
> Stefan
> 
> diff --git a/sys/arch/amd64/amd64/i8259.c b/sys/arch/amd64/amd64/i8259.c
> index d6fa0e3d7f3..c31df0a16f1 100644
> --- a/sys/arch/amd64/amd64/i8259.c
> +++ b/sys/arch/amd64/amd64/i8259.c
> @@ -105,78 +105,59 @@ struct pic i8259_pic = {
>  	i8259_stubs,
>  };
>  
>  void
>  i8259_default_setup(void)
>  {
>  	outb(IO_ICU1, 0x11);		/* reset; program device, four bytes */
>  
>  	outb(IO_ICU1+1, ICU_OFFSET);	/* starting at this vector index */
>  	outb(IO_ICU1+1, 1 << IRQ_SLAVE); /* slave on line 2 */
> -#ifdef AUTO_EOI_1
> -	outb(IO_ICU1+1, 2 | 1);		/* auto EOI, 8086 mode */
> -#else
>  	outb(IO_ICU1+1, 1);		/* 8086 mode */
> -#endif
>  	outb(IO_ICU1+1, 0xff);		/* leave interrupts masked */
>  	outb(IO_ICU1, 0x68);		/* special mask mode (if available) */
>  	outb(IO_ICU1, 0x0a);		/* Read IRR by default. */
> -#ifdef REORDER_IRQ
> -	outb(IO_ICU1, 0xc0 | (3 - 1));	/* pri order 3-7, 0-2 (com2 first) */
> -#endif
> -
>  	outb(IO_ICU2, 0x11);		/* reset; program device, four bytes */
> -
>  	outb(IO_ICU2+1, ICU_OFFSET+8);	/* staring at this vector index */
>  	outb(IO_ICU2+1, IRQ_SLAVE);
> -#ifdef AUTO_EOI_2
> -	outb(IO_ICU2+1, 2 | 1);		/* auto EOI, 8086 mode */
> -#else
>  	outb(IO_ICU2+1, 1);		/* 8086 mode */
> -#endif
>  	outb(IO_ICU2+1, 0xff);		/* leave interrupts masked */
>  	outb(IO_ICU2, 0x68);		/* special mask mode (if available) */
>  	outb(IO_ICU2, 0x0a);		/* Read IRR by default. */
>  }
>  
>  static void
>  i8259_hwmask(struct pic *pic, int pin)
>  {
>  	unsigned port;
>  	u_int8_t byte;
>  
>  	i8259_imen |= (1 << pin);
> -#ifdef PIC_MASKDELAY
> -	delay(10);
> -#endif
>  	if (pin > 7) {
>  		port = IO_ICU2 + 1;
>  		byte = i8259_imen >> 8;
>  	} else {
>  		port = IO_ICU1 + 1;
>  		byte = i8259_imen & 0xff;
>  	}
>  	outb(port, byte);
>  }
>  
>  static void
>  i8259_hwunmask(struct pic *pic, int pin)
>  {
>  	unsigned port;
>  	u_int8_t byte;
>  	u_long s;
>  
>  	s = intr_disable();
>  	i8259_imen &= ~(1 << pin);
> -#ifdef PIC_MASKDELAY
> -	delay(10);
> -#endif
>  	if (pin > 7) {
>  		port = IO_ICU2 + 1;
>  		byte = i8259_imen >> 8;
>  	} else {
>  		port = IO_ICU1 + 1;
>  		byte = i8259_imen & 0xff;
>  	}
>  	outb(port, byte);
>  	intr_restore(s);
>  }
> diff --git a/sys/arch/amd64/include/i8259.h b/sys/arch/amd64/include/i8259.h
> index f4c94e0e61f..9def85e1aca 100644
> --- a/sys/arch/amd64/include/i8259.h
> +++ b/sys/arch/amd64/include/i8259.h
> @@ -56,89 +56,37 @@ extern void i8259_default_setup(void);
>   */
>  #define	IRQ_SLAVE	2
>  
>  /*
>   * Interrupt Control offset into Interrupt descriptor table (IDT)
>   */
>  #define	ICU_OFFSET	32		/* 0-31 are processor exceptions */
>  #define	ICU_LEN		16		/* 32-47 are ISA interrupts */
>  
>  
> -#define ICU_HARDWARE_MASK
> -
> -/*
> - * These macros are fairly self explanatory.  If ICU_SPECIAL_MASK_MODE is
> - * defined, we try to take advantage of the ICU's `special mask mode' by only
> - * EOIing the interrupts on return.  This avoids the requirement of masking and
> - * unmasking.  We can't do this without special mask mode, because the ICU
> - * would also hold interrupts that it thinks are of lower priority.
> - *
> - * Many machines do not support special mask mode, so by default we don't try
> - * to use it.
> - */
> -
>  #define	IRQ_BIT(num)	(1 << ((num) % 8))
>  #define	IRQ_BYTE(num)	((num) >> 3)
>  
>  #define i8259_late_ack(num)
>  
> -#ifdef ICU_SPECIAL_MASK_MODE
> -
> -#define	i8259_asm_ack1(num)
> -#define	i8259_asm_ack2(num) \
> -	movb	$(0x60|IRQ_SLAVE),%al	/* specific EOI for IRQ2 */	;\
> -	outb	%al,$IO_ICU1
> -#define	i8259_asm_mask(num)
> -#define	i8259_asm_unmask(num) \
> -	movb	$(0x60|(num%8)),%al	/* specific EOI */		;\
> -	outb	%al,$ICUADDR
> -
> -#else /* ICU_SPECIAL_MASK_MODE */
> -
> -#ifndef	AUTO_EOI_1
>  #define	i8259_asm_ack1(num) \
>  	movb	$(0x60|(num%8)),%al	/* specific EOI */		;\
>  	outb	%al,$IO_ICU1
> -#else
> -#define	i8259_asm_ack1(num)
> -#endif
>  
> -#ifndef AUTO_EOI_2
>  #define	i8259_asm_ack2(num) \
>  	movb	$(0x60|(num%8)),%al	/* specific EOI */		;\
>  	outb	%al,$IO_ICU2		/* do the second ICU first */	;\
>  	movb	$(0x60|IRQ_SLAVE),%al	/* specific EOI for IRQ2 */	;\
>  	outb	%al,$IO_ICU1
> -#else
> -#define	i8259_asm_ack2(num)
> -#endif
> -
> -#ifdef PIC_MASKDELAY
> -#define MASKDELAY	pushl %eax ; inb $0x84,%al ; popl %eax
> -#else
> -#define MASKDELAY
> -#endif
> -
> -#ifdef ICU_HARDWARE_MASK
>  
>  #define	i8259_asm_mask(num) \
>  	movb	CVAROFF(i8259_imen, IRQ_BYTE(num)),%al			;\
>  	orb	$IRQ_BIT(num),%al					;\
>  	movb	%al,CVAROFF(i8259_imen, IRQ_BYTE(num))			;\
> -	MASKDELAY							;\
>  	outb	%al,$(ICUADDR+1)
>  #define	i8259_asm_unmask(num) \
>  	movb	CVAROFF(i8259_imen, IRQ_BYTE(num)),%al			;\
>  	andb	$~IRQ_BIT(num),%al					;\
>  	movb	%al,CVAROFF(i8259_imen, IRQ_BYTE(num))			;\
> -	MASKDELAY							;\
>  	outb	%al,$(ICUADDR+1)
>  
> -#else /* ICU_HARDWARE_MASK */
> -
> -#define	i8259_asm_mask(num)
> -#define	i8259_asm_unmask(num)
> -
> -#endif /* ICU_HARDWARE_MASK */
> -#endif /* ICU_SPECIAL_MASK_MODE */
> -
>  #endif /* !_MACHINE_I8259_H_ */
>