| Message ID | 20240814145633.2565126-1-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iommu/arm-smmu-v3: Match Stall behaviour for S2 | expand |
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. > + /* > + * 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)); } 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? Jason
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
On Thu, Aug 15, 2024 at 11:30:41AM +0000, Mostafa Saleh wrote: > 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. Okay, as long as the IOVA is decoded and passed into report_fault it should be fine We don't yet have support for specifying the IPA on nesting.. Jason
On 15/08/2024 12:30 pm, Mostafa Saleh wrote: > 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 don't see much value in adding effectively-dead checks for something which is already forbidden by the architecture. The definition of STALL_MODEL explicitly states: "An SMMU associated with a PCI system must not have STALL_MODEL == 0b10". Thanks, Robin.
Hi Robin, On Thu, Aug 15, 2024 at 01:16:19PM +0100, Robin Murphy wrote: > On 15/08/2024 12:30 pm, Mostafa Saleh wrote: > > 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 don't see much value in adding effectively-dead checks for something which > is already forbidden by the architecture. The definition of STALL_MODEL > explicitly states: > > "An SMMU associated with a PCI system must not have STALL_MODEL == 0b10". > Ah, I was expecting that as otherwise it's contradiction, but couldn't find it while searching. Thanks for pointing it out, I will drop all references to ATS then. Thanks, Mostafa > Thanks, > Robin.
On Thu, Aug 15, 2024 at 12:26:46PM +0000, Mostafa Saleh wrote: > Hi Robin, > > On Thu, Aug 15, 2024 at 01:16:19PM +0100, Robin Murphy wrote: > > On 15/08/2024 12:30 pm, Mostafa Saleh wrote: > > > 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 don't see much value in adding effectively-dead checks for something which > > is already forbidden by the architecture. The definition of STALL_MODEL > > explicitly states: > > > > "An SMMU associated with a PCI system must not have STALL_MODEL == 0b10". > > > > Ah, I was expecting that as otherwise it's contradiction, but couldn't > find it while searching. Thanks for pointing it out, I will drop all > references to ATS then. I was thinking this was also protecting against buggy FW since stall_enable can be set by: device_property_read_bool(dev, "dma-can-stall")) Alternatively we could directly prevent the clash even earlier: @@ -3292,8 +3292,13 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) if ((smmu->features & ARM_SMMU_FEAT_STALLS && device_property_read_bool(dev, "dma-can-stall")) || - smmu->features & ARM_SMMU_FEAT_STALL_FORCE) - master->stall_enabled = true; + smmu->features & ARM_SMMU_FEAT_STALL_FORCE) { + if (!dev_is_pci(dev)) + master->stall_enabled = true; + else + dev_err(dev, FW_BUG + "A SMMUv3 is required to run in stall mode for a PCI device\n"); + } if (dev_is_pci(dev)) { Though I have no idea how the GPU driver that wants to use this works - it doesn't seem to be intree :\ Jason
On 15/08/2024 1:59 pm, Jason Gunthorpe wrote: > On Thu, Aug 15, 2024 at 12:26:46PM +0000, Mostafa Saleh wrote: >> Hi Robin, >> >> On Thu, Aug 15, 2024 at 01:16:19PM +0100, Robin Murphy wrote: >>> On 15/08/2024 12:30 pm, Mostafa Saleh wrote: >>>> 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 don't see much value in adding effectively-dead checks for something which >>> is already forbidden by the architecture. The definition of STALL_MODEL >>> explicitly states: >>> >>> "An SMMU associated with a PCI system must not have STALL_MODEL == 0b10". >>> >> >> Ah, I was expecting that as otherwise it's contradiction, but couldn't >> find it while searching. Thanks for pointing it out, I will drop all >> references to ATS then. > > I was thinking this was also protecting against buggy FW since > stall_enable can be set by: > device_property_read_bool(dev, "dma-can-stall")) If firmware goes out of its way to describe the system incorrectly then I'd consider that all bets are off, and there's little point in us trying to guess how to cope with a contradiction. For all we know, it could be that the stall property is in fact genuine and it's the ATS capability that was advertised erroneously. > Alternatively we could directly prevent the clash even earlier: > > @@ -3292,8 +3292,13 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) > > if ((smmu->features & ARM_SMMU_FEAT_STALLS && > device_property_read_bool(dev, "dma-can-stall")) || > - smmu->features & ARM_SMMU_FEAT_STALL_FORCE) > - master->stall_enabled = true; > + smmu->features & ARM_SMMU_FEAT_STALL_FORCE) { > + if (!dev_is_pci(dev)) > + master->stall_enabled = true; > + else > + dev_err(dev, FW_BUG > + "A SMMUv3 is required to run in stall mode for a PCI device\n"); Unfortunately we can't do that, because there *are* RCiEP devices whose data interfaces are native AMBA, and thus for whom stalling is not actually a protocol violation as it would be on a real PCIe transport layer; correspondingly, it's *because* they are not true PCIe devices that they can't support ATS, and thus need stall support in order to do SVA, so things should still work out OK in practice. > + } > > if (dev_is_pci(dev)) { > > Though I have no idea how the GPU driver that wants to use this > works - it doesn't seem to be intree :\ It's not a GPU: drivers/crypto/hisilicon/zip/ Thanks, Robin.
On Thu, Aug 15, 2024 at 02:41:52PM +0100, Robin Murphy wrote: > Unfortunately we can't do that, because there *are* RCiEP devices whose data > interfaces are native AMBA, and thus for whom stalling is not actually a > protocol violation as it would be on a real PCIe transport layer; > correspondingly, it's *because* they are not true PCIe devices that they > can't support ATS, and thus need stall support in order to do SVA, so things > should still work out OK in practice. I wondered if that would be the case. Looks like if we want to do anything then arm_smmu_ats_supported() would be the right place. > > if (dev_is_pci(dev)) { > > > > Though I have no idea how the GPU driver that wants to use this > > works - it doesn't seem to be intree :\ > > It's not a GPU: drivers/crypto/hisilicon/zip/ Ohhhh.. so it goes through uacce, I see. Jason
On Thu, Aug 15, 2024 at 10:59:51AM -0300, Jason Gunthorpe wrote: > On Thu, Aug 15, 2024 at 02:41:52PM +0100, Robin Murphy wrote: > > > Unfortunately we can't do that, because there *are* RCiEP devices whose data > > interfaces are native AMBA, and thus for whom stalling is not actually a > > protocol violation as it would be on a real PCIe transport layer; > > correspondingly, it's *because* they are not true PCIe devices that they > > can't support ATS, and thus need stall support in order to do SVA, so things > > should still work out OK in practice. > > I wondered if that would be the case. > > Looks like if we want to do anything then arm_smmu_ats_supported() > would be the right place. Yes, I guess that might be the place, although the spec seems more relaxed regarding stage-1 stall. Anyway I will drop it from this patch, as Robin mentioned this should only happen with buggy firmware and we can have another patch to harden that if required. Thanks, Mostafa > > > > if (dev_is_pci(dev)) { > > > > > > Though I have no idea how the GPU driver that wants to use this > > > works - it doesn't seem to be intree :\ > > > > It's not a GPU: drivers/crypto/hisilicon/zip/ > > Ohhhh.. so it goes through uacce, I see. > > Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index a31460f9f3d4..3b70816a2b81 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1601,6 +1601,14 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target, target->data[2] = cpu_to_le64(FIELD_PREP(STRTAB_STE_2_S2VMID, 0)); } + + /* + * 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; } EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_cdtable_ste); @@ -1739,10 +1747,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt) return -EOPNOTSUPP; } - /* Stage-2 is always pinned at the moment */ - if (evt[1] & EVTQ_1_S2) - return -EFAULT; - if (!(evt[1] & EVTQ_1_STALL)) return -EOPNOTSUPP; diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 14bca41a981b..0dc7ad43c64c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -267,6 +267,7 @@ struct arm_smmu_ste { #define STRTAB_STE_2_S2AA64 (1UL << 51) #define STRTAB_STE_2_S2ENDI (1UL << 52) #define STRTAB_STE_2_S2PTW (1UL << 54) +#define STRTAB_STE_2_S2S (1UL << 57) #define STRTAB_STE_2_S2R (1UL << 58) #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4)
According to the spec (ARM IHI 0070 F.b), in "5.5 Fault configuration (A, R, S bits)": A STE with stage 2 translation enabled and STE.S2S == 0 is considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10. 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; Which means, S2S must be set when stall model is "ARM_SMMU_FEAT_STALL_FORCE", but at the moment the driver ignores that. Although, the driver can do the minimum and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1 behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the master has requested stalls. One caveat, is that S2S is not compatible with ATS, this mentioned in “5.2 Stream Table Entry”: In implementations of SMMUv3.1 and later, this configuration is ILLEGAL if EATS is not IGNORED and Config[1] == 1 and S2S == 1. In SMMUv3.0 implementations, this configuration is ILLEGAL if EATS is not IGNORED and S2S == 1 This is also described in the pseudocode “SteIllegal()” Also, since S2 stalls are enabled now, report them to the IOMMU layer and for VFIO devices it will fail anyway as VFIO doesn’t register an iopf handler. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- v2: - Fix index of the STE - Fix conflict with ATS - Squash the 2 patches and drop enable_nesting --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++++---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)