[v3] hw/arm/smmuv3: Add initial support for Secure State

[RFC v3 20/21] hw/arm/smmuv3: Initialize the secure register bank

Posted by Tao Tang 4 months ago

Initialize the secure register bank (SMMU_SEC_SID_S) with sane default
values during the SMMU's reset sequence.

This change ensures that key fields, such as the secure ID registers,
GBPA reset value, and queue entry sizes, are set to a known-good state.
The SECURE_IMPL attribute of the S_IDR1 register will be introduced
later via device properties.

This is a necessary step to prevent undefined behavior when secure SMMU
features are subsequently enabled and used by software.

Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
---
 hw/arm/smmuv3.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index b44859540f..0b366895ec 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -331,6 +331,15 @@ static void smmuv3_init_regs(SMMUv3State *s)
     bk->gerrorn = 0;
     s->statusr = 0;
     bk->gbpa = SMMU_GBPA_RESET_VAL;
+
+    /* Initialize Secure bank */
+    SMMUv3RegBank *sbk = &s->bank[SMMU_SEC_SID_S];
+
+    memset(sbk->idr, 0, sizeof(sbk->idr));
+    sbk->idr[1] = FIELD_DP32(sbk->idr[1], S_IDR1, S_SIDSIZE, SMMU_IDR1_SIDSIZE);
+    sbk->gbpa = SMMU_GBPA_RESET_VAL;
+    sbk->cmdq.entry_size = sizeof(struct Cmd);
+    sbk->eventq.entry_size = sizeof(struct Evt);
 }
 
 static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
-- 
2.34.1

Re: [RFC v3 20/21] hw/arm/smmuv3: Initialize the secure register bank

Posted by Eric Auger 2 months, 1 week ago

Hi Tao,

On 10/12/25 5:15 PM, Tao Tang wrote:
> Initialize the secure register bank (SMMU_SEC_SID_S) with sane default
> values during the SMMU's reset sequence.
>
> This change ensures that key fields, such as the secure ID registers,
> GBPA reset value, and queue entry sizes, are set to a known-good state.
> The SECURE_IMPL attribute of the S_IDR1 register will be introduced
> later via device properties.
>
> This is a necessary step to prevent undefined behavior when secure SMMU
> features are subsequently enabled and used by software.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
>  hw/arm/smmuv3.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index b44859540f..0b366895ec 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -331,6 +331,15 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      bk->gerrorn = 0;
>      s->statusr = 0;
>      bk->gbpa = SMMU_GBPA_RESET_VAL;
> +
> +    /* Initialize Secure bank */
> +    SMMUv3RegBank *sbk = &s->bank[SMMU_SEC_SID_S];
> +
> +    memset(sbk->idr, 0, sizeof(sbk->idr));
> +    sbk->idr[1] = FIELD_DP32(sbk->idr[1], S_IDR1, S_SIDSIZE, SMMU_IDR1_SIDSIZE);
> +    sbk->gbpa = SMMU_GBPA_RESET_VAL;
> +    sbk->cmdq.entry_size = sizeof(struct Cmd);
> +    sbk->eventq.entry_size = sizeof(struct Evt);
what about prod, cons, base? Don't they need to initialized as for NS.

Also I am surprised only one IDR field is set. No need for some others?

Eric
>  }
>  
>  static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,

Re: [RFC v3 20/21] hw/arm/smmuv3: Initialize the secure register bank

Posted by Tao Tang 2 months, 1 week ago

Hi Eric,

On 2025/12/3 00:36, Eric Auger wrote:
> Hi Tao,
>
> On 10/12/25 5:15 PM, Tao Tang wrote:
>> Initialize the secure register bank (SMMU_SEC_SID_S) with sane default
>> values during the SMMU's reset sequence.
>>
>> This change ensures that key fields, such as the secure ID registers,
>> GBPA reset value, and queue entry sizes, are set to a known-good state.
>> The SECURE_IMPL attribute of the S_IDR1 register will be introduced
>> later via device properties.
>>
>> This is a necessary step to prevent undefined behavior when secure SMMU
>> features are subsequently enabled and used by software.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> ---
>>   hw/arm/smmuv3.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index b44859540f..0b366895ec 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -331,6 +331,15 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>       bk->gerrorn = 0;
>>       s->statusr = 0;
>>       bk->gbpa = SMMU_GBPA_RESET_VAL;
>> +
>> +    /* Initialize Secure bank */
>> +    SMMUv3RegBank *sbk = &s->bank[SMMU_SEC_SID_S];
>> +
>> +    memset(sbk->idr, 0, sizeof(sbk->idr));
>> +    sbk->idr[1] = FIELD_DP32(sbk->idr[1], S_IDR1, S_SIDSIZE, SMMU_IDR1_SIDSIZE);
>> +    sbk->gbpa = SMMU_GBPA_RESET_VAL;
>> +    sbk->cmdq.entry_size = sizeof(struct Cmd);
>> +    sbk->eventq.entry_size = sizeof(struct Evt);
> what about prod, cons, base? Don't they need to initialized as for NS.
>
> Also I am surprised only one IDR field is set. No need for some others?
>
> Eric


I’ll add the missing initializations in the next version, and will try 
to keep the secure and non-secure banks as close to mirrored as 
possible, and only diverging where a field doesn’t exist on one side.


Thanks,

Tao