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 <[email protected]>
---
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
