| Message ID | 20221208151514.3840720-1-vladimir.oltean@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] arm64: dts: ls1028a: mark ARM SMMU as DMA coherent | expand |
Hi Robin, On Wed, Dec 14, 2022 at 08:33:10PM +0000, Robin Murphy wrote: > > Does looking at the CTTW bit make any sense for MMU-500? > > In general, yes. The result above does imply that NXP have inadvertently set > cfg_cttw wrong. For the avoidance of doubt, here's another MMU-500 showing > SMMU_IDR0.CTTW set: > > [ 3.014972] arm-smmu arm-smmu.0.auto: probing hardware configuration... > [ 3.014974] arm-smmu arm-smmu.0.auto: SMMUv2 with: > [ 3.014976] arm-smmu arm-smmu.0.auto: stage 2 translation > [ 3.014977] arm-smmu arm-smmu.0.auto: coherent table walk > [ 3.014979] arm-smmu arm-smmu.0.auto: stream matching with 128 register groups > [ 3.014981] arm-smmu arm-smmu.0.auto: 128 context banks (128 stage-2 only) > [ 3.014984] arm-smmu arm-smmu.0.auto: Supported page sizes: 0x60211000 > [ 3.014986] arm-smmu arm-smmu.0.auto: Stage-2: 48-bit IPA -> 48-bit PA Thanks for the explanations and the patch you've sent separately. I have a side question, why is the dev_name() of your SMMU set to "arm-smmu.0.auto" (determined by PLATFORM_DEVID_AUTO if I'm not mistaken)? I'm asking because I would like to study the mechanism through which your SMMU platform device get probed, to make sure that it's not possible, during shutdown, for both platform_driver :: shutdown() and platform_driver :: remove() methods to get called by the driver core. This is generally not disallowed, and even possible if the entity who registers these platform devices has its ->shutdown() method pointing at ->remove().
On 19/12/2022 12:16 pm, Vladimir Oltean wrote: > Hi Robin, > > On Wed, Dec 14, 2022 at 08:33:10PM +0000, Robin Murphy wrote: >>> Does looking at the CTTW bit make any sense for MMU-500? >> >> In general, yes. The result above does imply that NXP have inadvertently set >> cfg_cttw wrong. For the avoidance of doubt, here's another MMU-500 showing >> SMMU_IDR0.CTTW set: >> >> [ 3.014972] arm-smmu arm-smmu.0.auto: probing hardware configuration... >> [ 3.014974] arm-smmu arm-smmu.0.auto: SMMUv2 with: >> [ 3.014976] arm-smmu arm-smmu.0.auto: stage 2 translation >> [ 3.014977] arm-smmu arm-smmu.0.auto: coherent table walk >> [ 3.014979] arm-smmu arm-smmu.0.auto: stream matching with 128 register groups >> [ 3.014981] arm-smmu arm-smmu.0.auto: 128 context banks (128 stage-2 only) >> [ 3.014984] arm-smmu arm-smmu.0.auto: Supported page sizes: 0x60211000 >> [ 3.014986] arm-smmu arm-smmu.0.auto: Stage-2: 48-bit IPA -> 48-bit PA > > Thanks for the explanations and the patch you've sent separately. > > I have a side question, why is the dev_name() of your SMMU set to > "arm-smmu.0.auto" (determined by PLATFORM_DEVID_AUTO if I'm not mistaken)? This is an ACPI-based machine, where platform device discovery and creation is... different :) SMMUs are among those managed by drivers/acpi/arm64/iort.c > I'm asking because I would like to study the mechanism through which > your SMMU platform device get probed, to make sure that it's not > possible, during shutdown, for both platform_driver :: shutdown() > and platform_driver :: remove() methods to get called by the driver core. > This is generally not disallowed, and even possible if the entity who > registers these platform devices has its ->shutdown() method pointing > at ->remove(). Yikes, I'd very much hope that that's not a thing! Cheers, Robin.
On Tue, Jan 03, 2023 at 06:12:29PM +0000, Robin Murphy wrote: > > I have a side question, why is the dev_name() of your SMMU set to > > "arm-smmu.0.auto" (determined by PLATFORM_DEVID_AUTO if I'm not mistaken)? > > This is an ACPI-based machine, where platform device discovery and creation > is... different :) > > SMMUs are among those managed by drivers/acpi/arm64/iort.c > > > I'm asking because I would like to study the mechanism through which > > your SMMU platform device get probed, to make sure that it's not > > possible, during shutdown, for both platform_driver :: shutdown() > > and platform_driver :: remove() methods to get called by the driver core. > > This is generally not disallowed, and even possible if the entity who > > registers these platform devices has its ->shutdown() method pointing > > at ->remove(). > > Yikes, I'd very much hope that that's not a thing! Ah, ok. Appears to be fine. Looking at drivers/acpi/arm64/iort.c, it seems that no one is removing those platform devices.
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi index 383829ec7be7..bcce189c7a0a 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi @@ -715,6 +715,7 @@ smmu: iommu@5000000 { #global-interrupts = <8>; #iommu-cells = <1>; stream-match-mask = <0x7c00>; + dma-coherent; /* global secure fault */ interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>, /* combined secure interrupt */
Since commit df198b37e72c ("iommu/arm-smmu: Report IOMMU_CAP_CACHE_COHERENCY better"), the SMMU driver will say that a device has the IOMMU_CAP_CACHE_COHERENCY capability if the ARM_SMMU_FEAT_COHERENT_WALK bit was set in smmu->features. This breaks vfio-pci, as can be seen below: $ echo 0000:00:00.0 > /sys/bus/pci/drivers/fsl_enetc/unbind $ echo vfio-pci > /sys/bus/pci/devices/0000\:00\:00.0/driver_override $ echo 0000:00:00.0 > /sys/bus/pci/drivers/vfio-pci/bind [ 25.261941] vfio-pci 0000:00:00.0: arm_smmu_capable: smmu features 0xe9e [ 25.268877] vfio-pci 0000:00:00.0: vfio_group_find_or_alloc: device_iommu_capable() returned false [ 25.279271] vfio-pci 0000:00:00.0: vfio_pci_core_register_device: failed to register group dev: -EINVAL [ 25.301377] vfio-pci: probe of 0000:00:00.0 failed with error -22 The ARM_SMMU_FEAT_COHERENT_WALK feature is set in arm_smmu_device_dt_probe() if the OF node of the SMMU device was set as dma-coherent. In the case of LS1028A, it wasn't. Fix that. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- The LS1028A is not the only SoC affected by this, it seems. fsl-ls1088a.dtsi seems to be in the same situation where vfio-pci worked before. There are also other SoCs which don't have dma-coherent in the iommu node. There's also something I don't quite like about this patch technically introducing a regression which requires a device tree update. Can something different be done about that, or are LS1028A/LS1088A simply to blame because of breaching the dt-bindings contract, and in that case, I'll have to suck it up, put a Fixes tag here, write another patch for LS1088A, and resend? arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 1 + 1 file changed, 1 insertion(+)