Re: [RFC v4 30/31] hw/arm/smmuv3: Add secure bank migration and secure-impl property

[Tao Tang]

Hi Pierrick,

On 2026/2/26 06:20, Pierrick Bouvier wrote:
On 2/21/26 2:19 AM, Tao Tang wrote:
Add a secure-impl device property and advertise it through
S_IDR1.SECURE_IMPL.
Usage:
     -global arm-smmuv3,secure-impl=true

Add the smmuv3/bank_s migration subsection for the secure register bank.
Serialize secure bank state including GBPA, IRQ config, stream table and
queue state.

Signed-off-by: Tao Tang <tangtao1...@phytium.com.cn>
---
  hw/arm/smmuv3.c         | 56 +++++++++++++++++++++++++++++++++++++++++
  include/hw/arm/smmuv3.h |  2 ++
  2 files changed, 58 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index f0fbc5fc96b..678cbd584e2 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -327,6 +327,7 @@ static void smmuv3_init_id_regs(SMMUv3State *s)
      memset(sbk->idr, 0, sizeof(sbk->idr));
      sbk->idr[0] = FIELD_DP32(bk->idr[0], S_IDR0, STALL_MODEL, 1); 
/* No stall */
      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);
      smmuv3_accel_idr_override(s);
  }
  @@ -2632,6 +2633,54 @@ 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(gbpa, 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;
@@ -2686,6 +2735,7 @@ static const VMStateDescription vmstate_smmuv3 = {
      },
      .subsections = (const VMStateDescription * const []) {
          &vmstate_gbpa,
+        &vmstate_smmuv3_bank_s,
          NULL
      }
  };
@@ -2707,6 +2757,12 @@ static const Property smmuv3_properties[] = {
      DEFINE_PROP_BOOL("ats", SMMUv3State, ats, false),
      DEFINE_PROP_UINT8("oas", SMMUv3State, oas, 44),
      DEFINE_PROP_UINT8("ssidsize", SMMUv3State, ssidsize, 0),
+    /*
+     * 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),
  };


As mentioned before, should we enable it automatically in case 
secure-memory address space is available at realize time?


Let me take this opportunity to address the same concern you raised 
across patches #07, #17, and #30 on the secure-impl vs secure-memory topic.


My initial intent was a demand-driven (lazy) validation model — i.e. 
only validate Secure capability when a device actually declares 
sec-sid=secure and we go through smmu_init_sdev() / 
smmuv3_validate_sec_sid(). That avoids forcing Secure-related 
configuration checks on machines that never exercise Secure DMA.

That said, I agree with your point that secure-impl=on without a 
corresponding secure-memory link is an unconditional misconfiguration 
and should fail fast at realize time. For v5 I’ll combine both approaches:

1) Realize-time inference + fail-fast

- If a secure-memory address space is wired at realize time, the SMMU 
should “do the right thing out of the box” and enable Secure support 
automatically.

- If Secure support ends up enabled (either explicitly or via 
auto-inference) but secure-memory is missing, smmuv3_realize() will 
error out with a clear message.

Importantly, I’ll preserve the explicit user override you mentioned 
(-global arm-smmuv3.secure-impl=off).


2) Make helpers assume the invariant

With the realize-time invariant in place, the Secure branch in 
smmu_get_address_space() will no longer “warn + return NULL”; it will 
become an assert (or equivalent hard failure) because the machine 
configuration is already validated.


3) Keep the device-level guard

I will keep smmuv3_validate_sec_sid() as a complementary per-device 
guard. This still matters when the user forces secure-impl=off: if a 
device declares sec-sid=secure, we can reject it immediately with a 
targeted error at device init/hotplug time, which is a different 
granularity than the SMMU-wide realize check.


I’ll incorporate these changes in the next revision.



  static void smmuv3_instance_init(Object *obj)
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index d07bdfa1f27..79ce7c754c4 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -78,6 +78,8 @@ struct SMMUv3State {
      bool ats;
      uint8_t oas;
      uint8_t ssidsize;
+
+    bool secure_impl;
  };
    typedef enum {

Otherwise, congrats for the great series!

Thanks for your time!

Tao


Reviewed-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>