Re: [RFC v3 21/21] hw/arm/smmuv3: Add secure migration and enable secure state

[Eric Auger <eric.auger@redhat.com>]

On 10/12/25 5:16 PM, Tao Tang wrote:
> Introduce a bool secure_impl field to SMMUv3State and expose it as
> a secure-impl device property. The introduction of this property is the
> culminating step that activates the entire secure access data path,
> tying together all previously merged logic to provide full support for
> secure state accesses.
>
> Add live migration support for the SMMUv3 secure register bank.
>
> To correctly migrate the secure state, the migration logic must know
> if the secure functionality is enabled. To facilitate this, a bool
> secure_impl field is introduced and exposed as the secure-impl device
> property. This property is introduced at the point it is first
> required—for migration—and serves as the final piece of the series.
>
> The introduction of this property also completes and activates the
> entire secure access data path, tying together all previously merged
> logic to provide full support for secure state accesses.
>
> Usage:
>     -global arm-smmuv3,secure-impl=true
>
> When this property is enabled, the capability is advertised to the
> guest via the S_IDR1.SECURE_IMPL bit.
>
> The migration is implemented as follows:
>
> - A new vmstate_smmuv3_secure_bank, referenced by the smmuv3/bank_s
> subsection, serializes the secure bank's registers and queues.
>
> - A companion smmuv3/gbpa_secure subsection mirrors the non-secure
> GBPA handling, migrating the register only if its value diverges
> from the reset default.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
>  hw/arm/smmuv3.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/smmuv3.h |  1 +
>  2 files changed, 76 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 0b366895ec..ce41a12a36 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -337,6 +337,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>  
>      memset(sbk->idr, 0, sizeof(sbk->idr));
>      sbk->idr[1] = FIELD_DP32(sbk->idr[1], S_IDR1, S_SIDSIZE, SMMU_IDR1_SIDSIZE);
> +    sbk->idr[1] = FIELD_DP32(sbk->idr[1], S_IDR1, SECURE_IMPL, s->secure_impl);
>      sbk->gbpa = SMMU_GBPA_RESET_VAL;
>      sbk->cmdq.entry_size = sizeof(struct Cmd);
>      sbk->eventq.entry_size = sizeof(struct Evt);
> @@ -2452,6 +2453,53 @@ static const VMStateDescription vmstate_smmuv3_queue = {
>      },
>  };
>  
> +static const VMStateDescription vmstate_smmuv3_secure_bank = {
> +    .name = "smmuv3_secure_bank",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(features, SMMUv3RegBank),
> +        VMSTATE_UINT8(sid_split, SMMUv3RegBank),
> +        VMSTATE_UINT32_ARRAY(cr, SMMUv3RegBank, 3),
> +        VMSTATE_UINT32(cr0ack, SMMUv3RegBank),
> +        VMSTATE_UINT32(irq_ctrl, SMMUv3RegBank),
> +        VMSTATE_UINT32(gerror, SMMUv3RegBank),
> +        VMSTATE_UINT32(gerrorn, SMMUv3RegBank),
> +        VMSTATE_UINT64(gerror_irq_cfg0, SMMUv3RegBank),
> +        VMSTATE_UINT32(gerror_irq_cfg1, SMMUv3RegBank),
> +        VMSTATE_UINT32(gerror_irq_cfg2, SMMUv3RegBank),
> +        VMSTATE_UINT64(strtab_base, SMMUv3RegBank),
> +        VMSTATE_UINT32(strtab_base_cfg, SMMUv3RegBank),
> +        VMSTATE_UINT64(eventq_irq_cfg0, SMMUv3RegBank),
> +        VMSTATE_UINT32(eventq_irq_cfg1, SMMUv3RegBank),
> +        VMSTATE_UINT32(eventq_irq_cfg2, SMMUv3RegBank),
> +        VMSTATE_STRUCT(cmdq, SMMUv3RegBank, 0,
> +                       vmstate_smmuv3_queue, SMMUQueue),
> +        VMSTATE_STRUCT(eventq, SMMUv3RegBank, 0,
> +                       vmstate_smmuv3_queue, SMMUQueue),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static bool smmuv3_secure_bank_needed(void *opaque)
> +{
> +    SMMUv3State *s = opaque;
> +
> +    return s->secure_impl;
> +}
> +
> +static const VMStateDescription vmstate_smmuv3_bank_s = {
> +    .name = "smmuv3/bank_s",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = smmuv3_secure_bank_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT(bank[SMMU_SEC_SID_S], SMMUv3State, 0,
> +                       vmstate_smmuv3_secure_bank, SMMUv3RegBank),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
>  static bool smmuv3_gbpa_needed(void *opaque)
>  {
>      SMMUv3State *s = opaque;
> @@ -2472,6 +2520,25 @@ static const VMStateDescription vmstate_gbpa = {
>      }
>  };
>  
> +static bool smmuv3_gbpa_secure_needed(void *opaque)
I don't think you need that subsection. You can directly put this in the
secure subsection one. This was needed for NS to avoid breaking
migration but here you shall not need it.

Thanks

Eric
> +{
> +    SMMUv3State *s = opaque;
> +
> +    return s->secure_impl &&
> +           s->bank[SMMU_SEC_SID_S].gbpa != SMMU_GBPA_RESET_VAL;
> +}
> +
> +static const VMStateDescription vmstate_gbpa_secure = {
> +    .name = "smmuv3/gbpa_secure",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = smmuv3_gbpa_secure_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(bank[SMMU_SEC_SID_S].gbpa, SMMUv3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_smmuv3 = {
>      .name = "smmuv3",
>      .version_id = 1,
> @@ -2506,6 +2573,8 @@ static const VMStateDescription vmstate_smmuv3 = {
>      },
>      .subsections = (const VMStateDescription * const []) {
>          &vmstate_gbpa,
> +        &vmstate_smmuv3_bank_s,
> +        &vmstate_gbpa_secure,
>          NULL
>      }
>  };
> @@ -2519,6 +2588,12 @@ static const Property smmuv3_properties[] = {
>       * Defaults to stage 1
>       */
>      DEFINE_PROP_STRING("stage", SMMUv3State, stage),
> +    /*
> +     * SECURE_IMPL field in S_IDR1 register.
> +     * Indicates whether secure state is implemented.
> +     * Defaults to false (0)
> +     */
> +    DEFINE_PROP_BOOL("secure-impl", SMMUv3State, secure_impl, false),
>  };
>  
>  static void smmuv3_instance_init(Object *obj)
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index e9012fcdb0..8fec3f8edb 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -69,6 +69,7 @@ struct SMMUv3State {
>      qemu_irq     irq[4];
>      QemuMutex mutex;
>      char *stage;
> +    bool secure_impl;
>  };
>  
>  typedef enum {