Hi Mostafa, On 2026/2/27 PM10:59, Mostafa Saleh wrote:
On Sat, Feb 21, 2026 at 06:18:29PM +0800, Tao Tang wrote:Modify the main MMIO handlers (smmu_write_mmio, smmu_read_mmio) to determine the security state of the target register based on its memory-mapped offset.By checking if the offset is within the secure register space (>= SMMU_SECURE_REG_START), the handlers can deduce the register's SEC_SID (reg_sec_sid). This SID is then passed down to the register access helper functions (smmu_writel, smmu_readl, etc.). Inside these helpers, the switch statement now operates on a masked, relative offset: uint32_t reg_offset = offset & 0xfff; switch (reg_offset) { ... } This design leverages a key feature of the SMMU specification: registers with the same function across different 3 security states (Non-secure, Secure, Realm) share the same relative offset. This avoids significant code duplication. The reg_sec_sid passed from the MMIO handler determines which security bank to operate on, while the masked offset identifies the specific register within that bank. It is important to distinguish between the security state of the register itself and the security state of the access. A higher-privilege security state is permitted to access registers belonging to a lower-privilege state, but the reverse is not allowed. This patch lays the groundwork for enforcing such rules. For future compatibility with Realm states, the logic in the else block corresponding to the secure offset check: if (offset >= SMMU_SECURE_REG_START) { reg_sec_sid = SMMU_SEC_SID_S; } else { /* Future Realm handling */ } will need to be expanded. Signed-off-by: Tao Tang<[email protected]> --- hw/arm/smmuv3.c | 57 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 9c09ea0716e..d81485a6a46 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -1781,12 +1781,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp, SMMUSecSID sec_sid) }static MemTxResult smmu_writell(SMMUv3State *s, hwaddr offset,- uint64_t data, MemTxAttrs attrs) + uint64_t data, MemTxAttrs attrs, + SMMUSecSID reg_sec_sid) { - SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS; SMMUv3RegBank *bank = smmuv3_bank(s, reg_sec_sid); + uint32_t reg_offset = offset & 0xfff;This breaks all registers above 4k? For example, TCU registers/PMU reside on some of this area and if SW writes to those this might be aliased to something as CR0, which might disable the SMMU and bypass some security checks. (in case it’s managed by the hypervisor). I believe you should check the secure offset explicitly to distinguish secure and non-secure.
Thanks for catching this. You’re right! I implicitly assumed that anything “above 4KB” in the register aperture could be safely decoded by masking to a 4KB-local offset, but that overlooks the fact that real SMMU IPs populate the *IMPLEMENTATION DEFINED* windows with meaningful blocks. In practice, these implementation-defined ranges appear to start around 0x0E00 in the main bank and 0x8E00 in the secure bank (per the register overview)? So collapsing offsets with offset & 0xfff can easily alias accesses from those regions onto architected registers.
I’ll rework this part: explicitly decode the MMIO access by **register region/window**, and only canonicalize offsets in a way that preserves at least the 64KB page granularity.
- switch (offset) {+ switch (reg_offset) { case A_GERROR_IRQ_CFG0: if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) { /* SMMU_(*_)_IRQ_CTRL.GERROR_IRQEN == 1: IGNORED this write */ @@ -1851,13 +1852,14 @@ static MemTxResult smmu_writell(SMMUv3State *s, hwaddr offset, }static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,- uint64_t data, MemTxAttrs attrs) + uint64_t data, MemTxAttrs attrs, + SMMUSecSID reg_sec_sid) { Error *local_err = NULL; - SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS; SMMUv3RegBank *bank = smmuv3_bank(s, reg_sec_sid); + uint32_t reg_offset = offset & 0xfff;- switch (offset) {+ switch (reg_offset) { case A_CR0: bank->cr[0] = data; bank->cr0ack = data & ~SMMU_CR0_RESERVED; @@ -2094,16 +2096,25 @@ static MemTxResult smmu_write_mmio(void *opaque, hwaddr offset, uint64_t data, SMMUState *sys = opaque; SMMUv3State *s = ARM_SMMUV3(sys); MemTxResult r; + SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS;/* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */offset &= ~0x10000;+ /*+ * Realm and Non-secure share the same page-local offset layout; Secure uses + * the same layout but is mapped starting at 0x8000(SMMU_SECURE_REG_START) + */ + if (offset >= SMMU_SECURE_REG_START) { + reg_sec_sid = SMMU_SEC_SID_S; + } + switch (size) { case 8: - r = smmu_writell(s, offset, data, attrs); + r = smmu_writell(s, offset, data, attrs, reg_sec_sid); break; case 4: - r = smmu_writel(s, offset, data, attrs); + r = smmu_writel(s, offset, data, attrs, reg_sec_sid); break; default: r = MEMTX_ERROR; @@ -2115,12 +2126,13 @@ static MemTxResult smmu_write_mmio(void *opaque, hwaddr offset, uint64_t data, }static MemTxResult smmu_readll(SMMUv3State *s, hwaddr offset,- uint64_t *data, MemTxAttrs attrs) + uint64_t *data, MemTxAttrs attrs, + SMMUSecSID reg_sec_sid) { - SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS; SMMUv3RegBank *bank = smmuv3_bank(s, reg_sec_sid); + uint32_t reg_offset = offset & 0xfff;- switch (offset) {+ switch (reg_offset) { case A_GERROR_IRQ_CFG0: /* SMMU_(*_)GERROR_IRQ_CFG0 BOTH check SMMU_IDR0.MSI */ if (!smmu_msi_supported(s)) { @@ -2149,17 +2161,22 @@ static MemTxResult smmu_readll(SMMUv3State *s, hwaddr offset, }static MemTxResult smmu_readl(SMMUv3State *s, hwaddr offset,- uint64_t *data, MemTxAttrs attrs) + uint64_t *data, MemTxAttrs attrs, + SMMUSecSID reg_sec_sid) { - SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS; SMMUv3RegBank *bank = smmuv3_bank(s, reg_sec_sid); + uint32_t reg_offset = offset & 0xfff;- switch (offset) {+ switch (reg_offset) { case A_IDREGS ... A_IDREGS + 0x2f: - *data = smmuv3_idreg(offset - A_IDREGS); + *data = smmuv3_idreg(reg_offset - A_IDREGS); return MEMTX_OK; case A_IDR0 ... A_IDR5: - *data = bank->idr[(offset - A_IDR0) / 4]; + if (reg_sec_sid == SMMU_SEC_SID_S) { + g_assert((reg_offset - A_IDR0) / 4 < 5);Asserting here is too much as this can be easily triggered by guests, we should follow the convention of other unimplemented registers.
I'll fix it in V5.
Thanks, Mostafa
Thanks for your time! Tao
