| Message ID | 20240429032403.74910-3-smostafa@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
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
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 >
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;
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. However, this was not implemented correctly, as for stage 2, we Qemu only sets the S2 bit but not the IPA. 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 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(-)