From: Patrick Wildt Subject: Re: smmu(4) on QC Laptops To: Landry Breuil Cc: tech@openbsd.org, kettenis@openbsd.org, tobhe@openbsd.org, mlarkin@openbsd.org Date: Fri, 23 May 2025 16:55:48 +0200 On Fri, May 23, 2025 at 01:56:22PM +0200, Landry Breuil wrote: > Le Fri, May 23, 2025 at 10:22:19AM +0200, Patrick Wildt a ?crit : > > On Fri, May 23, 2025 at 09:20:52AM +0200, Landry Breuil wrote: > > > Le Fri, May 23, 2025 at 08:46:16AM +0200, Patrick Wildt a ?crit : > > > > 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. > > > > > > > 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. :) > > > > > > doesnt blow up on the hp omnibook, dmesg below. > > > > I forgot to mention, this needs a `make config` because of the early > > attach change that I forgot to mention, otherwise smmu1 pops up too > > late :/ Can you give it another try? Thanks! > > unless i did something very wrong, make config && make && make install > && reboot gives the same dmesg order for smmuX. what should i see > different ? It's not you, it's me. I'm sorry, I missed a tiny chunk that's in conf/ and I was sooo sure it was part of the mail. Same diff + the chunk for conf/: diff --git a/sys/arch/arm64/conf/GENERIC b/sys/arch/arm64/conf/GENERIC index 518a4271f5e..bb2f74e0de6 100644 --- a/sys/arch/arm64/conf/GENERIC +++ b/sys/arch/arm64/conf/GENERIC @@ -92,7 +92,7 @@ pci* at pciecam? sdhc* at fdt? sdmmc* at sdhc? bwfm* at sdmmc? # Broadcom FullMAC -smmu* at fdt? +smmu* at fdt? early 1 xhci* at fdt? ccp* at fdt? # AMD Cryptographic Co-processor ipmi* at fdt? 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;