Re: [RFC 04/11] hw/arm/smmuv3: Enable command processing for the Secure state

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 8/11/25 3:34 AM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 10/8/25 18:59, Tao Tang wrote:
>> On 2025/8/7 05:55, Pierrick Bouvier wrote:
>>> On 8/6/25 8:11 AM, Tao Tang wrote:
>>>> This patch enables the secure command queue, providing a dedicated
>>>> interface for secure software to issue commands to the SMMU. Based on
>>>> the SMMU_S_CMDQ_BASE configuration, the SMMU now fetches command
>>>> entries directly from the Secure PA space so that we need to pass the
>>>> memory transaction attributes when reading the command queue.
>>>>
>>>> This provides a parallel command mechanism to the non-secure world.
>>>>
>>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>>> ---
>>>>    hw/arm/smmuv3-internal.h |  8 ++++--
>>>>    hw/arm/smmuv3.c          | 55 +++++++++++++++++++++++++---------------
>>>>    hw/arm/trace-events      |  2 +-
>>>>    3 files changed, 41 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>>> index 1a8b1cb204..5b2ca00832 100644
>>>> --- a/hw/arm/smmuv3-internal.h
>>>> +++ b/hw/arm/smmuv3-internal.h
>>>> @@ -319,9 +319,13 @@ static inline void queue_cons_incr(SMMUQueue *q)
>>>>        q->cons = deposit32(q->cons, 0, q->log2size + 1, q->cons + 1);
>>>>    }
>>>>    -static inline bool smmuv3_cmdq_enabled(SMMUv3State *s)
>>>> +static inline bool smmuv3_cmdq_enabled(SMMUv3State *s, bool is_secure)
>>>>    {
>>>> -    return FIELD_EX32(s->cr[0], CR0, CMDQEN);
>>>> +    if (is_secure) {
>>>> +        return FIELD_EX32(s->secure_cr[0], S_CR0, CMDQEN);
>>>> +    } else {
>>>> +        return FIELD_EX32(s->cr[0], CR0, CMDQEN);
>>>> +    }
>>>>    }
> 
> 
>>> This looks like a reasonable and readable approach to support secure
>>> and non secure accesses.
>>
>> Hi Pierrick,
>>
>> Thank you so much for taking the time to review and for the very
>> positive feedback.
>>
>> I'm very relieved to hear you find the approach "reasonable and
>> readable". I was hoping that explicitly passing the parameter was the
>> right way to avoid issues with global state or code duplication, and
>> your confirmation is the best encouragement I could ask for.
> 
> An alternative (also suggested in patch #1) is to use index for banked
> registers.
> 
> For example we use M_REG_NS with Cortex-M cores (target/arm/cpu-qom.h):
> 
>       /* For M profile, some registers are banked secure vs non-secure;
>        * these are represented as a 2-element array where the first
>        * element is the non-secure copy and the second is the secure copy.
>        * When the CPU does not have implement the security extension then
>        * only the first element is used.
>        * This means that the copy for the current security state can be
>        * accessed via env->registerfield[env->v7m.secure] (whether the
>        * security extension is implemented or not).
>        */
>       enum {
>           M_REG_NS = 0,
>           M_REG_S = 1,
>           M_REG_NUM_BANKS = 2,
>       };
> 
> And generally for address spaces (target/arm/cpu.h):
> 
>       typedef enum ARMASIdx {
>           ARMASIdx_NS = 0,
>           ARMASIdx_S = 1,
>           ARMASIdx_TagNS = 2,
>           ARMASIdx_TagS = 3,
>       } ARMASIdx;
> 
> (not sure this one is appropriate here).
> 

This could be useful, especially to not duplicate register definitions.

However, for now, we only have two indexes (secure vs non-secure) and 
it's not clear if we'll need something more than that in a close future.
I think the current approach (a simple bool) is quite readable, and the 
boilerplate is limited, especially versus changing all existing code to 
support the new Idx approach.
As well, it can always be refactored easily later, when we'll need it.

That said, I will let the maintainers decide which approach they want to 
see implemented, to avoid Tao jumping between different people opinions.

Regards,
Pierrick