From: Patrick Wildt Subject: smmu(4) on QC Laptops To: tech@openbsd.org Cc: kettenis@openbsd.org, tobhe@openbsd.org, mlarkin@openbsd.org Date: Fri, 23 May 2025 08:46:16 +0200 Hi, Nearly three years ago we disabled smmu(4) for QC laptops because they just rebooted instantly. I've finally gone to the bottom of why this happens. On servers we usually boot up with most peripherals not doing any kind of DMA. A serial console is kind of the only thing we depend on, which usually doesn't use DMA for simplicity. Everything else, like NVMe or USB will get turned on later. For those use cases it's been perfectly fine to just turn the SMMU on with strict filtering, because only once the driver configures puts in a job DMA transactions will occur. On the QC laptops the framebuffer goes through an SMMU. That's one of the reasons we need to keep the streams alive and actively put them into a bypass context, which we do! As soon as a driver attaches, we move the stream over into a strict translation context. Unfortunately the changes I did were not completely sufficient. (1) The IOMMU enforcement happens before the driver gets a chance to stop current work. simplebus(4) creates an IOMMU-bound DMA tag before it attempts to attach the children. This leads do two interesting things: (a) If a simplebus(4) has an IOMMU assigned, it is passed an IOMMU-bound DMA tag. All children will get subjected to it. smmu1: establishing sid 0x423 simplebus1 at simplebus0: "geniqup" "serial" at simplebus1 not configured smmu1: establishing sid 0x123 simplebus2 at simplebus0: "geniqup" (b) If there's no driver to be found, the IOMMU-bound DMA tag is created anyway. smmu1: establishing sid 0x1c00 smmu1: took over 8/1c00/2 for sid 1c00 "display-subsystem" at simplebus0 not configured The good thing is that (a) is not a problem in this case as we do not have any active transactions such mappings, as far as I can see on the X1E. But the framebuffer is behind the display-subsystem which is now subjected to strict IOMMU configuration, making us need to fix (b). One idea I have is that we keep all the setup code where it is, at IOMMU-bound DMA tag creation time, but only move the stream to the strict context block once someone(TM) creates a DMA map using this tag. Is that a reasonable thing to do? I don't think anyone would be doing DMA without creating a DMA map first, right? (2) Re-use of SMRs doesn't take mask in account. In (1b) you can see that the display subsystem is using sid 0x1c00 with mask 0x2, matching to the following SMR: smmu1: SMR[8] = 0x1c00/0x2 We'd like to re-use that if possible, but because of the strict mask checking that wasn't possible. So I'd like to take the mask into consideration when "taking over" streams. This is only done on QC! Please give this a go on machines where `dmesg | grep ^smmu' gives output, and especially on QC machines with X1E, or SC8280XP/ThinkPad x13s. Please send me some dmesgs, as I have added some debug code that I'd like to see run on other machines. :) Cheers, Patrick diff --git a/sys/arch/arm64/dev/smmu.c b/sys/arch/arm64/dev/smmu.c index edf5dd26de8..43ab9622b49 100644 --- a/sys/arch/arm64/dev/smmu.c +++ b/sys/arch/arm64/dev/smmu.c @@ -282,6 +282,8 @@ smmu_attach(struct smmu_softc *sc) SMMU_SMR_ID_MASK; sc->sc_smr[i]->ss_mask = (reg >> SMMU_SMR_MASK_SHIFT) & SMMU_SMR_MASK_MASK; + printf("%s: SMR[%u] = 0x%x/0x%x\n", sc->sc_dev.dv_xname, + i, sc->sc_smr[i]->ss_id, sc->sc_smr[i]->ss_mask); if (sc->sc_bypass_quirk) { smmu_gr0_write_4(sc, SMMU_S2CR(i), SMMU_S2CR_TYPE_TRANS | @@ -561,6 +563,7 @@ smmu_domain_lookup(struct smmu_softc *sc, uint32_t sid) return dom; } + printf("%s: establishing sid 0x%x\n", sc->sc_dev.dv_xname, sid); return smmu_domain_create(sc, sid); } @@ -614,8 +617,12 @@ smmu_domain_create(struct smmu_softc *sc, uint32_t sid) /* Take over QCOM SMRs */ if (sc->sc_is_qcom && sc->sc_smr[i] != NULL && sc->sc_smr[i]->ss_dom == NULL && - sc->sc_smr[i]->ss_id == sid && - sc->sc_smr[i]->ss_mask == 0) { + !((sc->sc_smr[i]->ss_id ^ sid) & + ~sc->sc_smr[i]->ss_mask)) { + printf("%s: took over %u/%x/%x for sid %x\n", + sc->sc_dev.dv_xname, i, + sc->sc_smr[i]->ss_id, + sc->sc_smr[i]->ss_mask, sid); free(sc->sc_smr[i], M_DEVBUF, sizeof(struct smmu_smr)); sc->sc_smr[i] = NULL; @@ -771,6 +778,28 @@ smmu_domain_create(struct smmu_softc *sc, uint32_t sid) reg |= SMMU_CB_SCTLR_ASIDPNE; smmu_cb_write_4(sc, dom->sd_cb_idx, SMMU_CB_SCTLR, reg); + snprintf(dom->sd_exname, sizeof(dom->sd_exname), "%s:%x", + sc->sc_dev.dv_xname, sid); + dom->sd_iovamap = extent_create(dom->sd_exname, 0, + (1LL << iovabits) - 1, M_DEVBUF, NULL, 0, EX_WAITOK | + EX_NOCOALESCE); + + /* Reserve first page (to catch NULL access) */ + extent_alloc_region(dom->sd_iovamap, 0, PAGE_SIZE, EX_WAITOK); + + SIMPLEQ_INSERT_TAIL(&sc->sc_domains, dom, sd_list); + return dom; +} + +static void +smmu_domain_enable(struct smmu_domain *dom) +{ + struct smmu_softc *sc = dom->sd_sc; + uint32_t reg; + + if (dom->sd_enabled) + return; + /* Point stream to context block */ reg = SMMU_S2CR_TYPE_TRANS | dom->sd_cb_idx; if (sc->sc_has_exids && sc->sc_smr) @@ -779,23 +808,13 @@ smmu_domain_create(struct smmu_softc *sc, uint32_t sid) /* Map stream idx to S2CR idx */ if (sc->sc_smr) { - reg = sid; + reg = dom->sd_sid; if (!sc->sc_has_exids) reg |= SMMU_SMR_VALID; smmu_gr0_write_4(sc, SMMU_SMR(dom->sd_smr_idx), reg); } - snprintf(dom->sd_exname, sizeof(dom->sd_exname), "%s:%x", - sc->sc_dev.dv_xname, sid); - dom->sd_iovamap = extent_create(dom->sd_exname, 0, - (1LL << iovabits) - 1, M_DEVBUF, NULL, 0, EX_WAITOK | - EX_NOCOALESCE); - - /* Reserve first page (to catch NULL access) */ - extent_alloc_region(dom->sd_iovamap, 0, PAGE_SIZE, EX_WAITOK); - - SIMPLEQ_INSERT_TAIL(&sc->sc_domains, dom, sd_list); - return dom; + dom->sd_enabled = 1; } void @@ -1299,6 +1318,8 @@ smmu_dmamap_create(bus_dma_tag_t t, bus_size_t size, int nsegments, u_long dva, len; int error; + smmu_domain_enable(dom); + error = sc->sc_dmat->_dmamap_create(sc->sc_dmat, size, nsegments, maxsegsz, boundary, flags, &map); if (error) diff --git a/sys/arch/arm64/dev/smmu_fdt.c b/sys/arch/arm64/dev/smmu_fdt.c index 2c645da3f37..ac9ad9a9089 100644 --- a/sys/arch/arm64/dev/smmu_fdt.c +++ b/sys/arch/arm64/dev/smmu_fdt.c @@ -89,11 +89,6 @@ smmu_fdt_attach(struct device *parent, struct device *self, void *aux) if (OF_getproplen(faa->fa_node, "dma-coherent") == 0) sc->sc_coherent = 1; - if (sc->sc_is_qcom) { - printf(": disabled\n"); - return; - } - if (smmu_attach(sc) != 0) return; diff --git a/sys/arch/arm64/dev/smmuvar.h b/sys/arch/arm64/dev/smmuvar.h index c01e10518ee..03373167089 100644 --- a/sys/arch/arm64/dev/smmuvar.h +++ b/sys/arch/arm64/dev/smmuvar.h @@ -18,6 +18,7 @@ struct smmu_softc; struct smmu_domain { struct smmu_softc *sd_sc; + uint32_t sd_enabled; uint32_t sd_sid; bus_dma_tag_t sd_dmat; int sd_cb_idx;