Hi Eric,
On 2025/9/29 23:42, Eric Auger wrote:
On 9/26/25 5:23 AM, Tao Tang wrote:
My apologies, resending patches 13-14/14 to fix a threading mistake from
my previous attempt.
This commit completes the initial implementation of the Secure SMMUv3
model by making the feature user-configurable.
A new boolean property, "secure-impl", is introduced to the device.
This property defaults to false, ensuring backward compatibility for
existing machine types that do not expect the secure programming
interface.
When "secure-impl" is set to true, the smmuv3_init_regs function now
initializes the secure register bank (bank[SMMU_SEC_IDX_S]). Crucially,
the S_IDR1.SECURE_IMPL bit is set according to this property,
correctly advertising the presence of the secure functionality to the
guest.
This patch ties together all the previous refactoring work. With this
property enabled, the banked registers, security-aware queues, and
other secure features become active, allowing a guest to probe and
configure the Secure SMMU.
Signed-off-by: Tao Tang <[email protected]>
---
hw/arm/smmuv3.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index c92cc0f06a..80fbc25cf5 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -351,6 +351,16 @@ static void smmuv3_init_regs(SMMUv3State *s)
s->statusr = 0;
s->bank[SMMU_SEC_IDX_NS].gbpa = SMMU_GBPA_RESET_VAL;
+ /* Initialize Secure bank (SMMU_SEC_IDX_S) */
same comment as before, use a local pointer to the secure bank.
Of course, I will fix the code style by using a local pointer for the
secure bank access.
+ memset(s->bank[SMMU_SEC_IDX_S].idr, 0,
sizeof(s->bank[SMMU_SEC_IDX_S].idr));
+ s->bank[SMMU_SEC_IDX_S].idr[1] = FIELD_DP32(s->bank[SMMU_SEC_IDX_S].idr[1],
+ S_IDR1, SECURE_IMPL,
+ s->secure_impl);
+ s->bank[SMMU_SEC_IDX_S].idr[1] = FIELD_DP32(
+ s->bank[SMMU_SEC_IDX_S].idr[1], IDR1, SIDSIZE, SMMU_IDR1_SIDSIZE);
+ s->bank[SMMU_SEC_IDX_S].gbpa = SMMU_GBPA_RESET_VAL;
+ s->bank[SMMU_SEC_IDX_S].cmdq.entry_size = sizeof(struct Cmd);
+ s->bank[SMMU_SEC_IDX_S].eventq.entry_size = sizeof(struct Evt);
}
static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
@@ -2505,6 +2515,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),
};
I would introduce the secure-impl property at the very end of the series
because at this point migration is not yet supported.
By the way the secure_impl field can be introduced in the first which
uses it. I don't think "hw/arm/smmuv3: Introduce banked registers for
SMMUv3 state" does
Thanks
Eric
You are absolutely right. Introducing the "secure-impl" property before
the migration support is complete would expose an unfinished feature and
could lead to serious issues for users. It was a mistake to place it here.
And secure_impl field is only used once at init to set the SECURE_IMPL
register bit. After that, all checks correctly use the register bit, not
the field.
I understand this is inconsistent. In the next version, I will make the
logic more direct to improve readability.
Thanks,
Tao
static void smmuv3_instance_init(Object *obj)
--
2.34.1
