Re: [RFC PATCH v3 02/18] hw/arm/smmu: Fix IPA for stage-2 events

[Eric Auger]

Hi Mostafa,

On 4/29/24 05:23, Mostafa Saleh wrote:
> For the following events (ARM IHI 0070 F.b - 7.3 Event records):
> - F_TRANSLATION
> - F_ACCESS
> - F_PERMISSION
> - F_ADDR_SIZE
>
> If fault occurs at stage 2, S2 == 1 and:
>   - If translating an IPA for a transaction (whether by input to
>     stage 2-only configuration, or after successful stage 1 translation),
>     CLASS == IN, and IPA is provided.
CLASS == IN sounds a bit confusing here since the class value depends on
what is being translated and class is not handled in that patch.
>
> However, this was not implemented correctly, as for stage 2, we Qemu
s/we QEMU/ the code
> only sets the  S2 bit but not the IPA.
If this is a fix, please add the "Fixes:" tag and fixed commit sha1.
>
> This field has the same bits as FetchAddr in F_WALK_EABT which is
> populated correctly, so we don’t change that.
> The population of this field should be done from the walker as the IPA address
s/population/setting? I am not a native english speaker though
> wouldn't be known in case of nesting.
>
> For stage 1, the spec says:
>   If fault occurs at stage 1, S2 == 0 and:
>   CLASS == IN, IPA is UNKNOWN.
>
> So, no need to set it to for stage 1, as ptw_info is initialised by zero in
> smmuv3_translate().
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c | 10 ++++++----
>  hw/arm/smmuv3.c      |  4 ++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index eb2356bc35..8a8c718e6b 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -448,7 +448,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>       */
>      if (ipa >= (1ULL << inputsize)) {
>          info->type = SMMU_PTW_ERR_TRANSLATION;
> -        goto error;
> +        goto error_ipa;
>      }
>  
>      while (level < VMSA_LEVELS) {
> @@ -494,13 +494,13 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>           */
>          if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
>              info->type = SMMU_PTW_ERR_ACCESS;
> -            goto error;
> +            goto error_ipa;
>          }
>  
>          s2ap = PTE_AP(pte);
>          if (is_permission_fault_s2(s2ap, perm)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
> -            goto error;
> +            goto error_ipa;
>          }
>  
>          /*
> @@ -509,7 +509,7 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>           */
>          if (gpa >= (1ULL << cfg->s2cfg.eff_ps)) {
>              info->type = SMMU_PTW_ERR_ADDR_SIZE;
> -            goto error;
> +            goto error_ipa;
>          }
>  
>          tlbe->entry.translated_addr = gpa;
> @@ -522,6 +522,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>      }
>      info->type = SMMU_PTW_ERR_TRANSLATION;
>  
> +error_ipa:
> +    info->addr = ipa;
>  error:
>      info->stage = 2;
>      tlbe->entry.perm = IOMMU_NONE;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 2d1e0d55ec..9dd3ea48e4 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -949,6 +949,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>              if (PTW_RECORD_FAULT(cfg)) {
>                  event.type = SMMU_EVT_F_TRANSLATION;
>                  event.u.f_translation.addr = addr;
> +                event.u.f_translation.addr2 = ptw_info.addr;
>                  event.u.f_translation.rnw = flag & 0x1;
>              }
>              break;
> @@ -956,6 +957,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>              if (PTW_RECORD_FAULT(cfg)) {
>                  event.type = SMMU_EVT_F_ADDR_SIZE;
>                  event.u.f_addr_size.addr = addr;
> +                event.u.f_addr_size.addr2 = ptw_info.addr;
>                  event.u.f_addr_size.rnw = flag & 0x1;
>              }
>              break;
> @@ -963,6 +965,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>              if (PTW_RECORD_FAULT(cfg)) {
>                  event.type = SMMU_EVT_F_ACCESS;
>                  event.u.f_access.addr = addr;
> +                event.u.f_access.addr2 = ptw_info.addr;
>                  event.u.f_access.rnw = flag & 0x1;
>              }
>              break;
> @@ -970,6 +973,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>              if (PTW_RECORD_FAULT(cfg)) {
>                  event.type = SMMU_EVT_F_PERMISSION;
>                  event.u.f_permission.addr = addr;
> +                event.u.f_permission.addr2 = ptw_info.addr;
>                  event.u.f_permission.rnw = flag & 0x1;
>              }
>              break;
>
After taking into account above comments,
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric