Re: [RFC PATCH 01/12] hw/arm/smmu: Use enum for SMMU stage

[Eric Auger]

Hi Mostafa,

On 3/25/24 11:13, Mostafa Saleh wrote:
> Currently, translation stage is represented as an int, where 1 is stage-1 and
> 2 is stage-2, when nested is added, 3 would be confusing to represent nesting,
> so we use an enum instead.
>
> While keeping the same values, this is useful for:
>  - Doing tricks with bit masks, where BIT(0) is stage-1 and BIT(1) is
>    stage-2 and both is nested.
>  - Tracing, as stage is printed as int.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/smmu-common.c         | 14 +++++++-------
>  hw/arm/smmuv3.c              | 15 ++++++++-------
>  include/hw/arm/smmu-common.h | 11 +++++++++--
>  3 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 4caedb4998..3a7c350aca 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -304,7 +304,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>                            SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
>      dma_addr_t baseaddr, indexmask;
> -    int stage = cfg->stage;
> +    SMMUStage stage = cfg->stage;
>      SMMUTransTableInfo *tt = select_tt(cfg, iova);
>      uint8_t level, granule_sz, inputsize, stride;
>  
> @@ -392,7 +392,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>      info->type = SMMU_PTW_ERR_TRANSLATION;
>  
>  error:
> -    info->stage = 1;
> +    info->stage = SMMU_STAGE_1;
>      tlbe->entry.perm = IOMMU_NONE;
>      return -EINVAL;
>  }
> @@ -415,7 +415,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>                            dma_addr_t ipa, IOMMUAccessFlags perm,
>                            SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
> -    const int stage = 2;
> +    const SMMUStage stage = SMMU_STAGE_2;
>      int granule_sz = cfg->s2cfg.granule_sz;
>      /* ARM DDI0487I.a: Table D8-7. */
>      int inputsize = 64 - cfg->s2cfg.tsz;
> @@ -513,7 +513,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>      info->type = SMMU_PTW_ERR_TRANSLATION;
>  
>  error:
> -    info->stage = 2;
> +    info->stage = SMMU_STAGE_2;
>      tlbe->entry.perm = IOMMU_NONE;
>      return -EINVAL;
>  }
> @@ -532,9 +532,9 @@ error:
>  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
> -    if (cfg->stage == 1) {
> +    if (cfg->stage == SMMU_STAGE_1) {
>          return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> -    } else if (cfg->stage == 2) {
> +    } else if (cfg->stage == SMMU_STAGE_2) {
>          /*
>           * If bypassing stage 1(or unimplemented), the input address is 
> passed
>           * directly to stage 2 as IPA. If the input address of a transaction
> @@ -543,7 +543,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, 
> IOMMUAccessFlags perm,
>           */
>          if (iova >= (1ULL << cfg->oas)) {
>              info->type = SMMU_PTW_ERR_ADDR_SIZE;
> -            info->stage = 1;
> +            info->stage = SMMU_STAGE_1;
>              tlbe->entry.perm = IOMMU_NONE;
>              return -EINVAL;
>          }
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 9eb56a70f3..50e5a72d54 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -34,7 +34,8 @@
>  #include "smmuv3-internal.h"
>  #include "smmu-internal.h"
>  
> -#define PTW_RECORD_FAULT(cfg)   (((cfg)->stage == 1) ? (cfg)->record_faults 
> : \
> +#define PTW_RECORD_FAULT(cfg)   (((cfg)->stage == SMMU_STAGE_1) ? \
> +                                 (cfg)->record_faults : \
>                                   (cfg)->s2cfg.record_faults)
>  
>  /**
> @@ -402,7 +403,7 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t 
> t0sz, uint8_t gran)
>  
>  static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
>  {
> -    cfg->stage = 2;
> +    cfg->stage = SMMU_STAGE_2;
>  
>      if (STE_S2AA64(ste) == 0x0) {
>          qemu_log_mask(LOG_UNIMP,
> @@ -678,7 +679,7 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, 
> SMMUEventInfo *event)
>  
>      /* we support only those at the moment */
>      cfg->aa64 = true;
> -    cfg->stage = 1;
> +    cfg->stage = SMMU_STAGE_1;
>  
>      cfg->oas = oas2bits(CD_IPS(cd));
>      cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
> @@ -762,7 +763,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, 
> SMMUTransCfg *cfg,
>          return ret;
>      }
>  
> -    if (cfg->aborted || cfg->bypassed || (cfg->stage == 2)) {
> +    if (cfg->aborted || cfg->bypassed || (cfg->stage == SMMU_STAGE_2)) {
>          return 0;
>      }
>  
> @@ -882,7 +883,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>          goto epilogue;
>      }
>  
> -    if (cfg->stage == 1) {
> +    if (cfg->stage == SMMU_STAGE_1) {
>          /* Select stage1 translation table. */
>          tt = select_tt(cfg, addr);
>          if (!tt) {
> @@ -919,7 +920,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>               * nesting is not supported. So it is sufficient to check the
>               * translation stage to know the TLB stage for now.
>               */
> -            event.u.f_walk_eabt.s2 = (cfg->stage == 2);
> +            event.u.f_walk_eabt.s2 = (cfg->stage == SMMU_STAGE_2);
>              if (PTW_RECORD_FAULT(cfg)) {
>                  event.type = SMMU_EVT_F_PERMISSION;
>                  event.u.f_permission.addr = addr;
> @@ -935,7 +936,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>  
>      if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
>          /* All faults from PTW has S2 field. */
> -        event.u.f_walk_eabt.s2 = (ptw_info.stage == 2);
> +        event.u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2);
>          g_free(cached_entry);
>          switch (ptw_info.type) {
>          case SMMU_PTW_ERR_WALK_EABT:
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 5ec2e6c1a4..b3c881f0ee 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -49,8 +49,15 @@ typedef enum {
>      SMMU_PTW_ERR_PERMISSION,  /* Permission fault */
>  } SMMUPTWEventType;
>  
> +/* SMMU Stage */
> +typedef enum {
> +    SMMU_STAGE_1 = 1,
> +    SMMU_STAGE_2,
> +    SMMU_NESTED,
> +} SMMUStage;
> +
>  typedef struct SMMUPTWEventInfo {
> -    int stage;
> +    SMMUStage stage;
>      SMMUPTWEventType type;
>      dma_addr_t addr; /* fetched address that induced an abort, if any */
>  } SMMUPTWEventInfo;
> @@ -88,7 +95,7 @@ typedef struct SMMUS2Cfg {
>   */
>  typedef struct SMMUTransCfg {
>      /* Shared fields between stage-1 and stage-2. */
> -    int stage;                 /* translation stage */
> +    SMMUStage stage;           /* translation stage */
>      bool disabled;             /* smmu is disabled */
>      bool bypassed;             /* translation is bypassed */
>      bool aborted;              /* translation is aborted */