Download raw body.
smmu(4) on QC Laptops
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;
smmu(4) on QC Laptops