This patch introduces the necessary logic to handle security states
during the page table translation process.
Support for the NS (Non-secure) attribute bit is added to the parsing of
various translation structures, including CD and PTEs. This allows the
SMMU model to correctly determine the security properties of memory
during a translation.
With this change, a new translation stage is added:
- Secure Stage 1 translation
Note that this commit does not include support for Secure Stage 2
translation, which will be addressed in the future.
Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
---
hw/arm/smmu-common.c | 55 ++++++++++++++++++++++++++++++++----
hw/arm/smmu-internal.h | 7 +++++
hw/arm/smmuv3-internal.h | 2 ++
hw/arm/smmuv3.c | 2 ++
hw/arm/trace-events | 2 +-
include/hw/arm/smmu-common.h | 4 +++
6 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index bc13b00f1d..f563cba023 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -398,20 +398,25 @@ 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, SMMUTransCfg *cfg, int walk_ns)
{
int ret;
dma_addr_t addr = baseaddr + index * sizeof(*pte);
+ /* Only support Secure PA Space as RME isn't implemented yet */
+ MemTxAttrs attrs =
+ smmu_get_txattrs(walk_ns ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S);
+ AddressSpace *as =
+ smmu_get_address_space(walk_ns ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S);
/* 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;
info->addr = addr;
return -EINVAL;
}
- trace_smmu_get_pte(baseaddr, index, addr, *pte);
+ trace_smmu_get_pte(baseaddr, index, addr, *pte, walk_ns);
return 0;
}
@@ -542,6 +547,8 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
baseaddr = extract64(tt->ttb, 0, cfg->oas);
baseaddr &= ~indexmask;
+ int nscfg = tt->nscfg;
+ bool forced_ns = false; /* Track if NSTable=1 forced NS mode */
while (level < VMSA_LEVELS) {
uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
@@ -551,7 +558,9 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
uint8_t ap;
- if (get_pte(baseaddr, offset, &pte, info)) {
+ /* Use NS if forced by previous NSTable=1 or current nscfg */
+ int current_ns = forced_ns || nscfg;
+ if (get_pte(baseaddr, offset, &pte, info, cfg, current_ns)) {
goto error;
}
trace_smmu_ptw_level(stage, level, iova, subpage_size,
@@ -576,6 +585,26 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
goto error;
}
}
+
+ /*
+ * Hierarchical control of Secure/Non-secure accesses:
+ * If NSTable=1 from Secure space, force all subsequent lookups to
+ * Non-secure space and ignore future NSTable according to
+ * (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
+ */
+ if (!forced_ns) {
+ int new_nstable = PTE_NSTABLE(pte);
+ if (!current_ns && new_nstable) {
+ /* First transition from Secure to Non-secure */
+ forced_ns = true;
+ nscfg = 1;
+ } else if (!forced_ns) {
+ /* Still in original mode, update nscfg normally */
+ nscfg = new_nstable;
+ }
+ /* If forced_ns is already true, ignore NSTable bit */
+ }
level++;
continue;
} else if (is_page_pte(pte, level)) {
@@ -618,6 +647,8 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
goto error;
}
+ tlbe->sec_idx = PTE_NS(pte) ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S;
+ tlbe->entry.target_as = smmu_get_address_space(tlbe->sec_idx);
tlbe->entry.translated_addr = gpa;
tlbe->entry.iova = iova & ~mask;
tlbe->entry.addr_mask = mask;
@@ -687,7 +718,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
uint8_t s2ap;
- if (get_pte(baseaddr, offset, &pte, info)) {
+ /* Use NS as Secure Stage 2 is not implemented (SMMU_S_IDR1.SEL2 == 0)*/
+ if (get_pte(baseaddr, offset, &pte, info, cfg, 1)) {
goto error;
}
trace_smmu_ptw_level(stage, level, ipa, subpage_size,
@@ -740,6 +772,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
goto error_ipa;
}
+ tlbe->sec_idx = SMMU_SEC_IDX_NS;
+ tlbe->entry.target_as = &address_space_memory;
tlbe->entry.translated_addr = gpa;
tlbe->entry.iova = ipa & ~mask;
tlbe->entry.addr_mask = mask;
@@ -824,6 +858,17 @@ int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
return ret;
}
+ if (!cfg->sel2 && tlbe->sec_idx > SMMU_SEC_IDX_NS) {
+ /*
+ * Nested translation with Secure IPA output is not supported if
+ * Secure Stage 2 is not implemented.
+ */
+ info->type = SMMU_PTW_ERR_TRANSLATION;
+ info->stage = SMMU_STAGE_1;
+ tlbe->entry.perm = IOMMU_NONE;
+ return -EINVAL;
+ }
+
ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
if (ret) {
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index d143d296f3..cb3a6eb8d1 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -58,6 +58,10 @@
((level == 3) && \
((pte & ARM_LPAE_PTE_TYPE_MASK) == ARM_LPAE_L3_PTE_TYPE_PAGE))
+/* Non-secure bit */
+#define PTE_NS(pte) \
+ (extract64(pte, 5, 1))
+
/* access permissions */
#define PTE_AP(pte) \
@@ -66,6 +70,9 @@
#define PTE_APTABLE(pte) \
(extract64(pte, 61, 2))
+#define PTE_NSTABLE(pte) \
+ (extract64(pte, 63, 1))
+
#define PTE_AF(pte) \
(extract64(pte, 10, 1))
/*
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index cf17c405de..af2936cf16 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -704,6 +704,8 @@ static inline int oas2bits(int oas_field)
#define CD_R(x) extract32((x)->word[1], 13, 1)
#define CD_A(x) extract32((x)->word[1], 14, 1)
#define CD_AARCH64(x) extract32((x)->word[1], 9 , 1)
+#define CD_NSCFG0(x) extract32((x)->word[2], 0, 1)
+#define CD_NSCFG1(x) extract32((x)->word[4], 0, 1)
/**
* tg2granule - Decodes the CD translation granule size field according
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index eba709ae2b..2f8494c346 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -832,6 +832,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
}
+ tt->nscfg = i ? CD_NSCFG1(cd) : CD_NSCFG0(cd);
tt->had = CD_HAD(cd, i);
trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had);
}
@@ -929,6 +930,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event,
cfg->sec_idx = sec_idx;
cfg->txattrs = smmu_get_txattrs(sec_idx);
cfg->as = smmu_get_address_space(sec_idx);
+ cfg->sel2 = s->bank[SMMU_SEC_IDX_S].idr[1];
if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
SMMUConfigKey *persistent_key = g_new(SMMUConfigKey, 1);
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 80cb4d6b04..f99de78655 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -16,7 +16,7 @@ smmu_ptw_level(int stage, int level, uint64_t iova, size_t subpage_size, uint64_
smmu_ptw_invalid_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" offset=%d pte=0x%"PRIx64
smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d iova=0x%"PRIx64" base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" page address = 0x%"PRIx64
smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
-smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
+smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte, bool ns_walk) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64" ns_walk=%d"
smmu_iotlb_inv_all(void) "IOTLB invalidate all"
smmu_iotlb_inv_asid_vmid(int asid, int vmid) "IOTLB invalidate asid=%d vmid=%d"
smmu_iotlb_inv_vmid(int vmid) "IOTLB invalidate vmid=%d"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index ed21db7728..c27aec8bd4 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -109,6 +109,7 @@ typedef struct SMMUTransTableInfo {
uint8_t tsz; /* input range, ie. 2^(64 -tsz)*/
uint8_t granule_sz; /* granule page shift */
bool had; /* hierarchical attribute disable */
+ bool nscfg; /* Non-secure attribute of Starting-level TT */
} SMMUTransTableInfo;
typedef struct SMMUTLBEntry {
@@ -116,6 +117,7 @@ typedef struct SMMUTLBEntry {
uint8_t level;
uint8_t granule;
IOMMUAccessFlags parent_perm;
+ SMMUSecurityIndex sec_idx;
} SMMUTLBEntry;
/* Stage-2 configuration. */
@@ -156,6 +158,8 @@ typedef struct SMMUTransCfg {
SMMUSecurityIndex sec_idx; /* cached security index */
MemTxAttrs txattrs; /* cached transaction attributes */
AddressSpace *as; /* cached address space */
+ bool current_walk_ns; /* cached if the current walk is non-secure */
+ bool sel2;
} SMMUTransCfg;
typedef struct SMMUDevice {
--
2.34.1Hi Tao,
On 9/25/25 6:26 PM, Tao Tang wrote:
> This patch introduces the necessary logic to handle security states
> during the page table translation process.
>
> Support for the NS (Non-secure) attribute bit is added to the parsing of
> various translation structures, including CD and PTEs. This allows the
> SMMU model to correctly determine the security properties of memory
> during a translation.
>
> With this change, a new translation stage is added:
>
> - Secure Stage 1 translation
>
> Note that this commit does not include support for Secure Stage 2
> translation, which will be addressed in the future.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
> hw/arm/smmu-common.c | 55 ++++++++++++++++++++++++++++++++----
> hw/arm/smmu-internal.h | 7 +++++
> hw/arm/smmuv3-internal.h | 2 ++
> hw/arm/smmuv3.c | 2 ++
> hw/arm/trace-events | 2 +-
> include/hw/arm/smmu-common.h | 4 +++
> 6 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index bc13b00f1d..f563cba023 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -398,20 +398,25 @@ void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
> * @base_addr[@index]
Wile we add some new params it may be relevant to add some new doc
comments above
> */
> static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
> - SMMUPTWEventInfo *info)
> + SMMUPTWEventInfo *info, SMMUTransCfg *cfg, int walk_ns)
I see a cfg param is added while not used.
why walk_ns is an int while it seems to match a SecureIndex type? while
not directly passing the sec_sid?
> {
> int ret;
> dma_addr_t addr = baseaddr + index * sizeof(*pte);
> + /* Only support Secure PA Space as RME isn't implemented yet */
> + MemTxAttrs attrs =
> + smmu_get_txattrs(walk_ns ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S);
> + AddressSpace *as =
> + smmu_get_address_space(walk_ns ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S);
>
> /* 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;
> info->addr = addr;
> return -EINVAL;
> }
> - trace_smmu_get_pte(baseaddr, index, addr, *pte);
> + trace_smmu_get_pte(baseaddr, index, addr, *pte, walk_ns);
> return 0;
> }
>
> @@ -542,6 +547,8 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
>
> baseaddr = extract64(tt->ttb, 0, cfg->oas);
> baseaddr &= ~indexmask;
> + int nscfg = tt->nscfg;
> + bool forced_ns = false; /* Track if NSTable=1 forced NS mode */
>
> while (level < VMSA_LEVELS) {
> uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> @@ -551,7 +558,9 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
> dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> uint8_t ap;
>
> - if (get_pte(baseaddr, offset, &pte, info)) {
> + /* Use NS if forced by previous NSTable=1 or current nscfg */
> + int current_ns = forced_ns || nscfg;
> + if (get_pte(baseaddr, offset, &pte, info, cfg, current_ns)) {
> goto error;
> }
> trace_smmu_ptw_level(stage, level, iova, subpage_size,
> @@ -576,6 +585,26 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
> goto error;
> }
> }
> +
> + /*
> + * Hierarchical control of Secure/Non-secure accesses:
> + * If NSTable=1 from Secure space, force all subsequent lookups to
> + * Non-secure space and ignore future NSTable according to
> + * (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
> + */
> + if (!forced_ns) {
> + int new_nstable = PTE_NSTABLE(pte);
> + if (!current_ns && new_nstable) {
> + /* First transition from Secure to Non-secure */
> + forced_ns = true;
> + nscfg = 1;
> + } else if (!forced_ns) {
> + /* Still in original mode, update nscfg normally */
> + nscfg = new_nstable;
> + }
> + /* If forced_ns is already true, ignore NSTable bit */
> + }
> level++;
> continue;
> } else if (is_page_pte(pte, level)) {
> @@ -618,6 +647,8 @@ static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
> goto error;
> }
>
> + tlbe->sec_idx = PTE_NS(pte) ? SMMU_SEC_IDX_NS : SMMU_SEC_IDX_S;
> + tlbe->entry.target_as = smmu_get_address_space(tlbe->sec_idx);
> tlbe->entry.translated_addr = gpa;
> tlbe->entry.iova = iova & ~mask;
> tlbe->entry.addr_mask = mask;
> @@ -687,7 +718,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> uint8_t s2ap;
>
> - if (get_pte(baseaddr, offset, &pte, info)) {
> + /* Use NS as Secure Stage 2 is not implemented (SMMU_S_IDR1.SEL2 == 0)*/
> + if (get_pte(baseaddr, offset, &pte, info, cfg, 1)) {
> goto error;
> }
> trace_smmu_ptw_level(stage, level, ipa, subpage_size,
> @@ -740,6 +772,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> goto error_ipa;
> }
>
> + tlbe->sec_idx = SMMU_SEC_IDX_NS;
> + tlbe->entry.target_as = &address_space_memory;
> tlbe->entry.translated_addr = gpa;
> tlbe->entry.iova = ipa & ~mask;
> tlbe->entry.addr_mask = mask;
> @@ -824,6 +858,17 @@ int smmu_ptw(SMMUState *bs, SMMUTransCfg *cfg, dma_addr_t iova,
> return ret;
> }
>
> + if (!cfg->sel2 && tlbe->sec_idx > SMMU_SEC_IDX_NS) {
> + /*
> + * Nested translation with Secure IPA output is not supported if
> + * Secure Stage 2 is not implemented.
> + */
> + info->type = SMMU_PTW_ERR_TRANSLATION;
> + info->stage = SMMU_STAGE_1;
> + tlbe->entry.perm = IOMMU_NONE;
> + return -EINVAL;
> + }
> +
> ipa = CACHED_ENTRY_TO_ADDR(tlbe, iova);
> ret = smmu_ptw_64_s2(cfg, ipa, perm, &tlbe_s2, info);
> if (ret) {
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index d143d296f3..cb3a6eb8d1 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -58,6 +58,10 @@
> ((level == 3) && \
> ((pte & ARM_LPAE_PTE_TYPE_MASK) == ARM_LPAE_L3_PTE_TYPE_PAGE))
>
> +/* Non-secure bit */
> +#define PTE_NS(pte) \
> + (extract64(pte, 5, 1))
> +
I have not read that code for a while. Might be worth to create
differentiated sections for the different kinds of descriptors
For instance NS belongs to block & page descriptor while NSTable belongs
to a table descriptor.
> /* access permissions */
>
> #define PTE_AP(pte) \
> @@ -66,6 +70,9 @@
> #define PTE_APTABLE(pte) \
> (extract64(pte, 61, 2))
>
> +#define PTE_NSTABLE(pte) \
> + (extract64(pte, 63, 1))
> +
> #define PTE_AF(pte) \
> (extract64(pte, 10, 1))
> /*
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index cf17c405de..af2936cf16 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -704,6 +704,8 @@ static inline int oas2bits(int oas_field)
> #define CD_R(x) extract32((x)->word[1], 13, 1)
> #define CD_A(x) extract32((x)->word[1], 14, 1)
> #define CD_AARCH64(x) extract32((x)->word[1], 9 , 1)
> +#define CD_NSCFG0(x) extract32((x)->word[2], 0, 1)
> +#define CD_NSCFG1(x) extract32((x)->word[4], 0, 1)
>
> /**
> * tg2granule - Decodes the CD translation granule size field according
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index eba709ae2b..2f8494c346 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -832,6 +832,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
> }
>
> + tt->nscfg = i ? CD_NSCFG1(cd) : CD_NSCFG0(cd);
> tt->had = CD_HAD(cd, i);
> trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had);
> }
> @@ -929,6 +930,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event,
> cfg->sec_idx = sec_idx;
> cfg->txattrs = smmu_get_txattrs(sec_idx);
> cfg->as = smmu_get_address_space(sec_idx);
> + cfg->sel2 = s->bank[SMMU_SEC_IDX_S].idr[1];
S_IDR1 contains other feilds than SEL2 such as S_SIDSIZE?
Can't you split that patch again into 2 patches:
one related to the config data extraction and another one related to
page table walk according to the config settings?
>
> if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
> SMMUConfigKey *persistent_key = g_new(SMMUConfigKey, 1);
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 80cb4d6b04..f99de78655 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -16,7 +16,7 @@ smmu_ptw_level(int stage, int level, uint64_t iova, size_t subpage_size, uint64_
> smmu_ptw_invalid_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" offset=%d pte=0x%"PRIx64
> smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d iova=0x%"PRIx64" base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" page address = 0x%"PRIx64
> smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
> -smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
> +smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte, bool ns_walk) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64" ns_walk=%d"
> smmu_iotlb_inv_all(void) "IOTLB invalidate all"
> smmu_iotlb_inv_asid_vmid(int asid, int vmid) "IOTLB invalidate asid=%d vmid=%d"
> smmu_iotlb_inv_vmid(int vmid) "IOTLB invalidate vmid=%d"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index ed21db7728..c27aec8bd4 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -109,6 +109,7 @@ typedef struct SMMUTransTableInfo {
> uint8_t tsz; /* input range, ie. 2^(64 -tsz)*/
> uint8_t granule_sz; /* granule page shift */
> bool had; /* hierarchical attribute disable */
> + bool nscfg; /* Non-secure attribute of Starting-level TT */
> } SMMUTransTableInfo;
>
> typedef struct SMMUTLBEntry {
> @@ -116,6 +117,7 @@ typedef struct SMMUTLBEntry {
> uint8_t level;
> uint8_t granule;
> IOMMUAccessFlags parent_perm;
> + SMMUSecurityIndex sec_idx;
> } SMMUTLBEntry;
>
> /* Stage-2 configuration. */
> @@ -156,6 +158,8 @@ typedef struct SMMUTransCfg {
> SMMUSecurityIndex sec_idx; /* cached security index */
> MemTxAttrs txattrs; /* cached transaction attributes */
> AddressSpace *as; /* cached address space */
> + bool current_walk_ns; /* cached if the current walk is non-secure */
this does not seem to be used?
> + bool sel2;
would require a comment to remind the reader what sel2 is.
> } SMMUTransCfg;
>
> typedef struct SMMUDevice {
Thanks
Eric
Hi Eric,
On 2025/9/29 22:21, Eric Auger wrote:
> Hi Tao,
>
> On 9/25/25 6:26 PM, Tao Tang wrote:
>> This patch introduces the necessary logic to handle security states
>> during the page table translation process.
>>
>> Support for the NS (Non-secure) attribute bit is added to the parsing of
>> various translation structures, including CD and PTEs. This allows the
>> SMMU model to correctly determine the security properties of memory
>> during a translation.
>>
>> With this change, a new translation stage is added:
>>
>> - Secure Stage 1 translation
>>
>> Note that this commit does not include support for Secure Stage 2
>> translation, which will be addressed in the future.
>>
>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>> ---
>> hw/arm/smmu-common.c | 55 ++++++++++++++++++++++++++++++++----
>> hw/arm/smmu-internal.h | 7 +++++
>> hw/arm/smmuv3-internal.h | 2 ++
>> hw/arm/smmuv3.c | 2 ++
>> hw/arm/trace-events | 2 +-
>> include/hw/arm/smmu-common.h | 4 +++
>> 6 files changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index bc13b00f1d..f563cba023 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -398,20 +398,25 @@ void smmu_iotlb_inv_vmid_s1(SMMUState *s, int vmid)
>> * @base_addr[@index]
> Wile we add some new params it may be relevant to add some new doc
> comments above
Thank you for another incredibly thorough and insightful review. I
sincerely appreciate you taking the time to go through the code in such
detail.
You're right. I missed some necessary comments when adding new
parameters. I will add them in next series.
>> */
>> static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
>> - SMMUPTWEventInfo *info)
>> + SMMUPTWEventInfo *info, SMMUTransCfg *cfg, int walk_ns)
> I see a cfg param is added while not used.
My original plan was to cache the NS attr in a cfg->walk_ns field, which
is why I passed cfg into this function. I later realized this caching
wasn't necessary and removed the walk_ns member from the struct, but I
clearly missed removing the now-redundant cfg parameter from the
function signature. Thanks for spotting this oversight; I will remove it
in v3.
> why walk_ns is an int while it seems to match a SecureIndex type? while
> not directly passing the sec_sid?
You're right. I will replace 'int walk_ns ' with sec_sid in v3.
>>
>> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
>> index d143d296f3..cb3a6eb8d1 100644
>> --- a/hw/arm/smmu-internal.h
>> +++ b/hw/arm/smmu-internal.h
>> @@ -58,6 +58,10 @@
>> ((level == 3) && \
>> ((pte & ARM_LPAE_PTE_TYPE_MASK) == ARM_LPAE_L3_PTE_TYPE_PAGE))
>>
>> +/* Non-secure bit */
>> +#define PTE_NS(pte) \
>> + (extract64(pte, 5, 1))
>> +
> I have not read that code for a while. Might be worth to create
> differentiated sections for the different kinds of descriptors
> For instance NS belongs to block & page descriptor while NSTable belongs
> to a table descriptor.
The original code didn't have clear comments to differentiate between
the descriptor types. Now that I've introduced the new NS and NSTable
attribute bits, which can be easily confused, it has become necessary to
add these clarifying sections. I will add comments to group the macros
by descriptor type to improve readability in the next version. Thanks
for the suggestion.
>>
>>
>> /**
>> * tg2granule - Decodes the CD translation granule size field according
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index eba709ae2b..2f8494c346 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -832,6 +832,7 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
>> tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb);
>> }
>>
>> + tt->nscfg = i ? CD_NSCFG1(cd) : CD_NSCFG0(cd);
>> tt->had = CD_HAD(cd, i);
>> trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had);
>> }
>> @@ -929,6 +930,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event,
>> cfg->sec_idx = sec_idx;
>> cfg->txattrs = smmu_get_txattrs(sec_idx);
>> cfg->as = smmu_get_address_space(sec_idx);
>> + cfg->sel2 = s->bank[SMMU_SEC_IDX_S].idr[1];
> S_IDR1 contains other feilds than SEL2 such as S_SIDSIZE?
>
> Can't you split that patch again into 2 patches:
> one related to the config data extraction and another one related to
> page table walk according to the config settings?
Sure. I'll split it into 2 patchs in v3 and cfg->sel2 will be corrected.
>>
>>
>>
>> typedef struct SMMUTLBEntry {
>> @@ -116,6 +117,7 @@ typedef struct SMMUTLBEntry {
>> uint8_t level;
>> uint8_t granule;
>> IOMMUAccessFlags parent_perm;
>> + SMMUSecurityIndex sec_idx;
>> } SMMUTLBEntry;
>>
>> /* Stage-2 configuration. */
>> @@ -156,6 +158,8 @@ typedef struct SMMUTransCfg {
>> SMMUSecurityIndex sec_idx; /* cached security index */
>> MemTxAttrs txattrs; /* cached transaction attributes */
>> AddressSpace *as; /* cached address space */
>> + bool current_walk_ns; /* cached if the current walk is non-secure */
> this does not seem to be used?
>> + bool sel2;
> would require a comment to remind the reader what sel2 is.
>> } SMMUTransCfg;
>>
>> typedef struct SMMUDevice {
> Thanks
>
> Eric
Yes. current_walk_ns will be removed and comment will be add in v3.
Thanks,
Tao
© 2016 - 2026 Red Hat, Inc.