From: hshoexer Subject: Re: un-ifdef i8259 on amd64 To: tech@openbsd.org Date: Thu, 15 Jan 2026 12:52:32 +0100 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_ */ >