Index | Thread | Search

From:
Patrick Wildt <patrick@blueri.se>
Subject:
Re: smmu(4) on QC Laptops
To:
Landry Breuil <landry@openbsd.org>
Cc:
tech@openbsd.org, kettenis@openbsd.org, tobhe@openbsd.org, mlarkin@openbsd.org
Date:
Fri, 23 May 2025 16:55:48 +0200

Download raw body.

Thread
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.
> > > <snip>
> > > > 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;