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 <tangtao1634@phytium.com.cn>
>> ---
>> 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
>>