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 <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
