On 8/13/26 6:25 PM, 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 <[email protected]>
> ---
> 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)
do we need that change given secure S2 is not implemented yet. Can't we
focus on secure S1 first?
> {
> 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);
> cfg->asid = asid;
> cfg->stage = SMMU_NESTED;
>
> @@ -524,6 +525,7 @@ static inline int translate_table_addr_ipa(SMMUState *bs,
> * @perm: access type
> * @tlbe: SMMUTLBEntry (out)
> * @info: handle to an error info
> + * @sec_sid: StreamID Security state
> *
> * Return 0 on success, < 0 on error. In case of error, @info is filled
> * and tlbe->perm is set to IOMMU_NONE.
> @@ -532,12 +534,16 @@ static inline int translate_table_addr_ipa(SMMUState
> *bs,
> */
> static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
> dma_addr_t iova, IOMMUAccessFlags perm,
> - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info,
> + SMMUSecSID sec_sid)
> {
> dma_addr_t baseaddr, indexmask;
> SMMUStage stage = cfg->stage;
> SMMUTransTableInfo *tt = select_tt(cfg, iova);
> uint8_t level, granule_sz, inputsize, stride;
> + int nscfg, current_ns, new_nstable;
> + bool sid_is_ns = sec_sid == SMMU_SEC_SID_NS;
prefer !smmu_sec_sid_is_secure()?
> + SMMUSecSID table_sec_sid;
>
> if (!tt || tt->disabled) {
> info->type = SMMU_PTW_ERR_TRANSLATION;
> @@ -552,6 +558,7 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg
> *cfg,
>
> baseaddr = extract64(tt->ttb, 0, cfg->oas);
> baseaddr &= ~indexmask;
> + nscfg = tt->nscfg;
>
> while (level < VMSA_LEVELS) {
> uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> @@ -560,8 +567,19 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg
> *cfg,
> uint64_t pte, gpa;
> dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> uint8_t ap;
> + AddressSpace *pte_as;
> + MemTxAttrs pte_attrs;
> + SMMUSecSID cur_sec_sid;
>
> - if (get_pte(baseaddr, offset, &pte, info)) {
> + /*
> + * Start in NS for Non-secure streams or CD.NSCFGx == 1.
May I suggest something like:
at starting level we start fetching CD/TTBx in NS if non-secure SIDs of
if CD.NSCFGx == 1. Once we start fetching in NS space, then we continue
the PTW in next levels in NS space.
> + * Once walk is in NS, NSTable is ignored on subsequent levels.
> + */
> + current_ns = sid_is_ns || nscfg;
> + table_sec_sid = current_ns ? SMMU_SEC_SID_NS : sec_sid;
> + pte_as = smmu_get_address_space(bs, table_sec_sid);
> + pte_attrs = smmu_get_txattrs(table_sec_sid);
> + if (get_pte(baseaddr, offset, &pte, info, pte_as, pte_attrs)) {
> goto error;
> }
> trace_smmu_ptw_level(stage, level, iova, subpage_size,
> @@ -582,10 +600,25 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg
> *cfg,
> }
> baseaddr = get_table_pte_address(pte, granule_sz);
> if (cfg->stage == SMMU_NESTED) {
> - if (translate_table_addr_ipa(bs, &baseaddr, cfg, info)) {
> + if (translate_table_addr_ipa(bs, &baseaddr, cfg,
> + info, table_sec_sid)) {
> goto error;
> }
> }
> +
> + /*
> + * NSTable can switch the walk to NS only while the current walk
> + * level is Secure. Once switched to NS, NSTable is ignored
> according
> + * to hierarchical control of Secure/Non-secure accesses:
> + * (IHI 0070G.b)13.4.1 Stage 1 page permissions and
> + * (DDI 0487H.a)D8.4.2 Control of Secure or Non-secure memory
> access
> + */
above suggested comment may be sufficient
> + if (!current_ns) {
> + new_nstable = PTE_NSTABLE(pte);
not sure you need new_nstable local variable
> + nscfg = new_nstable ? 1 : 0;
and I think you can directly assign nscfg to PTE_NSTABLE(pte), no?
> + } else {
> + nscfg = 1;
> + }
> level++;
> continue;
> } else if (is_page_pte(pte, level)) {
> @@ -628,6 +661,12 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg
> *cfg,
> goto error;
> }
>
> + if (current_ns) {
> + cur_sec_sid = SMMU_SEC_SID_NS;
> + } else {
> + cur_sec_sid = PTE_NS(pte) ? SMMU_SEC_SID_NS : SMMU_SEC_SID_S;
> + }
> + tlbe->entry.target_as = smmu_get_address_space(bs, cur_sec_sid);
> tlbe->entry.translated_addr = gpa;
> tlbe->entry.iova = iova & ~mask;
> tlbe->entry.addr_mask = mask;
> @@ -697,7 +736,10 @@ static int smmu_ptw_64_s2(SMMUState *bs, SMMUTransCfg
> *cfg,
> uint64_t pte, gpa;
> dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> uint8_t s2ap;
> - if (get_pte(baseaddr, offset, &pte, info)) {
> + AddressSpace *pte_as = &bs->memory_as;
> + MemTxAttrs pte_attrs = MEMTXATTRS_UNSPECIFIED;
same question here, why do we care about smmu_ptw_64_s() atm
> +
> + if (get_pte(baseaddr, offset, &pte, info, pte_as, pte_attrs)) {
> goto error;
> }
> trace_smmu_ptw_level(stage, level, ipa, subpage_size,
> @@ -792,7 +834,7 @@ static void combine_tlb(SMMUTLBEntry *tlbe, SMMUTLBEntry
> *tlbe_s2,
> }
>
> /**
> - * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
> + * smmu_ptw - Walk the page tables for an IOVA, according to @cfg and
> @sec_sid
> *
> * @bs: smmu state which includes TLB instance
> * @cfg: translation configuration
> @@ -800,18 +842,20 @@ static void combine_tlb(SMMUTLBEntry *tlbe,
> SMMUTLBEntry *tlbe_s2,
> * @perm: tentative access type
> * @tlbe: returned entry
> * @info: ptw event handle
> + * @sec_sid: StreamID Security state
> *
> * return 0 on success
> */
> int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
> - IOMMUAccessFlags perm, SMMUTLBEntry *tlbe, SMMUPTWEventInfo
> *info)
> + IOMMUAccessFlags perm, SMMUTLBEntry *tlbe, SMMUPTWEventInfo
> *info,
> + SMMUSecSID sec_sid)
> {
> int ret;
> SMMUTLBEntry tlbe_s2;
> dma_addr_t ipa;
>
> if (cfg->stage == SMMU_STAGE_1) {
> - return smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info);
> + return smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info, sec_sid);
> } else if (cfg->stage == SMMU_STAGE_2) {
> /*
> * If bypassing stage 1(or unimplemented), the input address is
> passed
> @@ -830,7 +874,7 @@ int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t
> iova,
> }
>
> /* SMMU_NESTED. */
> - ret = smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info);
> + ret = smmu_ptw_64_s1(bs, cfg, iova, perm, tlbe, info, sec_sid);
> if (ret) {
> return ret;
> }
> @@ -846,7 +890,8 @@ int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t
> iova,
> }
>
> SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t
> addr,
> - IOMMUAccessFlags flag, SMMUPTWEventInfo *info)
> + IOMMUAccessFlags flag, SMMUPTWEventInfo *info,
> + SMMUSecSID sec_sid)
> {
> SMMUTLBEntry *cached_entry = NULL;
> SMMUTransTableInfo *tt;
> @@ -888,7 +933,7 @@ SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg
> *cfg, dma_addr_t addr,
> }
>
> cached_entry = g_new0(SMMUTLBEntry, 1);
> - status = smmu_ptw(bs, cfg, addr, flag, cached_entry, info);
> + status = smmu_ptw(bs, cfg, addr, flag, cached_entry, info, sec_sid);
> if (status) {
> g_free(cached_entry);
> return NULL;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 6b34f76f11b..cc5d3ab696c 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -394,7 +394,8 @@ static SMMUTranslationStatus
> smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> SMMUEventInfo *event,
> IOMMUAccessFlags flag,
> SMMUTLBEntry **out_entry,
> - SMMUTranslationClass class);
> + SMMUTranslationClass class,
> + SMMUSecSID sec_sid);
> /* @ssid > 0 not supported yet */
> static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg,
> uint32_t ssid, CD *buf, SMMUEventInfo *event,
> @@ -411,7 +412,7 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste,
> SMMUTransCfg *cfg,
>
> if (cfg->stage == SMMU_NESTED) {
> status = smmuv3_do_translate(s, addr, cfg, event,
> - IOMMU_RO, &entry, SMMU_CLASS_CD);
> + IOMMU_RO, &entry, SMMU_CLASS_CD,
> sec_sid);
>
> /* Same PTW faults are reported but with CLASS = CD. */
> if (status != SMMU_TRANS_SUCCESS) {
> @@ -796,7 +797,7 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> SMMUEventInfo *event,
> }
>
> static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> - CD *cd, SMMUEventInfo *event)
> + CD *cd, SMMUEventInfo *event, SMMUSecSID sec_sid)
> {
> int ret = -EINVAL;
> int i;
> @@ -869,7 +870,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> /* Translate the TTBx, from IPA to PA if nesting is enabled. */
> if (cfg->stage == SMMU_NESTED) {
> status = smmuv3_do_translate(s, tt->ttb, cfg, event, IOMMU_RO,
> - &entry, SMMU_CLASS_TT);
> + &entry, SMMU_CLASS_TT, sec_sid);
in the context secure S2 is not supported, can sec_sid be set here? In
other words, do we really need to add sed_sid arg to decode_cd() at that
stage or can we just invoke with ns and add a comment?
> /*
> * Same PTW faults are reported but with CLASS = TT.
> * If TTBx is larger than the effective stage 1 output addres
> @@ -939,7 +940,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr,
> SMMUTransCfg *cfg,
> return ret;
> }
>
> - return decode_cd(s, cfg, &cd, event);
> + return decode_cd(s, cfg, &cd, event, sec_sid);
> }
>
> /**
> @@ -1004,7 +1005,8 @@ static SMMUTranslationStatus
> smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> SMMUEventInfo *event,
> IOMMUAccessFlags flag,
> SMMUTLBEntry **out_entry,
> - SMMUTranslationClass class)
> + SMMUTranslationClass class,
> + SMMUSecSID sec_sid)
> {
> SMMUPTWEventInfo ptw_info = {};
> SMMUState *bs = ARM_SMMU(s);
> @@ -1030,7 +1032,7 @@ static SMMUTranslationStatus
> smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> cfg->stage = SMMU_STAGE_2;
> }
>
> - cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info);
> + cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info, sec_sid);
>
> if (desc_s2_translation) {
> cfg->asid = asid;
> @@ -1176,13 +1178,14 @@ static IOMMUTLBEntry
> smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> }
>
> status = smmuv3_do_translate(s, addr, cfg, &event, flag,
> - &cached_entry, SMMU_CLASS_IN);
> + &cached_entry, SMMU_CLASS_IN, sec_sid);
>
> epilogue:
> qemu_mutex_unlock(&s->mutex);
> switch (status) {
> case SMMU_TRANS_SUCCESS:
> entry.perm = cached_entry->entry.perm;
> + entry.target_as = cached_entry->entry.target_as;
> entry.translated_addr = CACHED_ENTRY_TO_ADDR(cached_entry, addr);
> entry.addr_mask = cached_entry->entry.addr_mask;
> trace_smmuv3_translate_success(mr->parent_obj.name, sid, addr,
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 1aa4e3208da..0c5718ea684 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -224,18 +224,19 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
>
> /**
> * smmu_ptw - Perform the page table walk for a given iova / access flags
> - * pair, according to @cfg translation config
> + * pair, according to @cfg translation config and @sec_sid
> */
> int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
> IOMMUAccessFlags perm, SMMUTLBEntry *tlbe,
> - SMMUPTWEventInfo *info);
> + SMMUPTWEventInfo *info, SMMUSecSID sec_sid);
>
> /*
> * smmu_translate - Look for a translation in TLB, if not, do a PTW.
> * Returns NULL on PTW error or incase of TLB permission errors.
> */
> SMMUTLBEntry *smmu_translate(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t
> addr,
> - IOMMUAccessFlags flag, SMMUPTWEventInfo *info);
> + IOMMUAccessFlags flag, SMMUPTWEventInfo *info,
> + SMMUSecSID sec_sid);
>
> /**
> * select_tt - compute which translation table shall be used according to
Thanks
Eric
