On Sun, Mar 01, 2026 at 09:44:32PM +0800, Tao Tang wrote:
> 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():
I see, I missed that, in that case we can just keep it as is, no need to
change.
Thanks,
Mostafa
>
>
> ```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
>
