Hi Mostafa,
On 2026/2/27 PM10:38, Mostafa Saleh wrote:
On Sat, Feb 21, 2026 at 06:02:23PM +0800, 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]>
Reviewed-by: Eric Auger <[email protected]>
There are a few warning from scripts/checkpatch.pl, plus some other
nits I mentioned, otherwise:
Reviewed-by: Mostafa Saleh <[email protected]>
---
hw/arm/smmuv3-accel.c | 42 +++--
hw/arm/smmuv3-internal.h | 24 ++-
hw/arm/smmuv3.c | 345 +++++++++++++++++++----------------
include/hw/arm/smmu-common.h | 6 +
include/hw/arm/smmuv3.h | 30 ++-
5 files changed, 257 insertions(+), 190 deletions(-)
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
@@ -687,22 +689,24 @@ void smmuv3_accel_idr_override(SMMUv3State *s)
return;
}
+ SMMUv3RegBank *bank = smmuv3_bank(s, SMMU_SEC_SID_NS);
Sometimes you have:
SMMUv3RegBank *bank = smmuv3_bank(s, SMMU_SEC_SID_NS);
Others
SMMUv3RegBank *bk = smmuv3_bank(s, SMMU_SEC_SID_NS);
It would be helpful if naming is consistent.
Apologies for not switching to plain text only in Thunderbird in the
previous reply email, which made the formatting look a bit strange.
I'll unify this definition across all affected places in V5, and also
fix style warnings you mentioned previously.
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
@@ -94,7 +99,14 @@ struct SMMUv3Class {
#define TYPE_ARM_SMMUV3 "arm-smmuv3"
OBJECT_DECLARE_TYPE(SMMUv3State, SMMUv3Class, ARM_SMMUV3)
-#define STAGE1_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S1P)
-#define STAGE2_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S2P)
+#define STAGE1_SUPPORTED(s) \
+ FIELD_EX32((s)->bank[SMMU_SEC_SID_NS].idr[0], IDR0, S1P)
+#define STAGE2_SUPPORTED(s) \
+ FIELD_EX32((s)->bank[SMMU_SEC_SID_NS].idr[0], IDR0, S2P)
+
+static inline SMMUv3RegBank *smmuv3_bank(SMMUv3State *s, SMMUSecSID sec_sid)
+{
+ return &s->bank[sec_sid];
+}
If this is a macro you can use in other cases as in state structs as
it currently inlined in:
VMSTATE_UINT32(bank[SMMU_SEC_SID_NS].irq_ctrl, SMMUv3State),
My understanding is that VMSTATE_* wants an offsetof() ( VMSTATE_UINT32
-> VMSTATE_INT32_V -> VMSTATE_SINGLE -> VMSTATE_SINGLE_TEST ->
vmstate_offset_value ->offsetof ), so maybe we could not directly reuse
the runtime helper there. At runtime, smmuv3_bank() needs a real
`SMMUv3State *s` to compute the actual address of the selected bank
(`&s->bank[sec_sid]`), which is different from a compile-time field offset.
A small compromise that works for both is to define a field-path macro
and then take its address in smmuv3_bank():
```C
/* Field-path macro (offsetof-friendly) */
#define SMMUV3_BANK_F(_sid) bank[(_sid)]
#define SMMUV3_NS_BANK_F SMMUV3_BANK_F(SMMU_SEC_SID_NS)
static inline SMMUv3RegBank *smmuv3_bank(SMMUv3State *s, SMMUSecSID sec_sid)
{
return &s->SMMUV3_BANK_F(sec_sid);
}
/* VMSTATE uses the field-path macro directly */
VMSTATE_UINT32(SMMUV3_NS_BANK_F.irq_ctrl, SMMUv3State);
...
```
How do you think about it?
Thanks,
Mostafa
Thanks for your review.
Tao
