[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zr3m4YCY7Ape3R6y@google.com>
Date: Thu, 15 Aug 2024 11:30:41 +0000
From: Mostafa Saleh <smostafa@...gle.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, will@...nel.org,
robin.murphy@....com, joro@...tes.org, jean-philippe@...aro.org,
nicolinc@...dia.com, mshavit@...gle.com
Subject: Re: [PATCH v2] iommu/arm-smmu-v3: Match Stall behaviour for S2
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
Powered by blists - more mailing lists