Re: [RFC v5 02/28] hw/arm/smmuv3: Introduce banked registers for SMMUv3 state

Hi Jim,

On 8/21/2026 5:52 PM, Jim MacArthur wrote:
> On Fri, Aug 14, 2026 at 12:24:07AM +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.
>>
>>   static void smmuv3_reset(SMMUv3State *s)
>>   {
>> -    s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
>> -    s->cmdq.prod = 0;
>> -    s->cmdq.cons = 0;
>> -    s->cmdq.entry_size = sizeof(struct Cmd);
>> -    s->eventq.base = deposit64(s->eventq.base, 0, 5, SMMU_EVENTQS);
>> -    s->eventq.prod = 0;
>> -    s->eventq.cons = 0;
>> -    s->eventq.entry_size = sizeof(struct Evt);
>> -
>> -    s->features = 0;
>> -    s->sid_split = 0;
>> -    s->cr[0] = 0;
>> -    s->cr0ack = 0;
>> -    s->irq_ctrl = 0;
>> -    s->gerror = 0;
>> -    s->gerrorn = 0;
>> +    SMMUv3RegBank *bank = smmuv3_bank(s, SMMU_SEC_SID_NS);
>> +
>> +    bank->cmdq.base = deposit64(bank->cmdq.base, 0, 5, SMMU_CMDQS);
>> +    bank->cmdq.prod = 0;
>> +    bank->cmdq.cons = 0;
>> +    bank->cmdq.entry_size = sizeof(struct Cmd);
>> +    bank->eventq.base = deposit64(bank->eventq.base, 0, 5, SMMU_EVENTQS);
>> +    bank->eventq.prod = 0;
>> +    bank->eventq.cons = 0;
>> +    bank->eventq.entry_size = sizeof(struct Evt);
>> +
>> +    bank->features = 0;
>> +    bank->sid_split = 0;
>> +    bank->cr[0] = 0;
>> +    bank->cr0ack = 0;
>> +    bank->irq_ctrl = 0;
>> +    bank->gerror = 0;
>> +    bank->gerrorn = 0;
>> +    bank->gbpa = SMMU_GBPA_RESET_VAL;
>> +
>> +    s->aidr = 0x1;
>>       s->statusr = 0;
>> -    s->gbpa = SMMU_GBPA_RESET_VAL;
>>   }
> Why reset s->aidr here? It should be read-only and set in smmuv3_init_id_regs.


Good catch, this is redundant since AIDR is initialized in 
smmuv3_init_id_regs(). I'll remove it in the next revision.


>
> It won't do any harm and the reset of this patch looks correct, so:
>
> Reviewed-by: Jim MacArthur <[email protected]>


Thanks,

Tao