From: Patrick Wildt Subject: Re: agintc(4): clamp down to maximum possible deviceid To: Mark Kettenis Cc: tech@openbsd.org Date: Mon, 24 Jun 2024 00:13:02 +0200 On Sun, Jun 23, 2024 at 11:43:41PM +0200, Mark Kettenis wrote: > > Date: Sun, 23 Jun 2024 22:55:20 +0200 > > From: Patrick Wildt > > > > Hi, > > > > the Qualcomm Snapdragon X Elite (X1E80100) claims to support > > 32 device-id bits, but it also seems to only allow a 4k pagesize > > with 8 bytes per entry. Without support for INDIRECT (status quo) > > this yields a maximum deviceid of 0x1ffff, which is way less than the > > 32 devbits. Even with INDIRECT we would only get 0x3ffffff. > > > > Hence let's only allocate as much as possibly configurable, and bail out > > when someone tries to establish a device with it. > > > > With this establishing nvme(4) on the Lenovo Yoga Slim 7x is still being > > attempted, but will fail. It would be nicer if the MSI map failed, but > > with this the machine at least doesn't hang anymore on boot. > > If the hardware really generated deviceids that can't be handled, they > probably would have noticed that the hardware is broken. So I don't > think we need to worry about that. > > ok kettenis@ > > I still think we want INDIRECT support. That'll come after this? Oh, we definitely need it because NVMe's device-id is 0xe0000 which is out of scope without INDIRECT. Here's the diff for INDIRECT, but right now NVMe MSIs don't seem to work with this enabled... So either no NVMe w/o INDIRECT, or a hanging system w/ INDIRECT. Patrick diff --git a/sys/arch/arm64/dev/agintc.c b/sys/arch/arm64/dev/agintc.c index 86b8aefc384..53c4b37444f 100644 --- a/sys/arch/arm64/dev/agintc.c +++ b/sys/arch/arm64/dev/agintc.c @@ -1577,6 +1577,7 @@ struct agintc_msi_softc { struct agintc_dmamem *sc_dtt; size_t sc_dtt_pgsz; uint8_t sc_dte_sz; + int sc_dtt_indirect; int sc_cidbits; struct agintc_dmamem *sc_ctt; size_t sc_ctt_pgsz; @@ -1705,12 +1706,30 @@ agintc_msi_attach(struct device *parent, struct device *self, void *aux) size = (1ULL << sc->sc_devbits) * sc->sc_dte_sz; size = roundup(size, sc->sc_dtt_pgsz); + /* Might make sense to go indirect */ + if (size > 2 * sc->sc_dtt_pgsz) { + bus_space_write_8(sc->sc_iot, sc->sc_ioh, GITS_BASER(i), + baser | GITS_BASER_INDIRECT); + if (bus_space_read_8(sc->sc_iot, sc->sc_ioh, + GITS_BASER(i)) & GITS_BASER_INDIRECT) + sc->sc_dtt_indirect = 1; + } + if (sc->sc_dtt_indirect) { + size = (1ULL << sc->sc_devbits); + size /= (sc->sc_dtt_pgsz / sc->sc_dte_sz); + size *= sizeof(uint64_t); + size = roundup(size, sc->sc_dtt_pgsz); + } + /* Clamp down to maximum configurable num pages */ if (size / sc->sc_dtt_pgsz > GITS_BASER_SZ_MASK + 1) size = (GITS_BASER_SZ_MASK + 1) * sc->sc_dtt_pgsz; /* Calculate max deviceid based off configured size */ sc->sc_deviceid_max = (size / sc->sc_dte_sz) - 1; + if (sc->sc_dtt_indirect) + sc->sc_deviceid_max = ((size / sizeof(uint64_t)) * + (sc->sc_dtt_pgsz / sc->sc_dte_sz)) - 1; /* Allocate table. */ sc->sc_dtt = agintc_dmamem_alloc(sc->sc_dmat, @@ -1725,7 +1744,9 @@ agintc_msi_attach(struct device *parent, struct device *self, void *aux) KASSERT((dtt_pa & GITS_BASER_PA_MASK) == dtt_pa); bus_space_write_8(sc->sc_iot, sc->sc_ioh, GITS_BASER(i), GITS_BASER_IC_NORM_NC | baser & GITS_BASER_PGSZ_MASK | - dtt_pa | (size / sc->sc_dtt_pgsz) - 1 | GITS_BASER_VALID); + dtt_pa | (size / sc->sc_dtt_pgsz) - 1 | + (sc->sc_dtt_indirect ? GITS_BASER_INDIRECT : 0) | + GITS_BASER_VALID); } /* Set up collection translation table. */ @@ -1861,6 +1882,40 @@ agintc_msi_wait_cmd(struct agintc_msi_softc *sc) printf("%s: command queue timeout\n", sc->sc_dev.dv_xname); } +int +agintc_msi_create_device_table(struct agintc_msi_softc *sc, uint32_t deviceid) +{ + uint64_t *table = AGINTC_DMA_KVA(sc->sc_dtt); + uint32_t idx = deviceid / (sc->sc_dtt_pgsz / sc->sc_dte_sz); + struct agintc_dmamem *dtt; + paddr_t dtt_pa; + + /* Out of bounds */ + if (deviceid > sc->sc_deviceid_max) + return ENXIO; + + /* No need to adjust */ + if (!sc->sc_dtt_indirect) + return 0; + + /* Table already allocated */ + if (table[idx]) + return 0; + + /* FIXME: leaks */ + dtt = agintc_dmamem_alloc(sc->sc_dmat, + sc->sc_dtt_pgsz, sc->sc_dtt_pgsz); + if (dtt == NULL) + return ENOMEM; + + dtt_pa = AGINTC_DMA_DVA(dtt); + KASSERT((dtt_pa & GITS_BASER_PA_MASK) == dtt_pa); + table[idx] = dtt_pa | GITS_BASER_VALID; + cpu_dcache_wb_range((vaddr_t)&table[idx], sizeof(table[idx])); + __asm volatile("dsb sy"); + return 0; +} + struct agintc_msi_device * agintc_msi_create_device(struct agintc_msi_softc *sc, uint32_t deviceid) { @@ -1870,6 +1925,9 @@ agintc_msi_create_device(struct agintc_msi_softc *sc, uint32_t deviceid) if (deviceid > sc->sc_deviceid_max) return NULL; + if (agintc_msi_create_device_table(sc, deviceid) != 0) + return NULL; + md = malloc(sizeof(*md), M_DEVBUF, M_ZERO | M_WAITOK); md->md_deviceid = deviceid; md->md_itt = agintc_dmamem_alloc(sc->sc_dmat,