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 <smost...@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.au...@redhat.com>
>
> Eric
>
