Note: Resending due to delivery failure to qemu-devel mailing list. I'm
not sure if everyone received the original email (at least qemu-devel
not), so please disregard if this is a duplicate.
On 2025/11/21 21:02, Eric Auger wrote:
Hi Tao,
On 10/12/25 5:06 PM, Tao Tang wrote:
Rework the SMMUv3 state management by introducing a banked register
structure. This is a purely mechanical refactoring with no functional
changes.
To support multiple security states, a new enum, SMMUSecSID, is
introduced to identify each state, sticking to the spec terminology.
A new structure, SMMUv3RegBank, is then defined to hold the state
for a single security context. The main SMMUv3State now contains an
array of these banks, indexed by SMMUSecSID. This avoids the need for
separate fields for non-secure and future secure registers.
All existing code, which handles only the Non-secure state, is updated
to access its state via s->bank[SMMU_SEC_SID_NS]. A local bank helper
pointer is used where it improves readability.
Function signatures and logic remain untouched in this commit to
isolate the structural changes and simplify review. This is the
foundational step for building multi-security-state support.
Signed-off-by: Tao Tang <[email protected]>
---
hw/arm/smmuv3-internal.h | 24 ++-
hw/arm/smmuv3.c | 344 +++++++++++++++++++----------------
include/hw/arm/smmu-common.h | 6 +
include/hw/arm/smmuv3.h | 38 +++-
4 files changed, 239 insertions(+), 173 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index e420c5dc72..858bc206a2 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -248,7 +248,9 @@ REG32(S_EVENTQ_IRQ_CFG2, 0x80bc)
static inline int smmu_enabled(SMMUv3State *s)
{
- return FIELD_EX32(s->cr[0], CR0, SMMUEN);
+ SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
+ SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
+ return FIELD_EX32(bank->cr[0], CR0, SMMUEN);
}
/* Command Queue Entry */
@@ -276,12 +278,16 @@ static inline uint32_t smmuv3_idreg(int regoffset)
static inline bool smmuv3_eventq_irq_enabled(SMMUv3State *s)
{
- return FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN);
+ SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
+ SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
why aren't you using smmuv3_bank_ns(s) here and elsewhere. Some other
functions are already using it.
Is it to reduce the diffstat in subsequent patches?
Hi Eric,
Yes, you guessed correctly—my original intention was indeed to reduce
the diffstat in subsequent patches.
The smmuv3_bank_ns helper was originally kept mainly to handle specific
edge cases like IDR5.OAS, which is unique because it only exists in the
Non-Secure bank. But it seems that IDR5.OAS is the ONLY case
using smmuv3_bank_ns.
Therefore, for the sake of consistency, I plan to remove the
smmuv3_bank_ns helper entirely and use smmuv3_bank(s, SMMU_SEC_SID_NS)
uniformly in V4.
+ return FIELD_EX32(bank->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN);
}
static inline bool smmuv3_gerror_irq_enabled(SMMUv3State *s)
{
- return FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN);
+ SMMUSecSID sec_sid = SMMU_SEC_SID_NS;
+ SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
+ return FIELD_EX32(bank->irq_ctrl, IRQ_CTRL, GERROR_IRQEN);
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
+
+ bk->features = 0;
+ bk->sid_split = 0;
s->aidr = 0x1;
maybe put the non banked regs at the end to have a clear separation.
There is no ordering concern I think.
- s->cr[0] = 0;
- s->cr0ack = 0;
- s->irq_ctrl = 0;
- s->gerror = 0;
- s->gerrorn = 0;
+ bk->cr[0] = 0;
+ bk->cr0ack = 0;
+ bk->irq_ctrl = 0;
+ bk->gerror = 0;
+ bk->gerrorn = 0;
s->statusr = 0;
- s->gbpa = SMMU_GBPA_RESET_VAL;
+ bk->gbpa = SMMU_GBPA_RESET_VAL;
}
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
@@ -548,7 +556,8 @@ static int decode_ste(SMMUv3State *s,
SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
{
uint32_t config;
- uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
+ /* OAS is shared between S and NS and only present on NS-IDR5 */
I am not sure the comment belongs to this patch as up to now we are just
converting the existing code
I will also remove the premature comment regarding OAS and reorder the
non-banked registers initialization to the end in V4.
Thanks again for your suggestion.
Regards,
Tao
