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

[Mostafa Saleh <smostafa@google.com>]

Hi Eric,

On Mon, May 13, 2024 at 01:47:44PM +0200, Eric Auger wrote:
> 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.
At this point only CLASS IN is used as nesting is not supported,
I will clarify that in the commit message.

> >
> > However, this was not implemented correctly, as for stage 2, we Qemu
> s/we QEMU/ the code
Will do.

> > only sets the  S2 bit but not the IPA.
> If this is a fix, please add the "Fixes:" tag and fixed commit sha1.
Will do.

> >
> > 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
Me neither :), I will change it.

Thanks,
Mostafa
> > 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
>