iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
Robin Murphy
reviewerPATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
We now delay installing our per-bus iommu_ops until we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value of bus_set_iommu() back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning -EBUSY to the driver core and getting torn down again.
There are essentially 3 cases in which bus_set_iommu() returns nonzero:
1. The bus already has iommu_ops installed
2. One of the add_device callbacks from the initial notifier failed
3. Allocating or installing the notifier itself failed
The first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/iommu/arm-smmu-v3.c +4 -7
@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ; }static int arm_smmu_device_remove (struct platform_device *pdev)
Signature · 1 line
1.9.1
Robin Murphy
reviewerPATCH 2/2 iommu/arm-smmu: Work around ARM DMA configuration
The 32-bit ARM DMA configuration code predates the IOMMU core's default domain functionality, and instead relies on allocating its own domains and attaching any devices using the generic IOMMU binding to them. Unfortunately, it does this relatively early on in the creation of the device, before we've seen our add_device callback, which leads us to attempt to operate on a half-configured master.
To avoid a crash, check for this situation on attach, but refuse to play, as there's nothing we can do. This at least allows VFIO to keep working for people who update their 32-bit DTs to the generic binding, albeit with a few (innocuous) warnings from the DMA layer on boot.
--- drivers/iommu/arm-smmu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/iommu/arm-smmu.c +10 -0
@@ -1228,6 +1228,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)return -ENXIO ; } +/* +* FIXME: The arch/arm DMA API code tries to attach devices to its own +* domains between of_xlate() and add_device() - we have no way to cope +* with that, so until ARM gets converted to rely on groups and default +* domains, just say no (but more politely than by dereferencing NULL). +* This should be at least a WARN_ON once that's sorted. +*/ +if (!fwspec->iommu_priv) +return -ENODEV ; + smmu =fwspec_smmu (fwspec);/* Ensure that the domain is finalised */ ret =arm_smmu_init_domain_context (domain, smmu);
Signature · 1 line
1.9.1
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
In reply to Robin MurphyQuoted 37 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-3@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +3
-4#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;
Nit: I do not see why you would not take the same approach as the ARM SMMUv1/v2, namely checking if ops are already set and skip the call if that's the case.
Anyway:
Quoted 6 lines
drivers/iommu/arm-smmu-v3.c +0
-0}static int arm_smmu_device_remove (struct platform_device *pdev)Signature · 1 line
1.9.1
Robin Murphy
reviewerPATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
Hi Lorenzo,
Quoted 44 lines · Lorenzo Pieralisi
Quoted 43 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +2
-6@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +2
-1#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;Nit: I do not see why you would not take the same approach as the ARM SMMUv1/v2, namely checking if ops are already set and skip the call if that's the case.
Well, I'd say it really goes the other way around - since the very first thing bus_set_iommu() does is check if ops are present, and return if so, and the v2 driver already doesn't care about that return value, there's not really any need for it to duplicate the check either. I didn't change it at the time to avoid cluttering the gigantic rework any further, but I could spin a cleanup patch if you like.
Quoted 3 lines
Anyway:
Thanks!
Robin.
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
Quoted 56 lines · Robin Murphy
Hi Lorenzo,
Quoted 44 lines · Lorenzo Pieralisi
Quoted 43 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +2
-6@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +2
-1#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;Nit: I do not see why you would not take the same approach as the ARM SMMUv1/v2, namely checking if ops are already set and skip the call if that's the case.
Quoted 6 lines
Well, I'd say it really goes the other way around - since the very first thing
bus_set_iommu()does is check if ops are present, and return if so, and the v2 driver already doesn't care about that return value, there's not really any need for it to duplicate the check either. I didn't change it at the time to avoid cluttering the gigantic rework any further, but I could spin a cleanup patch if you like.
No worries, it was to understand if there was a reason to keep the code different and after another look I agree with what you are saying (by checking if ops are present you could eg avoid calling pci_request_acs() every probe but that's a detail).
Thanks for fixing it !
Lorenzo
Quoted 8 lines
Quoted 3 lines
Anyway:
Thanks!
Robin.
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
In reply to Robin MurphyHi,
Quoted 44 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Robin Murphy
Sent: Monday, October 17, 2016 4:36 PM
To: will.deacon at arm.com; joro at 8bytes.org
Cc: iommu at lists.linux-foundation.org; Lorenzo Pieralisi <[email protected]>; linux-arm-kernel at lists.infradead.org
Subject: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3sWe now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
drivers/iommu/arm-smmu-v3.c| 11 ++++-------1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-3@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +3
-4#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;
Regards,
Sricharan
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
In reply to Robin MurphyQuoted 37 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-3@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +3
-4#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;
In which case, we should probably add an iommu_present check, like we have for the v2 driver.
Will
Robin Murphy
reviewerPATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
Quoted 43 lines · Will Deacon
Quoted 42 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-6@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret;
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 5 lines
drivers/iommu/arm-smmu-v3.c +3
-1+bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;In which case, we should probably add an
iommu_presentcheck, like we have for the v2 driver.
As touched upon in the commit message, the first thing bus_set_iommu() does is perform that same check and return immediately if any ops are already set. Do you really want redundant checks added, or shall I spin that cleanup patch removing them from v2 that I proposed to Lorenzo?
Robin.
Quoted 3 lines
Will
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
Quoted 53 lines · Robin Murphy
Quoted 43 lines · Will Deacon
Quoted 42 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failedThe first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-6@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)of_iommu_set_ops (dev->of_node, &arm_smmu_ops);#ifdef CONFIG_PCI pci_request_acs (); - ret =bus_set_iommu (&pci_bus_type, &arm_smmu_ops); -if (ret) -return ret; +bus_set_iommu (&pci_bus_type, &arm_smmu_ops);#endif #ifdef CONFIG_ARM_AMBA - ret =bus_set_iommu (&amba_bustype, &arm_smmu_ops); -if (ret) -return ret;Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_dt_probe(struct platform_device *pdev) · line 2646
Quoted 5 lines
drivers/iommu/arm-smmu-v3.c +3
-1+bus_set_iommu (&amba_bustype, &arm_smmu_ops);#endif -return bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +bus_set_iommu (&platform_bus_type, &arm_smmu_ops); +return 0 ;In which case, we should probably add an
iommu_presentcheck, like we have for the v2 driver.
Quoted 4 lines
As touched upon in the commit message, the first thing
bus_set_iommu()does is perform that same check and return immediately if any ops are already set. Do you really want redundant checks added, or shall I spin that cleanup patch removing them from v2 that I proposed to Lorenzo?
A cleanup patch sounds good, to keep the two drivers consistent.
Will
PATCH 1/2 iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s
In reply to Robin MurphyQuoted 13 lines · Robin Murphy
We now delay installing our per-bus
iommu_opsuntil we know an SMMU has successfully probed, as they don't serve much purpose beforehand, and doing so also avoids fights between multiple IOMMU drivers in a single kernel. However, the upshot of passing the return value ofbus_set_iommu()back from our probe function is that if there happens to be more than one SMMUv3 device in a system, the second and subsequent probes will wind up returning-EBUSYto the driver core and getting torn down again.There are essentially 3 cases in which
bus_set_iommu()returns nonzero:
1. The bus already hasiommu_opsinstalled
2. One of theadd_devicecallbacks from the initial notifier failed
3. Allocating or installing the notifier itself failed
Quoted 8 lines
The first two are down to devices other than the SMMU in question, so shouldn't abort an otherwise-successful SMMU probe, whilst the third is indicative of the kind of catastrophic system failure which isn't going to get much further anyway. Consequently, there is little harm in ignoring the return value either way.
CC Lorenzo Pieralisi <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Thanks
Hanjun