From: Hans-Jörg Höxer Subject: Re: [EXT] ioapic(4) cleanup diff To: Date: Tue, 2 Sep 2025 14:26:09 +0200 On Sat, Aug 30, 2025 at 09:25:27PM +0200, Mark Kettenis wrote: > The current code takes some shortcuts and doesn't use the bus_space(9) > APIs. This diff fixes that. In order to do so, it replaces some > assembly code with a call to C functions. Those C functions do the > locking that the asm code skips. So this could use some testing. > Especially on somewhat older hardware that doesn't use MSIs for most > of its interrupts. I've tested this on host (AMD cpu) and virtualized (no-SEV, SEV, SEV-ES) on vmm/vmd. diff ok hshoexer > Index: arch/amd64/amd64/genassym.cf > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/genassym.cf,v > diff -u -p -r1.47 genassym.cf > --- arch/amd64/amd64/genassym.cf 10 Jul 2025 13:59:27 -0000 1.47 > +++ arch/amd64/amd64/genassym.cf 28 Aug 2025 19:29:41 -0000 > @@ -12,7 +12,6 @@ include > include > include > include > -include > > export SONPROC > > @@ -140,10 +139,6 @@ member ih_arg > member ih_next > member ih_level > member IH_COUNT ih_count.ec_count > - > -struct ioapic_softc > -member IOAPIC_SC_REG sc_reg > -member IOAPIC_SC_DATA sc_data > > # pte fields > export PG_V > Index: arch/amd64/amd64/ioapic.c > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/ioapic.c,v > diff -u -p -r1.32 ioapic.c > --- arch/amd64/amd64/ioapic.c 29 Apr 2023 10:18:06 -0000 1.32 > +++ arch/amd64/amd64/ioapic.c 28 Aug 2025 19:29:41 -0000 > @@ -86,9 +86,6 @@ int ioapic_match(struct device *, vo > void ioapic_attach(struct device *, struct device *, void *); > int ioapic_activate(struct device *, int); > > -extern int x86_mem_add_mapping(bus_addr_t, bus_size_t, > - int, bus_space_handle_t *); /* XXX XXX */ > - > void ioapic_hwmask(struct pic *, int); > void ioapic_hwunmask(struct pic *, int); > void ioapic_addroute(struct pic *, struct cpu_info *, int, int, int); > @@ -131,19 +128,15 @@ ioapic_unlock(struct ioapic_softc *sc, u > static __inline u_int32_t > ioapic_read_ul(struct ioapic_softc *sc,int regid) > { > - u_int32_t val; > - > - *(sc->sc_reg) = regid; > - val = *sc->sc_data; > - > - return (val); > + bus_space_write_4(sc->sc_memt, sc->sc_memh, IOAPIC_REG, regid); > + return bus_space_read_4(sc->sc_memt, sc->sc_memh, IOAPIC_DATA); > } > > static __inline void > ioapic_write_ul(struct ioapic_softc *sc,int regid, u_int32_t val) > { > - *(sc->sc_reg) = regid; > - *(sc->sc_data) = val; > + bus_space_write_4(sc->sc_memt, sc->sc_memh, IOAPIC_REG, regid); > + bus_space_write_4(sc->sc_memt, sc->sc_memh, IOAPIC_DATA, val); > } > > static __inline u_int32_t > @@ -264,16 +257,12 @@ ioapic_set_id(struct ioapic_softc *sc) > printf(", remapped"); > } > > -/* > - * can't use bus_space_xxx as we don't have a bus handle ... > - */ > void > ioapic_attach(struct device *parent, struct device *self, void *aux) > { > struct ioapic_softc *sc = (struct ioapic_softc *)self; > struct apic_attach_args *aaa = (struct apic_attach_args *)aux; > int apic_id; > - bus_space_handle_t bh; > u_int32_t ver_sz; > int i; > > @@ -291,12 +280,12 @@ ioapic_attach(struct device *parent, str > > printf(" pa 0x%lx", aaa->apic_address); > > - if (x86_mem_add_mapping(aaa->apic_address, PAGE_SIZE, 0, &bh) != 0) { > + sc->sc_memt = aaa->apic_memt; > + if (bus_space_map(sc->sc_memt, aaa->apic_address, PAGE_SIZE, 0, > + &sc->sc_memh)) { > printf(", map failed\n"); > return; > } > - sc->sc_reg = (volatile u_int32_t *)(bh + IOAPIC_REG); > - sc->sc_data = (volatile u_int32_t *)(bh + IOAPIC_DATA); > > sc->sc_pic.pic_type = PIC_IOAPIC; > #ifdef MULTIPROCESSOR > Index: arch/amd64/amd64/mpbios.c > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/mpbios.c,v > diff -u -p -r1.33 mpbios.c > --- arch/amd64/amd64/mpbios.c 22 Oct 2024 21:50:02 -0000 1.33 > +++ arch/amd64/amd64/mpbios.c 28 Aug 2025 19:29:41 -0000 > @@ -142,10 +142,16 @@ struct mp_map { > int psize; > }; > > +struct mpbios_softc { > + struct device sc_dev; > + bus_space_tag_t sc_memt; > +}; > + > int mp_print(void *, const char *); > int mp_match(struct device *, void *, void *); > const void *mpbios_search(struct device *, paddr_t, int, struct mp_map *); > static __inline int mpbios_cksum(const void *, int); > +void mpbios_scan(struct mpbios_softc *); > > void mp_cfg_special_intr(const struct mpbios_int *, u_int32_t *); > void mp_print_special_intr(int); > @@ -161,9 +167,9 @@ void mp_print_eisa_intr(int); > void mp_cfg_isa_intr(const struct mpbios_int *, u_int32_t *); > void mp_print_isa_intr(int); > > -void mpbios_cpu(const u_int8_t *, struct device *); > -void mpbios_bus(const u_int8_t *, struct device *); > -void mpbios_ioapic(const u_int8_t *, struct device *); > +void mpbios_cpu(struct mpbios_softc *, const u_int8_t *); > +void mpbios_bus(struct mpbios_softc *, const u_int8_t *); > +void mpbios_ioapic(struct mpbios_softc *, const u_int8_t *); > int mpbios_int(const u_int8_t *, int, struct mp_intr_map *); > > const void *mpbios_map(paddr_t, int, struct mp_map *); > @@ -184,7 +190,7 @@ int mpbios_match(struct device *, void * > void mpbios_attach(struct device *, struct device *, void *); > > const struct cfattach mpbios_ca = { > - sizeof(struct device), mpbios_match, mpbios_attach > + sizeof(struct mpbios_softc), mpbios_match, mpbios_attach > }; > > struct cfdriver mpbios_cd = { > @@ -195,9 +201,9 @@ int > mpbios_match(struct device *parent, void *match, void *aux) > { > struct cfdata *cf = match; > - struct bios_attach_args *bia = aux; > + struct bios_attach_args *ba = aux; > > - if (strcmp(bia->ba_name, cf->cf_driver->cd_name) == 0) > + if (strcmp(ba->ba_name, cf->cf_driver->cd_name) == 0) > return (1); > return (0); > } > @@ -205,7 +211,11 @@ mpbios_match(struct device *parent, void > void > mpbios_attach(struct device *parent, struct device *self, void *aux) > { > - mpbios_scan(self); > + struct mpbios_softc *sc = (struct mpbios_softc *)self; > + struct bios_attach_args *ba = aux; > + > + sc->sc_memt = ba->ba_memt; > + mpbios_scan(sc); > } > > int > @@ -488,7 +498,7 @@ static struct mp_bus nmi_bus = { > * nintrs > */ > void > -mpbios_scan(struct device *self) > +mpbios_scan(struct mpbios_softc *sc) > { > const u_int8_t *position, *end; > int count; > @@ -497,7 +507,7 @@ mpbios_scan(struct device *self) > paddr_t lapic_base; > const struct mpbios_int *iep; > struct mpbios_int ie; > - struct ioapic_softc *sc; > + struct ioapic_softc *ioapic; > > printf(": Intel MP Specification 1.%d\n", mp_fps->spec_rev); > > @@ -528,7 +538,7 @@ mpbios_scan(struct device *self) > if (type >= MPS_MCT_NTYPES) { > printf("%s: unknown entry type %x" > " in MP config table\n", > - self->dv_xname, type); > + sc->sc_dev.dv_xname, type); > break; > } > mp_conf[type].count++; > @@ -568,21 +578,21 @@ mpbios_scan(struct device *self) > while ((count--) && (position < end)) { > switch (type = *(u_char *)position) { > case MPS_MCT_CPU: > - mpbios_cpu(position, self); > + mpbios_cpu(sc, position); > break; > case MPS_MCT_BUS: > - mpbios_bus(position, self); > + mpbios_bus(sc, position); > break; > case MPS_MCT_IOAPIC: > - mpbios_ioapic(position, self); > + mpbios_ioapic(sc, position); > break; > case MPS_MCT_IOINT: > iep = (const struct mpbios_int *)position; > ie = *iep; > if (iep->dst_apic_id == MPS_ALL_APICS) { > - for (sc = ioapics ; sc != NULL; > - sc = sc->sc_next) { > - ie.dst_apic_id = sc->sc_apicid; > + for (ioapic = ioapics ; ioapic != NULL; > + ioapic = ioapic->sc_next) { > + ie.dst_apic_id = ioapic->sc_apicid; > if (mpbios_int((char *)&ie, > type, &mp_intrs[cur_intr]) == 0) > cur_intr++; > @@ -601,7 +611,7 @@ mpbios_scan(struct device *self) > default: > printf("%s: unknown entry type %x " > "in MP config table\n", > - self->dv_xname, type); > + sc->sc_dev.dv_xname, type); > /* NOTREACHED */ > return; > } > @@ -613,7 +623,7 @@ mpbios_scan(struct device *self) > if (mp_verbose && mp_cth->ext_len) > printf("%s: MP WARNING: %d " > "bytes of extended entries not examined\n", > - self->dv_xname, mp_cth->ext_len); > + sc->sc_dev.dv_xname, mp_cth->ext_len); > > /* Clean up. */ > mp_fps = NULL; > @@ -630,10 +640,10 @@ mpbios_scan(struct device *self) > } > > void > -mpbios_cpu(const u_int8_t *ent, struct device *self) > +mpbios_cpu(struct mpbios_softc *sc, const u_int8_t *ent) > { > const struct mpbios_proc *entry = (const struct mpbios_proc *)ent; > - struct device *mainbus = self->dv_parent->dv_parent; > + struct device *mainbus = sc->sc_dev.dv_parent->dv_parent; > struct cpu_attach_args caa; > > /* XXX move this into the CPU attachment goo. */ > @@ -856,12 +866,12 @@ mp_print_eisa_intr(int intr) > #endif > > void > -mpbios_bus(const u_int8_t *ent, struct device *self) > +mpbios_bus(struct mpbios_softc *sc, const u_int8_t *ent) > { > const struct mpbios_bus *entry = (const struct mpbios_bus *)ent; > int bus_id = entry->bus_id; > > - printf("%s: bus %d is type %6.6s\n", self->dv_xname, > + printf("%s: bus %d is type %6.6s\n", sc->sc_dev.dv_xname, > bus_id, entry->bus_type); > > #ifdef DIAGNOSTIC > @@ -871,7 +881,7 @@ mpbios_bus(const u_int8_t *ent, struct d > */ > if (bus_id >= mp_nbusses) { > panic("%s: bus number %d out of range?? (type %6.6s)", > - self->dv_xname, bus_id, entry->bus_type); > + sc->sc_dev.dv_xname, bus_id, entry->bus_type); > } > #endif > > @@ -893,7 +903,7 @@ mpbios_bus(const u_int8_t *ent, struct d > > if (mp_eisa_bus) > printf("%s: multiple eisa busses?\n", > - self->dv_xname); > + sc->sc_dev.dv_xname); > else > mp_eisa_bus = &mp_busses[bus_id]; > #endif > @@ -904,21 +914,21 @@ mpbios_bus(const u_int8_t *ent, struct d > mp_busses[bus_id].mb_intr_cfg = mp_cfg_isa_intr; > if (mp_isa_bus) > printf("%s: multiple isa busses?\n", > - self->dv_xname); > + sc->sc_dev.dv_xname); > else > mp_isa_bus = &mp_busses[bus_id]; > } else { > - printf("%s: unsupported bus type %6.6s\n", self->dv_xname, > + printf("%s: unsupported bus type %6.6s\n", sc->sc_dev.dv_xname, > entry->bus_type); > } > } > > > void > -mpbios_ioapic(const u_int8_t *ent, struct device *self) > +mpbios_ioapic(struct mpbios_softc *sc, const u_int8_t *ent) > { > const struct mpbios_ioapic *entry = (const struct mpbios_ioapic *)ent; > - struct device *mainbus = self->dv_parent->dv_parent; > + struct device *mainbus = sc->sc_dev.dv_parent->dv_parent; > struct apic_attach_args aaa; > > /* XXX let flags checking happen in ioapic driver.. */ > @@ -926,9 +936,10 @@ mpbios_ioapic(const u_int8_t *ent, struc > return; > > aaa.aaa_name = "ioapic"; > + aaa.apic_memt = sc->sc_memt; > aaa.apic_id = entry->apic_id; > aaa.apic_version = entry->apic_version; > - aaa.apic_address = (paddr_t)entry->apic_address; > + aaa.apic_address = entry->apic_address; > aaa.apic_vecbase = -1; > aaa.flags = (mp_fps->mpfb2 & 0x80) ? IOAPIC_PICMODE : IOAPIC_VWIRE; > > Index: arch/amd64/include/apicvar.h > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/include/apicvar.h,v > diff -u -p -r1.3 apicvar.h > --- arch/amd64/include/apicvar.h 23 Mar 2011 16:54:34 -0000 1.3 > +++ arch/amd64/include/apicvar.h 28 Aug 2025 19:29:41 -0000 > @@ -35,6 +35,8 @@ > #ifndef _MACHINE_APICVAR_H_ > #define _MACHINE_APICVAR_H_ > > +#include > + > struct apic_attach_args { > const char *aaa_name; > int apic_id; > @@ -42,7 +44,8 @@ struct apic_attach_args { > int flags; > #define IOAPIC_PICMODE 0x01 > #define IOAPIC_VWIRE 0x02 > - paddr_t apic_address; > + bus_space_tag_t apic_memt; > + bus_addr_t apic_address; > int apic_vecbase; > }; > > Index: arch/amd64/include/i82093reg.h > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/include/i82093reg.h,v > diff -u -p -r1.8 i82093reg.h > --- arch/amd64/include/i82093reg.h 1 Dec 2022 00:26:15 -0000 1.8 > +++ arch/amd64/include/i82093reg.h 28 Aug 2025 19:29:41 -0000 > @@ -119,62 +119,17 @@ > movl $0,(local_apic+LAPIC_EOI)(%rip) ;\ > CODEPATCH_END(CPTAG_EOI) > > - > -#ifdef MULTIPROCESSOR > - > -#ifdef notyet > -#define ioapic_asm_lock(num) \ > - movl $1,%esi ;\ > -77: \ > - xchgl %esi,PIC_LOCK(%rdi) ;\ > - testl %esi,%esi ;\ > - jne 77b > - > -#define ioapic_asm_unlock(num) \ > - movl $0,PIC_LOCK(%rdi) > -#else > -#define ioapic_asm_lock(num) > -#define ioapic_asm_unlock(num) > -#endif > - > -#else > - > -#define ioapic_asm_lock(num) > -#define ioapic_asm_unlock(num) > - > -#endif /* MULTIPROCESSOR */ > - > - > #define ioapic_mask(num) \ > movq IS_PIC(%r14),%rdi ;\ > - ioapic_asm_lock(num) ;\ > movl IS_PIN(%r14),%esi ;\ > - leaq 0x10(%rsi,%rsi,1),%rsi ;\ > - movq IOAPIC_SC_REG(%rdi),%r15 ;\ > - movl %esi, (%r15) ;\ > - movq IOAPIC_SC_DATA(%rdi),%r15 ;\ > - movl (%r15),%esi ;\ > - orl $IOAPIC_REDLO_MASK,%esi ;\ > - andl $~IOAPIC_REDLO_RIRR,%esi ;\ > - movl %esi,(%r15) ;\ > - ioapic_asm_unlock(num) > + call ioapic_hwmask > > #define ioapic_unmask(num) \ > cmpq $IREENT_MAGIC,IF_ERR(%rsp) ;\ > jne 79f ;\ > movq IS_PIC(%r14),%rdi ;\ > - ioapic_asm_lock(num) ;\ > movl IS_PIN(%r14),%esi ;\ > - leaq 0x10(%rsi,%rsi,1),%rsi ;\ > - movq IOAPIC_SC_REG(%rdi),%r15 ;\ > - movq IOAPIC_SC_DATA(%rdi),%r13 ;\ > - movl %esi, (%r15) ;\ > - movl (%r13),%r12d ;\ > - andl $~IOAPIC_REDLO_MASK,%r12d ;\ > - andl $~IOAPIC_REDLO_RIRR,%r12d ;\ > - movl %esi,(%r15) ;\ > - movl %r12d,(%r13) ;\ > - ioapic_asm_unlock(num) ;\ > + call ioapic_hwunmask ;\ > 79: > > #endif > Index: arch/amd64/include/i82093var.h > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/include/i82093var.h,v > diff -u -p -r1.7 i82093var.h > --- arch/amd64/include/i82093var.h 22 Oct 2024 21:50:02 -0000 1.7 > +++ arch/amd64/include/i82093var.h 28 Aug 2025 19:29:41 -0000 > @@ -53,9 +53,8 @@ struct ioapic_softc { > int sc_apic_vecbase; /* global int base if ACPI */ > int sc_apic_sz; /* apic size*/ > int sc_flags; > - paddr_t sc_pa; /* PA of ioapic */ > - volatile u_int32_t *sc_reg; /* KVA of ioapic addr */ > - volatile u_int32_t *sc_data; /* KVA of ioapic data */ > + bus_space_tag_t sc_memt; > + bus_space_handle_t sc_memh; > struct ioapic_pin *sc_pins; /* sc_apic_sz entries */ > }; > > Index: arch/amd64/include/mpbiosvar.h > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/include/mpbiosvar.h,v > diff -u -p -r1.6 mpbiosvar.h > --- arch/amd64/include/mpbiosvar.h 22 Nov 2014 18:31:46 -0000 1.6 > +++ arch/amd64/include/mpbiosvar.h 28 Aug 2025 19:29:41 -0000 > @@ -45,7 +45,6 @@ > #include > > #if defined(_KERNEL) > -void mpbios_scan(struct device *); > int mpbios_probe(struct device *); > > void mpbios_intr_fixup(void); > Index: arch/i386/i386/ioapic.c > =================================================================== > RCS file: /cvs/src/sys/arch/i386/i386/ioapic.c,v > diff -u -p -r1.44 ioapic.c > --- arch/i386/i386/ioapic.c 30 Jan 2023 10:49:05 -0000 1.44 > +++ arch/i386/i386/ioapic.c 28 Aug 2025 19:29:42 -0000 > @@ -294,7 +294,7 @@ ioapic_attach(struct device *parent, str > > ioapic_add(sc); > > - printf(" pa 0x%x", aaa->apic_address); > + printf(" pa 0x%lx", aaa->apic_address); > > if (bus_mem_add_mapping(aaa->apic_address, PAGE_SIZE, 0, &bh) != 0) { > printf(", map failed\n"); > Index: arch/i386/include/apicvar.h > =================================================================== > RCS file: /cvs/src/sys/arch/i386/include/apicvar.h,v > diff -u -p -r1.6 apicvar.h > --- arch/i386/include/apicvar.h 23 Mar 2011 16:54:35 -0000 1.6 > +++ arch/i386/include/apicvar.h 28 Aug 2025 19:29:42 -0000 > @@ -35,6 +35,8 @@ > #ifndef _MACHINE_APICVAR_H_ > #define _MACHINE_APICVAR_H_ > > +#include > + > struct apic_attach_args { > const char *aaa_name; > int apic_id; > @@ -42,7 +44,8 @@ struct apic_attach_args { > int flags; > #define IOAPIC_PICMODE 0x01 > #define IOAPIC_VWIRE 0x02 > - u_int32_t apic_address; > + bus_space_tag_t apic_memt; > + bus_addr_t apic_address; > int apic_vecbase; > }; > > Index: dev/acpi/acpimadt.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpimadt.c,v > diff -u -p -r1.39 acpimadt.c > --- dev/acpi/acpimadt.c 24 Nov 2022 04:04:39 -0000 1.39 > +++ dev/acpi/acpimadt.c 28 Aug 2025 19:29:42 -0000 > @@ -286,6 +286,7 @@ acpimadt_attach(struct device *parent, s > > memset(&aaa, 0, sizeof(struct apic_attach_args)); > aaa.aaa_name = "ioapic"; > + aaa.apic_memt = acpi_sc->sc_memt; > aaa.apic_id = entry->madt_ioapic.acpi_ioapic_id; > aaa.apic_address = entry->madt_ioapic.address; > aaa.apic_vecbase = entry->madt_ioapic.global_int_base; >