Re: [RFC 01/11] hw/arm/smmuv3: Introduce secure registers and commands

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 6/8/25 17:11, Tao Tang wrote:
> The Arm SMMUv3 architecture defines a set of registers and commands for
> managing secure transactions and context.
> 
> This patch introduces the definitions for these secure registers and
> commands within the SMMUv3 device model internal header.
> 
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
>   hw/arm/smmuv3-internal.h | 57 ++++++++++++++++++++++++++++++++++++++++
>   include/hw/arm/smmuv3.h  | 23 ++++++++++++++++
>   2 files changed, 80 insertions(+)


> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index d183a62766..72ad042514 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -63,6 +63,29 @@ struct SMMUv3State {
>       qemu_irq     irq[4];
>       QemuMutex mutex;
>       char *stage;
> +
> +    /* Secure state */
> +    uint32_t secure_idr[5];
> +    uint32_t secure_cr[3];
> +    uint32_t secure_cr0ack;
> +    uint32_t secure_init;
> +    uint32_t secure_gbpa;
> +    uint32_t secure_irq_ctrl;
> +    uint32_t secure_gerror;
> +    uint32_t secure_gerrorn;
> +    uint64_t secure_gerror_irq_cfg0;
> +    uint32_t secure_gerror_irq_cfg1;
> +    uint32_t secure_gerror_irq_cfg2;
> +    uint64_t secure_strtab_base;
> +    uint32_t secure_strtab_base_cfg;
> +    uint8_t  secure_sid_split;
> +    uint32_t secure_features;
> +
> +    uint64_t secure_eventq_irq_cfg0;
> +    uint32_t secure_eventq_irq_cfg1;
> +    uint32_t secure_eventq_irq_cfg2;
> +
> +    SMMUQueue secure_eventq, secure_cmdq;

Note, we could also add these fields as

       struct {
           uint32_t idr[5];
           ...

       } secure;

With some IDEs it allows to only expand which set you are
interested in when debugging.

I then since it is mostly the same banked set, I wonder why we
don't extract the state and bank it:

       struct {
           uint32_t idr[5];
           ...

       } state[REG_NUM_BANKS];

I haven't looked at the rest, but this might simplify the
implementation.

Then maybe we can use the ARMASIdx enum as index.

>   };Shouldn't we add a subsection for these new fields in vmstate_smmuv3?

(If using banked state, then this is greatly simplified IMHO).

Regards,

Phil.