Re: [RFC v5 11/28] hw/arm/smmu-common: Implement secure state handling in ptw

[Tao Tang <tangtao1634@phytium.com.cn>]

Hi Mostafa,

On 2026/9/1 22:09, Mostafa Saleh wrote:
> On Fri, Aug 14, 2026 at 12:25:08AM +0800, Tao Tang wrote:
>> Enhance the page table walker to correctly handle secure and non-secure
>> memory accesses. This change introduces logic to select the appropriate
>> address space and enforce architectural security policies during walks.
>>
>> The page table walker now correctly processes Secure Stage 1
>> translations. Key changes include:
>>
>> - The get_pte() function now uses the effective security state to fetch
>> page-table entries from either the Secure or Non-secure address space,
>> with explicit transaction attributes matching that address space.
>>
>> - The stage 1 walker tracks the security state, respecting the NSCFG
>> and NSTable attributes. It correctly handles the hierarchical security
>> model: if a table descriptor in a secure walk has NSTable=1, all
>> subsequent lookups for that walk are forced into the Non-secure space.
>> This is a one-way transition, as specified by the architecture.
>>
>> - The final TLB entry is tagged with the correct output address space,
>> ensuring proper memory isolation.
>>
>> Note: We do not yet support secure stage 2 translations. This patch
>> only implements Secure stage 1 page-table walks. Baseline propagation
>> of the incoming NS attribute for stage 1 bypass is handled separately
>> in this series with ATTR_PERMS_OVR == 0. Full ATTR_PERMS_OVR support
>> is left for a separate series.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> ---
>>   hw/arm/smmu-common.c         | 73 +++++++++++++++++++++++++++++-------
>>   hw/arm/smmuv3.c              | 19 ++++++----
>>   include/hw/arm/smmu-common.h |  7 ++--
>>   3 files changed, 74 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 63c36329a98..317cfafded2 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -408,13 +408,13 @@ void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
>>    * @base_addr[@index]
>>    */
>>   static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
>> -                   SMMUPTWEventInfo *info)
>> +                   SMMUPTWEventInfo *info, AddressSpace *as, MemTxAttrs attrs)
>>   {
>>       int ret;
>>       dma_addr_t addr = baseaddr + index * sizeof(*pte);
>>   
>>       /* TODO: guarantee 64-bit single-copy atomicity */
>> -    ret = ldq_le_dma(&address_space_memory, addr, pte, MEMTXATTRS_UNSPECIFIED);
>> +    ret = ldq_le_dma(as, addr, pte, attrs);
>>   
>>       if (ret != MEMTX_OK) {
>>           info->type = SMMU_PTW_ERR_WALK_EABT;
>> @@ -488,7 +488,8 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>>   static inline int translate_table_addr_ipa(SMMUState *bs,
>>                                              dma_addr_t *table_addr,
>>                                              SMMUTransCfg *cfg,
>> -                                           SMMUPTWEventInfo *info)
>> +                                           SMMUPTWEventInfo *info,
>> +                                           SMMUSecSID sec_sid)
>>   {
>>       dma_addr_t addr = *table_addr;
>>       SMMUTLBEntry *cached_entry;
>> @@ -501,7 +502,7 @@ static inline int translate_table_addr_ipa(SMMUState *bs,
>>       asid = cfg->asid;
>>       cfg->stage = SMMU_STAGE_2;
>>       cfg->asid = -1;
>> -    cached_entry = smmu_translate(bs, cfg, addr, IOMMU_RO, info);
>> +    cached_entry = smmu_translate(bs, cfg, addr, IOMMU_RO, info, sec_sid);
> Should we have an assertion instead as it is not possible to reach
> this path with the secure bit?
>
> Thanks,
> Mostafa


Thanks for the suggestion! Eric suggested avoiding the extra sec_sid 
plumbing while Secure stage 2 is unsupported, so in v6 I want to remove 
the sec_sid parameter from translate_table_addr_ipa() and explicitly 
pass SMMU_SEC_SID_NS to smmu_translate() inside the helper. Since 
decode_ste() already returns C_BAD_STE for Secure STEs with stage 2 
enabled, including nested translation, could we omit this assertion for 
now? What do you think?

Best regards,
Tao