feat(armv8/iommu): add SMMUv3 stage-2 driver - #382
Conversation
joaopeixoto13
left a comment
joaopeixoto13
left a comment
There was a problem hiding this comment.
Thanks a lot for this. An SMMUv3 driver has been sitting in our "someday" pile for some time, so it's great to see a clean, focused driver actually land!
Did a close pass and two things I'd want sorted before moving on, and a couple of design questions that aren't blockers.
Functional issues
-
Init-order race on the poll hook.
smmu.evtq_prodgets published near the top ofsmmuv3_init(), right after themem_alloc_map_dev(), butsmmu.evtqisn't allocated andEVENTQ_BASEisn't programmed until much further down.smmuv3_poll_events()only checksevtq_prod != NULLand fires fromgic_handle()on every core for every IRQ, so any IPI landing on a secondary CPU while the boot CPU is still mid-init will latch a stale PROD/CONS delta and walksmmu.evtqwhile it's still NULL. Cleanest fix is areadyflag flipped last, onceSMMUENis acked, with the poll hook gated on that instead of on the pointer. Left specifics inline. -
No SMMU TLBI on stage-2 remap. You issue
TLBI_S12_VMALLat bind time, but nothing hangs off the stage-2 invalidation path (core/inc/tlb.h, the// TODO: inval iommu tlbssites), soTLBI VMALLS12E1ISon the CPU side never reaches the SMMU's TLBs. Fine as long as every device mapping is static, which is almost certainly why it comes up clean, but the moment a bound VM's stage-2 gets touched the SMMU keeps hitting stale walk-cache/TLB entries and translating against the old tables. This PR is what finally makes those TODOs actionable, so it's the natural spot to hook SMMU invalidation into that path, or at least to spell out the static-mapping assumption in the docs.
Design questions (not blockers)
-
BYPASS-by-default vs abort-by-default. Any streamID without a group stays parked on a BYPASS STE, i.e. free-for-all DMA into any VM's PA space. Reasonable for a transparent firmware handoff, but it means the SMMU is acting as an opt-in translator rather than an isolation boundary: a master nobody enumerated sails through instead of getting terminated. Once a platform's streamID map is fully known you'd usually want the unbound default to be abort (invalid STE, faults land in the event queue). Any appetite for a build/per-platform knob to flip the default?
-
Single instance. One
static struct smmuv3_priv smmuplus a singleplatform.arch.smmubakes in one SMMU per SoC. Multi-SMMU parts are increasingly common, and a streamID is only unique within a given instance's stream table, so the config eventually needs a per-group "which SMMU" selector. Nothing to do here, just worth not boxing the interface in now.
Rest is smaller stuff and a few latent-but-not-yet-reachable bits, all inline. Thanks again for putting this up.
| vaddr_t regs = mem_alloc_map_dev(&cpu()->as, SEC_HYP_GLOBAL, INVALID_VA, | ||
| platform.arch.smmu.base, NUM_PAGES(SMMUV3_PAGE1_EVENTQ_CONS + 4)); | ||
| smmu.hw = (volatile struct smmuv3_hw*)regs; | ||
| smmu.evtq_prod = (volatile uint32_t*)(regs + SMMUV3_PAGE1_EVENTQ_PROD); |
There was a problem hiding this comment.
evtq_prod/evtq_cons are published early in smmuv3_init(), but smmu.evtq is allocated later and EVENTQ_BASE programmed later still. smmuv3_poll_events() (called from gic_handle() on every CPU, every IRQ) gates only on evtq_prod == NULL, so an IPI taken by another core during init can read a stale PROD/CONS pair, and dereference smmu.evtq while it's still NULL.
Suggest a dedicated ready flag published last. Sketch (spans three spots, so not a one-click suggestion):
/* in struct smmuv3_priv, alongside `coherent`: */
bool coherent;
bool ready; /* set once init completes; gates the poll hook */
spinlock_t lock;
/* in smmuv3_poll_events(), replace the NULL gate: */
if (!smmu.ready) {
return;
}
/* at the very end of smmuv3_init(), after SMMUEN is acked: */
smmu.ready = true;There was a problem hiding this comment.
Agee - I will add ready flag along with release barrier before publishing ready
/* smmuv3.c — struct */
struct smmuv3_priv {
...
bool coherent;
volatile bool ready; /* published last; gates the poll hook */
};
/* smmuv3_poll_events() */
void smmuv3_poll_events(void)
{
if (!smmu.ready) {
return;
}
/* Acquire: order the flag load before the queue-state loads below.
* A control dependency alone doesn't order load→load on ARMv8. */
fence_ord_read(); /* DMB ISHLD */
uint32_t prod = *smmu.evtq_prod;
}
/* end of smmuv3_init(), after the CR0ACK poll loop */
/* Release: make every prior store (evtq pointer, zeroed queue memory,
* PROD/CONS shadow init) visible to other observers before the flag.
* Without this, another core can observe ready==true while smmu.evtq
* is still NULL from its point of view.
*/
fence_ord_write(); /* DMB ISHST — store→store ordering */
smmu.ready = true;
| #if (SMMU_VERSION == 3) | ||
| /* Surface any SMMU translation faults (a refused DMA usually coincides with | ||
| * a device error IRQ). Cheap no-op when the event queue is empty. */ | ||
| smmuv3_poll_events(); |
There was a problem hiding this comment.
This runs on every interrupt on every CPU and does two page-1 MMIO reads even in the common empty case: a measurable per-IRQ cost on interrupt-heavy workloads. It's a genuinely useful backstop where the wired eventq IRQ isn't reliably delivered, so I wouldn't drop it, but just consider scoping it. Cheapest option, restrict the poll to the master CPU (the wired IRQ handler already covers the general case):
#if (SMMU_VERSION == 3)
if (cpu_is_master()) {
smmuv3_poll_events();
}
#endifThere was a problem hiding this comment.
Agree, I will update the PR with this change.
| /* Linear stream table cap: 2^8 = 256 entries (16 KiB). This comfortably covers | ||
| * typical single-level stream-ID spaces without needing a 2-level table. */ | ||
| #define SMMUV3_ST_LOG2_CAP (8) |
There was a problem hiding this comment.
The 256-entry cap assumes a small, dense stream-ID space. Stream IDs are integrator-defined and can be sparse or start high (PCIe Requester IDs especially). On a platform whose IDs exceed 255, every smmuv3_write_ste_s2() fails its bounds check and VM init aborts, with the only fix being to edit this constant. The table is linear, so letting a platform override it costs only memory. Suggestion makes it overridable from platform.mk (e.g. -DSMMUV3_ST_LOG2_CAP=11):
| /* Linear stream table cap: 2^8 = 256 entries (16 KiB). This comfortably covers | |
| * typical single-level stream-ID spaces without needing a 2-level table. */ | |
| #define SMMUV3_ST_LOG2_CAP (8) | |
| /* Linear stream table cap: 2^SMMUV3_ST_LOG2_CAP entries (default 2^8 = 256, | |
| * 16 KiB). A platform whose stream-ID space is sparse or starts high can | |
| * override this from its platform.mk (e.g. -DSMMUV3_ST_LOG2_CAP=11). */ | |
| #ifndef SMMUV3_ST_LOG2_CAP | |
| #define SMMUV3_ST_LOG2_CAP (8) | |
| #endif |
There was a problem hiding this comment.
Good point - I will update the PR code , so it can be overridable from platform.mk.
| * wired event IRQ isn't delivered. No-op until the driver is initialised. */ | ||
| void smmuv3_poll_events(void) | ||
| { | ||
| if (smmu.evtq_prod == NULL) { |
There was a problem hiding this comment.
Part of the init-race fix above. This is the gate that goes live too early. Once a ready flag exists, this line becomes if (!smmu.ready) return;. Flagging it here so the two spots are reviewed together.
| bool gicd_get_pend(irqid_t int_id) | ||
| { | ||
| return (gicd->ISPENDR[GIC_INT_REG(int_id)] & GIC_INT_MASK(int_id)) != 0; | ||
| } | ||
| { return (gicd->ISPENDR[GIC_INT_REG(int_id)] & GIC_INT_MASK(int_id)) != 0; } |
There was a problem hiding this comment.
This looks like unrelated reformatting (the one-lined bodies don't match the surrounding style and aren't part of the SMMUv3 change). Worth reverting to keep the diff focused:
| bool gicd_get_pend(irqid_t int_id) | |
| { | |
| return (gicd->ISPENDR[GIC_INT_REG(int_id)] & GIC_INT_MASK(int_id)) != 0; | |
| } | |
| { return (gicd->ISPENDR[GIC_INT_REG(int_id)] & GIC_INT_MASK(int_id)) != 0; } | |
| bool gicd_get_pend(irqid_t int_id) | |
| { | |
| return (gicd->ISPENDR[GIC_INT_REG(int_id)] & GIC_INT_MASK(int_id)) != 0; | |
| } |
There was a problem hiding this comment.
This must be due to my local clang-format check, will fix in PR.
| } | ||
| } | ||
|
|
||
| bool gicd_get_act(irqid_t int_id) |
| for (size_t i = 0; i < SMMUV3_POLL_LIMIT; i++) { | ||
| if ((smmu.hw->CMDQ_CONS & SMMUV3_Q_IDX_MASK(SMMUV3_CMDQ_LOG2SIZE)) == | ||
| (smmu.cmdq_prod & SMMUV3_Q_IDX_MASK(SMMUV3_CMDQ_LOG2SIZE))) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The sync-completion check compares only the index bits and drops the wrap bit, so a completely full queue (PROD = CONS + size) is indistinguishable from empty. Not reachable today since every push sequence syncs within a few entries of a 128-deep queue, but latent if commands are ever batched before syncing. cmdq_prod already carries the full {wrap, index} (it's kept modulo 2×size), and hardware maintains the wrap bit in CMDQ_CONS, so comparing the full field is robust:
| for (size_t i = 0; i < SMMUV3_POLL_LIMIT; i++) { | |
| if ((smmu.hw->CMDQ_CONS & SMMUV3_Q_IDX_MASK(SMMUV3_CMDQ_LOG2SIZE)) == | |
| (smmu.cmdq_prod & SMMUV3_Q_IDX_MASK(SMMUV3_CMDQ_LOG2SIZE))) { | |
| return; | |
| } | |
| } | |
| uint32_t fullmask = (SMMUV3_Q_WRAP(SMMUV3_CMDQ_LOG2SIZE) << 1) - 1; | |
| for (size_t i = 0; i < SMMUV3_POLL_LIMIT; i++) { | |
| if ((smmu.hw->CMDQ_CONS & fullmask) == (smmu.cmdq_prod & fullmask)) { | |
| return; | |
| } | |
| } |
There was a problem hiding this comment.
Agree - I will update the PR.
| #define SMMUV3_IDR1_TABLES_PRESET (1U << 30) | ||
| #define SMMUV3_IDR1_QUEUES_PRESET (1U << 29) | ||
| #define SMMUV3_IDR1_CMDQS_OFF (21) | ||
| #define SMMUV3_IDR1_CMDQS_LEN (5) | ||
| #define SMMUV3_IDR1_EVTQS_OFF (16) | ||
| #define SMMUV3_IDR1_EVTQS_LEN (5) |
There was a problem hiding this comment.
These capability macros are defined but never used
There was a problem hiding this comment.
I will remove these unused macros.
|
Thanks for the review @joaopeixoto13
I agree with your suggestion and will update the code in this PR.
Agreed this is a real gap. It's not reachable in the current static-partitioning model. I foresee the testing would be a major challenge as well.
The knob is cheap, low-risk, and strictly increases capability while keeping the safe default — worth adding.
Fair point. I'll leave single-instance as-is for now, but I agree the interface shouldn't be boxed in — a per-group "which SMMU" selector is the right eventual shape given stream IDs are only unique within an instance's stream table. |
Yeah, agreed. Just note it needs a barrier on both sides, not just the writer. The
Fine to split it. Nothing hits that path in the static model today, so documenting the assumption now and doing the real hook later works for me. Two small asks: drop a note at the two And yeah, testing is the hard bit. You can't really trigger the stale-TLB case without remapping an already-bound VM's stage-2, which the static model never does, so the test only makes sense once the invalidation code exists. Fine to defer.
Sounds good, that's exactly what I'd want:
Yeah, a code note is enough. I'd put it in two spots: at |
dee589a to
5a5b7a5
Compare
5a5b7a5 to
00ad894
Compare
Add a minimal ARM SMMUv3 (IHI 0070) driver for Armv8-A, providing stage-2-only DMA translation. The driver takes over the non-secure SMMU left enabled by firmware: it installs a linear stream table (all streams BYPASS by default so masters keep working with their incoming attributes), a command queue and an event queue, and promotes individual streams to stage-2 as VMs bind them. Faults are drained and logged via the event queue (wired SPI plus a cheap poll hook on the GIC IRQ path). Driver selection is done at build time via SMMU_VERSION (default 2): arch/smmu.h resolves to smmuv2.h or smmuv3.h, and objects.mk builds the matching driver. Platforms opt into v3 with SMMU_VERSION:=3 in their platform.mk; all existing platforms keep SMMUv2 unchanged. Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
a4946c2 to
650e9d7
Compare
| * 16 KiB). A platform whose stream-ID space is sparse or starts high can | ||
| * override this from its platform.mk (e.g. -DSMMUV3_ST_LOG2_CAP=11). */ | ||
| #ifndef SMMUV3_ST_LOG2_CAP | ||
| #define SMMUV3_ST_LOG2_CAP (8) |
There was a problem hiding this comment.
I know the platform.mk override came out of the earlier discussion, but I'd rather not have a compile time cap at all. There is no reason 2^8 covers whatever platforms we end up running on, and any PCIe derived stream IDs blow way past what a linear table can reasonably hold. Since the full config is available when the driver initializes, can we size the table from it instead? Scan every VM's smmu groups and device ids, take the largest (for groups, id | mask), round up to a power of two and clamp to SIDSIZE. We could eventually do it completely statically using the config scritps to generate macros. Anything above the used ID space then fail-closes with C_BAD_STREAMID without spending STEs on it. Also please add a TODO noting the proper fix for large or sparse StreamID spaces is a 2-level stream table when SMMU_IDR0.ST_LEVEL allows it.
@joaopeixoto13 wdyt, does the config derived sizing work for you?
|
|
||
| static void smmuv3_check_features(void) | ||
| { | ||
| uint32_t aidr = smmu.hw->AIDR & 0xF; |
There was a problem hiding this comment.
Maybe we could:
| uint32_t aidr = smmu.hw->AIDR & 0xF; | |
| uint32_t aidr = smmu.hw->AIDR & SMMUV3_AIDR_ArchMinorRev_MASK; |
| * its platform.mk. Note this only changes the unbound default; GBPA is kept in | ||
| * BYPASS during the reconfiguration window regardless (see smmuv3_init), so | ||
| * in-flight transactions during handoff never fault. */ | ||
| static void smmuv3_ste_fill_default(struct smmuv3_ste* e) |
There was a problem hiding this comment.
Picking up the bypass vs abort question from the earlier discussion: I'd go all the way and make abort the only behavior, dropping SMMUV3_DEFAULT_ABORT and the GBPA bypass during the init window with it. The smmuv2 driver already faults unmatched streams (USFCFG), and with bypass the SMMU stops being an isolation boundary for anything the configs didn't bind. It's also inconsistent: stream IDs above the table size already abort with C_BAD_STREAMID, so whether an unbound master is contained depends on where its ID happens to fall. @rabara Is there a concrete use case you have or can think of that requires this bypass? If something ever requires it, I'd rather have an explicit per-stream bypass in the config than a global default.
@joaopeixoto13 you raised the original question, agree with going abort only?
| smmu.evtq_prod = (volatile uint32_t*)(regs + SMMUV3_PAGE1_EVENTQ_PROD); | ||
| smmu.evtq_cons = (volatile uint32_t*)(regs + SMMUV3_PAGE1_EVENTQ_CONS); | ||
|
|
||
| smmu.coherent = (smmu.hw->IDR0 & SMMUV3_IDR0_COHACC_BIT) != 0; |
There was a problem hiding this comment.
I see coherent is only used to decide whether to flush the driver's own structures (stream table and queues). But COHACC covers every SMMU-originated access, including the stage 2 walks, and those read the VM page tables, which Bao writes cacheable and never cleans since CPU walkers are coherent. So on a COHACC=0 SMMU the queues would work but the walks would read stale PTEs. I'd make this a hard requirement: ERROR in smmuv3_check_features when COHACC is 0, and drop the non-coherent path altogether, keeping only the barriers. At some point we can add the infrastructure to propagate the core's TLB maintenance to the IOMMU and lift this, but until that exists coherency is a must. Btw, does your Agilex5 integration tie COHACC high? On MMU-600 this depends on the sup_cohacc tie-off.
| STE_W2_S2AFFD | STE_W2_S2R; | ||
|
|
||
| spin_lock(&smmu.lock); | ||
| struct smmuv3_ste* e = &smmu.st[sid]; |
There was a problem hiding this comment.
Nothing prevents two VMs from binding the same stream id: the second write just replaces the STE and the device silently moves to the last VM that bound it. The v2 driver ERRORs on this (smmu_compatible_sme_exists). Since the old STE is right here, checking for a valid entry with a different S2VMID before writing and ERRORing would be enough. We could also return success if the ste we are writing for that sid matches what is already there. This would be useful in platforms where sids are programable and we decide to set all sid of devices assigned to the same VM with the same sid
| * track is whether the STE(s) have been installed. | ||
| */ | ||
| struct iommu_vm_arch { | ||
| bool initialized; |
There was a problem hiding this comment.
This is written but never read anywhere. I'd drop it.
| smmu.coherent ? "coherent" : "non-coherent"); | ||
| } | ||
|
|
||
| bool smmuv3_write_ste_bypass(streamid_t sid) |
There was a problem hiding this comment.
Nothing calls this. If we go abort only for unbound streams it becomes the building block for an explicit per-stream bypass, but until that exists I'd drop it.
| interrupts_reserve(platform.arch.smmu.interrupt_id, smmuv3_eventq_irq_handler); | ||
| if (eid != INVALID_IRQID) { | ||
| interrupts_cpu_enable(eid, true); | ||
| smmu.hw->IRQ_CTRL = SMMUV3_IRQ_CTRL_EVENTQ | SMMUV3_IRQ_CTRL_GERROR; |
There was a problem hiding this comment.
GERROR is enabled here but there's no handler or ack path for it, so a command queue error only surfaces as the CMD_SYNC timeout WARNING. Either handle it minimally, log plus ack, or don't enable it.
| { | ||
| uint32_t wrapmask = (SMMUV3_Q_WRAP(SMMUV3_EVTQ_LOG2SIZE) << 1) - 1; | ||
| uint32_t idxmask = SMMUV3_Q_IDX_MASK(SMMUV3_EVTQ_LOG2SIZE); | ||
| uint32_t prod = *smmu.evtq_prod & wrapmask; |
There was a problem hiding this comment.
The overflow flag in EVENTQ_PROD (bit 31) is masked out and ignored, so an overflowing queue silently loses fault records. Worth at least a WARNING when it's set.
| * + logs faults, so a refused DMA is visible instead of silent. */ | ||
| if (platform.arch.smmu.interrupt_id != 0) { | ||
| irqid_t eid = | ||
| interrupts_reserve(platform.arch.smmu.interrupt_id, smmuv3_eventq_irq_handler); |
There was a problem hiding this comment.
On the fault path as a whole: since we run the terminate model, an aborted DMA already surfaces inside the owning VM through the device itself, bus error status, the device's own error interrupt, a timeout, and the guest driver has to handle those on bare metal anyway. The eventq SPI on the other hand has a single target, so whoever runs on the master cpu absorbs fault handling for every VM's devices, a cross VM interference channel. Hypervisor side fault observation is still valuable to detect a misbehaving VM, but smmuv2 already reports nothing and I'd rather we approach that in a structured way for all iommus in a separate PR, where we can also design around the interference. So for this one I'd drop the eventq interrupt and the gic.c poll entirely and keep the driver to what the isolation needs. @joaopeixoto13 this reverses the earlier conclusion on scoping the poll to the master cpu, wdyt?
There was a problem hiding this comment.
Maybe the comment on EVENTQ_PROD comment loses the effect if we remove fault handling...
3 participants
Add a minimal ARM SMMUv3 (IHI 0070) driver for Armv8-A, providing stage-2-only DMA translation. The driver takes over the non-secure SMMU left enabled by firmware: it installs a linear stream table (all streams BYPASS by default so masters keep working with their incoming attributes), a command queue and an event queue, and promotes individual streams to stage-2 as VMs bind them. Faults are drained and logged via the event queue (wired SPI plus a cheap poll hook on the GIC IRQ path).
Driver selection is done at build time via SMMU_VERSION (default 2): arch/smmu.h resolves to smmuv2.h or smmuv3.h, and objects.mk builds the matching driver. Platforms opt into v3 with SMMU_VERSION:=3 in their platform.mk.
Verified working on Agilex5 SoCFPGA platform.