Re: [PATCH v2 14/14] hw/arm/smmuv3: Optional Secure bank migration via subsections

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

Hi Tao,

On 9/26/25 5:30 AM, Tao Tang wrote:
> My apologies, resending patches 13-14/14 to fix a threading mistake from
> my previous attempt.
>
> Introduce a generic vmstate_smmuv3_bank that serializes a single SMMUv3
> bank (registers and queues). Add a 'smmuv3/bank_s' subsection guarded by
> secure_impl and a new 'migrate-secure-bank' property; when enabled, the S
> bank is migrated. Add a 'smmuv3/gbpa_secure' subsection which is only sent
> when GBPA differs from its reset value.
>
> This keeps the existing migration stream unchanged by default and remains
> backward compatible: older QEMUs can ignore unknown subsections, and with
> 'migrate-secure-bank' defaulting to off, the stream is identical to before.
>
> This also prepares for future RME extensions (Realm/Root) by reusing the
> bank subsection pattern.
>
> Usage:
>   -global arm-smmuv3,secure-impl=on,migrate-secure-bank=on
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
>  hw/arm/smmuv3.c         | 70 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/smmuv3.h |  1 +
>  2 files changed, 71 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 80fbc25cf5..2a1e80d179 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -2450,6 +2450,53 @@ static const VMStateDescription vmstate_smmuv3_queue = {
>      },
>  };
>
> +static const VMStateDescription vmstate_smmuv3_bank = {
I would name this vmstate_smmuv3_secure_bank
> +    .name = "smmuv3_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 && s->migrate_secure_bank;
why is it needed to check s->migrate_secure_bank?
> +}
> +
> +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_IDX_S], SMMUv3State, 0,
> +                       vmstate_smmuv3_bank, SMMUv3RegBank),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
>  static bool smmuv3_gbpa_needed(void *opaque)
>  {
>      SMMUv3State *s = opaque;
> @@ -2469,6 +2516,25 @@ static const VMStateDescription vmstate_gbpa = {
>      }
>  };
>
> +static bool smmuv3_gbpa_secure_needed(void *opaque)
> +{
> +    SMMUv3State *s = opaque;
> +
> +    return s->secure_impl && s->migrate_secure_bank &&
same
> +           s->bank[SMMU_SEC_IDX_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_IDX_S].gbpa, SMMUv3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_smmuv3 = {
>      .name = "smmuv3",
>      .version_id = 1,
> @@ -2502,6 +2568,8 @@ static const VMStateDescription vmstate_smmuv3 = {
>      },
>      .subsections = (const VMStateDescription * const []) {
>          &vmstate_gbpa,
> +        &vmstate_smmuv3_bank_s,
> +        &vmstate_gbpa_secure,
>          NULL
>      }
>  };
> @@ -2521,6 +2589,8 @@ static const Property smmuv3_properties[] = {
>       * Defaults to false (0)
>       */
>      DEFINE_PROP_BOOL("secure-impl", SMMUv3State, secure_impl, false),
> +    DEFINE_PROP_BOOL("migrate-secure-bank", SMMUv3State,
> +                     migrate_secure_bank, false),
I don't get why you need another migrate_secure_bank prop. You need to
migrate the subsection if secure_impl is implemented, don't you?
>  };
>
>  static void smmuv3_instance_init(Object *obj)
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index 572f15251e..5ffb609fa2 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -71,6 +71,7 @@ struct SMMUv3State {
>      QemuMutex mutex;
>      char *stage;
>      bool secure_impl;
> +    bool migrate_secure_bank;
>  };
>
>  typedef enum {
> --
> 2.34.1
>
Eric