[RFC v3 15/21] hw/arm/smmuv3: Determine register bank from MMIO offset

Tao Tang posted 21 patches 4 months ago
Maintainers: Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
[RFC v3 15/21] hw/arm/smmuv3: Determine register bank from MMIO offset
Posted by Tao Tang 4 months ago
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 <tangtao1634@phytium.com.cn>
---
 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.
+     */
+    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;
+    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;
-- 
2.34.1
Re: [RFC v3 15/21] hw/arm/smmuv3: Determine register bank from MMIO offset
Posted by Eric Auger 2 months, 1 week ago

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 <tangtao1634@phytium.com.cn>
> ---
>  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
Re: [RFC v3 15/21] hw/arm/smmuv3: Determine register bank from MMIO offset
Posted by Tao Tang 2 months, 1 week ago
Hi Eric,

On 2025/12/4 22:21, Eric Auger wrote:
>
> 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 <tangtao1634@phytium.com.cn>
>> ---
>>   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
>> ------------------------------<snip>------------------------------
>>
>>
>>
>> ------------------------------<snip>------------------------------
>>           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.
>> ------------------------------<snip>------------------------------
>>
>>
>>
>> ------------------------------<snip>------------------------------
>>   
>>   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


Thanks for the review. I'll keep only a short comment in the code and 
move the roadmap description into commit msg. I'll also move these 
declarations to the beginning (I've found this issue in a few other places).


Best regards,

Tao
Re: [RFC v3 15/21] hw/arm/smmuv3: Determine register bank from MMIO offset
Posted by Pierrick Bouvier 3 months, 4 weeks ago
Hi Tao,

On 10/12/25 8:13 AM, 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 <tangtao1634@phytium.com.cn>
> ---
>   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).
> +     *

This is not exact, as Root registers have their own subset and offsets. 
Thus, they will need a specific bank and adapted read/write functions to 
deal with that.

It's not a big deal, but better to edit the comment.

...