Download raw body.
ioapic(4) cleanup diff
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 have tested it with AMD CPU on hardware as host, on vmm/vmd as
guest, on KVM/qemu as guest, without SEV, with SEV, and SEV-ES.
bluhm
> 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 <machine/pte.h>
> include <machine/vmparam.h>
> include <machine/intr.h>
> include <machine/tss.h>
> -include <machine/i82093var.h>
>
> 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 <machine/bus.h>
> +
> 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 <machine/mpconfig.h>
>
> #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 <machine/bus.h>
> +
> 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;
ioapic(4) cleanup diff