Re: [RFC v3 11/21] hw/arm/smmuv3: Decode security attributes from descriptors

[Eric Auger <eric.auger@redhat.com>]

Hi Tao,

On 10/12/25 5:06 PM, Tao Tang wrote:
> As the first step in implementing secure page table walks, this patch
> introduces the logic to decode security-related attributes from various
> SMMU structures.
>
> The NSCFG bits from the Context Descriptor are now decoded and stored.
> These bits control the security attribute of the starting-level
> translation table, which is crucial for managing secure and non-secure
> memory accesses.
>
> The SMMU_S_IDR1.SEL2 bit is read to determine if Secure stage 2
> translations are supported. This capability is cached in the
> SMMUTransCfg structure for the page table walker's use.
>
> Finally, new macros (PTE_NS, PTE_NSTABLE) are added to prepare for
> extracting attributes from page and table descriptors. To improve
> clarity, these different attribute bits are organized into distinct
> subsections in the header file.
>
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> ---
>  hw/arm/smmu-internal.h       | 16 ++++++++++++++--
>  hw/arm/smmuv3-internal.h     |  2 ++
>  hw/arm/smmuv3.c              |  2 ++
>  include/hw/arm/smmu-common.h |  3 +++
>  4 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index d143d296f3..a0454f720d 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -58,16 +58,28 @@
>      ((level == 3) &&                                                    \
>       ((pte & ARM_LPAE_PTE_TYPE_MASK) == ARM_LPAE_L3_PTE_TYPE_PAGE))
>  
> +/* Block & page descriptor attributes */
> +/* Non-secure bit */
> +#define PTE_NS(pte) \
> +    (extract64(pte, 5, 1))
> +
>  /* access permissions */
>  
>  #define PTE_AP(pte) \
>      (extract64(pte, 6, 2))
>  
> +/* access flag */
> +#define PTE_AF(pte) \
> +    (extract64(pte, 10, 1))
> +
> +
> +/* Table descriptor attributes */
>  #define PTE_APTABLE(pte) \
>      (extract64(pte, 61, 2))
>  
> -#define PTE_AF(pte) \
> -    (extract64(pte, 10, 1))
> +#define PTE_NSTABLE(pte) \
> +    (extract64(pte, 63, 1))
> +
>  /*
>   * TODO: At the moment all transactions are considered as privileged (EL1)
>   * as IOMMU translation callback does not pass user/priv attributes.
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 99fdbcf3f5..1e757af459 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -703,6 +703,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 55f4ad1757..3686056d8e 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -812,6 +812,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);
>      }
> @@ -915,6 +916,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event,
>              cfg = NULL;
>              return cfg;
>          }
> +        cfg->sel2 = FIELD_EX32(s->bank[SMMU_SEC_SID_S].idr[1], S_IDR1, SEL2);
I don't get why we store sel2 in the cfg as it does not vary.

Thanks

Eric
>  
>          if (!smmuv3_decode_config(&sdev->iommu, cfg, event)) {
>              SMMUConfigKey *persistent_key = g_new(SMMUConfigKey, 1);
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index bccbbe0115..90a37fe32d 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 */
> +    int 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;
> +    SMMUSecSID sec_sid;
>  } SMMUTLBEntry;
>  
>  /* Stage-2 configuration. */
> @@ -156,6 +158,7 @@ typedef struct SMMUTransCfg {
>      struct SMMUS2Cfg s2cfg;
>      MemTxAttrs txattrs;        /* cached transaction attributes */
>      AddressSpace *as;          /* cached address space */
> +    int sel2;                 /* Secure EL2 and Secure stage 2 support */
>  } SMMUTransCfg;
>  
>  typedef struct SMMUDevice {