On 10/12/25 5:13 PM, 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 security states
> (Non-secure, Realm, Root) 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 and Root 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/Root handling */
> }
>
> will need to be expanded. This will be necessary to differentiate
> between the Root Control Page and Realm Register Pages, especially since
> the Root Control Page is IMPLEMENTATION DEFINED.
>
> Signed-off-by: Tao Tang <[email protected]>
> ---
> hw/arm/smmuv3.c | 61 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index f161dd3eff..100caeeb35 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1675,12 +1675,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> }
>
> 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;
>
> - 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 */
> @@ -1745,12 +1746,13 @@ 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)
> {
> - 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;
> @@ -1985,12 +1987,33 @@ static MemTxResult smmu_write_mmio(void *opaque,
> hwaddr offset, uint64_t data,
> /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */
> offset &= ~0x10000;
>
> + SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS;
> + /*
> + * The security state of the access (MemTxAttrs attrs) may differ from
> the
> + * security state to which the register belongs. Future support will
> include
> + * Realm and Root security states.
> + *
> + * The SMMU architecture specifies that Realm, Root, and Non-secure
> + * registers share the same offset layout within the last 4 hexadecimal
> + * digits (16 bits) of the address. Only Secure state registers are
> + * mapped to a higher address space, starting from
> + * SMMU_SECURE_REG_START (0x8000).
> + *
> + * Therefore, we can directly use the offset to distinguish Secure
> + * registers. When Realm and Root support is needed in the future, we
> + * only need to enhance the 'else' block of the corresponding 'if'
> + * statement to handle those specific security states.
I wouldn't add that many references to the Realm and Root support in the
very code. Maybe move that in the commit desc.
> + */
> + 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;
> @@ -2002,12 +2025,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, reg_sec_sid)) {
> @@ -2036,12 +2060,13 @@ 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);
> return MEMTX_OK;
> @@ -2165,13 +2190,17 @@ static MemTxResult smmu_read_mmio(void *opaque,
> hwaddr offset, uint64_t *data,
>
> /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */
> offset &= ~0x10000;
> + SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS;
declaration should be at the beginning of the function, ie before the
offset setting
> + if (offset >= SMMU_SECURE_REG_START) {
> + reg_sec_sid = SMMU_SEC_SID_S;
> + }
>
> switch (size) {
> case 8:
> - r = smmu_readll(s, offset, data, attrs);
> + r = smmu_readll(s, offset, data, attrs, reg_sec_sid);
> break;
> case 4:
> - r = smmu_readl(s, offset, data, attrs);
> + r = smmu_readl(s, offset, data, attrs, reg_sec_sid);
> break;
> default:
> r = MEMTX_ERROR;
Thanks
Eric
