[PATCH v2] iommu/arm-smmu-v3: Match Stall behaviour for S2
Mostafa Saleh
smostafa at google.com
Thu Aug 15 04:30:41 PDT 2024
Hi Jason,
On Wed, Aug 14, 2024 at 12:51:51PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 14, 2024 at 02:56:33PM +0000, Mostafa Saleh wrote:
>
> > Also described in the pseudocode “SteIllegal()”
> > if eff_idr0_stall_model == '10' && STE.S2S == '0' then
> > // stall_model forcing stall, but S2S == 0
> > return TRUE;
>
> This clips out an important bit:
>
> if STE.Config == '11x' then
> [..]
> if eff_idr0_stall_model == '10' && STE.S2S == '0' then
> // stall_model forcing stall, but S2S == 0
> return TRUE;
>
> And here we are using STRTAB_STE_0_CFG_S1_TRANS which is 101 and won't
> match the STE.Config qualification.
>
> The plain text language said the S2S is only required if the S2 is
> translating, STRTAB_STE_0_CFG_S1_TRANS puts it in bypass.
Yes, my bad, this should be for stage-2 only which is populated in
arm_smmu_make_s2_domain_ste()
>
> > + /*
> > + * S2S is ignored if stage-2 exists but not enabled.
> > + * S2S is not compatible with ATS.
> > + */
> > + if (master->stall_enabled && !ats_enabled &&
> > + smmu->features & ARM_SMMU_FEAT_TRANS_S2)
> > + target->data[2] |= STRTAB_STE_2_S2S;
>
> We can't ignore ATS if it was requested here.
>
> I think that does point to an issue, ATS should be fixed up here:
>
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2492,6 +2492,9 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
> if (!(fwspec->flags & IOMMU_FWSPEC_PCI_RC_ATS))
> return false;
>
> + if (master->stall_enabled)
> + return false;
> +
> return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
> }
Makes sense, I will add that to the patch instead of checking at STE
creation time.
>
> And your hunk above should be placed in arm_smmu_make_s2_domain_ste()
> not arm_smmu_make_cdtable_ste()
>
> Not ignoring the event still makes sense to me, but I didn't check it
> carefully. We can decode the S2 event right?
>
Yes, s2 translation fault events are the same but with an extra bit
set (S2), and with a new field for IPA which is not relevant here as
we only report the IOVA.
When full nesting is supported, as iopf_fault doesn’t understand
nesting and only have the IOVA in addr, we would need to filter the
event by stage.
Thanks,
Mostafa
> Jason
More information about the linux-arm-kernel
mailing list