Re: [PATCH v2] iommu/arm-smmu-v3: Match Stall behaviour for S2

[Mostafa Saleh <smostafa@google.com>]

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