[PATCH v2 0/9] iommu: Permit modular builds of ARM SMMU[v3] drivers

[PATCH v2 0/9] iommu: Permit modular builds of ARM SMMU[v3] drivers

[Will Deacon]

Hi all,

This is version two of the patches I previously posted here:

  https://lore.kernel.org/lkml/20191030145112.19738-1-will@kernel.org/

Changes since v1 include:

  * Build single "arm-smmu-mod.ko" module for the Arm SMMU driver
  * Hold a reference to the IOMMU driver module across {add,remove}_device()
  * Take a reference to the IOMMU driver module during of_xlate()
  * Added Bjorn's ack on the PCI export patch

Please note that I haven't been able to test this properly, since I don't
currently have access to any Arm SMMU hardware.

Cheers,

Will

Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: Jordan Crouse <jcrouse@codeaurora.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Saravana Kannan <saravanak@google.com>
Cc: "Isaac J. Manjarres" <isaacm@codeaurora.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>

--->8

Will Deacon (9):
  drivers/iommu: Export core IOMMU API symbols to permit modular drivers
  iommu/of: Request ACS from the PCI core when configuring IOMMU linkage
  PCI: Export pci_ats_disabled() as a GPL symbol to modules
  drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()
  iommu/of: Take a ref to the IOMMU driver during ->of_xlate()
  Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
  iommu/arm-smmu-v3: Allow building as a module
  Revert "iommu/arm-smmu: Make arm-smmu explicitly non-modular"
  iommu/arm-smmu: Allow building as a module

 drivers/iommu/Kconfig       | 16 ++++++-
 drivers/iommu/Makefile      |  3 +-
 drivers/iommu/arm-smmu-v3.c | 27 +++++++-----
 drivers/iommu/arm-smmu.c    | 87 ++++++++++++++++++++++---------------
 drivers/iommu/iommu-sysfs.c |  5 +++
 drivers/iommu/iommu.c       | 27 +++++++++++-
 drivers/iommu/of_iommu.c    | 17 +++++---
 drivers/pci/pci.c           |  1 +
 include/linux/iommu.h       |  2 +
 9 files changed, 130 insertions(+), 55 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 1/9] drivers/iommu: Export core IOMMU API symbols to permit modular drivers

[Will Deacon]

Building IOMMU drivers as modules requires that the core IOMMU API
symbols are exported as GPL symbols.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu-sysfs.c | 5 +++++
 drivers/iommu/iommu.c       | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index e436ff813e7e..99869217fbec 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -87,6 +87,7 @@ int iommu_device_sysfs_add(struct iommu_device *iommu,
 	put_device(iommu->dev);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(iommu_device_sysfs_add);
 
 void iommu_device_sysfs_remove(struct iommu_device *iommu)
 {
@@ -94,6 +95,8 @@ void iommu_device_sysfs_remove(struct iommu_device *iommu)
 	device_unregister(iommu->dev);
 	iommu->dev = NULL;
 }
+EXPORT_SYMBOL_GPL(iommu_device_sysfs_remove);
+
 /*
  * IOMMU drivers can indicate a device is managed by a given IOMMU using
  * this interface.  A link to the device will be created in the "devices"
@@ -119,6 +122,7 @@ int iommu_device_link(struct iommu_device *iommu, struct device *link)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(iommu_device_link);
 
 void iommu_device_unlink(struct iommu_device *iommu, struct device *link)
 {
@@ -128,3 +132,4 @@ void iommu_device_unlink(struct iommu_device *iommu, struct device *link)
 	sysfs_remove_link(&link->kobj, "iommu");
 	sysfs_remove_link_from_group(&iommu->dev->kobj, "devices", dev_name(link));
 }
+EXPORT_SYMBOL_GPL(iommu_device_unlink);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d658c7c6a2ab..c1aadb570145 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -141,6 +141,7 @@ int iommu_device_register(struct iommu_device *iommu)
 	spin_unlock(&iommu_device_lock);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(iommu_device_register);
 
 void iommu_device_unregister(struct iommu_device *iommu)
 {
@@ -148,6 +149,7 @@ void iommu_device_unregister(struct iommu_device *iommu)
 	list_del(&iommu->list);
 	spin_unlock(&iommu_device_lock);
 }
+EXPORT_SYMBOL_GPL(iommu_device_unregister);
 
 static struct iommu_param *iommu_get_dev_param(struct device *dev)
 {
@@ -886,6 +888,7 @@ struct iommu_group *iommu_group_ref_get(struct iommu_group *group)
 	kobject_get(group->devices_kobj);
 	return group;
 }
+EXPORT_SYMBOL_GPL(iommu_group_ref_get);
 
 /**
  * iommu_group_put - Decrement group reference
@@ -1259,6 +1262,7 @@ struct iommu_group *generic_device_group(struct device *dev)
 {
 	return iommu_group_alloc();
 }
+EXPORT_SYMBOL_GPL(generic_device_group);
 
 /*
  * Use standard PCI bus topology, isolation features, and DMA alias quirks
@@ -1326,6 +1330,7 @@ struct iommu_group *pci_device_group(struct device *dev)
 	/* No shared group found, allocate new */
 	return iommu_group_alloc();
 }
+EXPORT_SYMBOL_GPL(pci_device_group);
 
 /* Get the IOMMU group for device on fsl-mc bus */
 struct iommu_group *fsl_mc_device_group(struct device *dev)
@@ -1338,6 +1343,7 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
 		group = iommu_group_alloc();
 	return group;
 }
+EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
 /**
  * iommu_group_get_for_dev - Find or create the IOMMU group for a device
@@ -1406,6 +1412,7 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 
 	return group;
 }
+EXPORT_SYMBOL(iommu_group_get_for_dev);
 
 struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 {
@@ -2185,6 +2192,7 @@ struct iommu_resv_region *iommu_alloc_resv_region(phys_addr_t start,
 	region->type = type;
 	return region;
 }
+EXPORT_SYMBOL_GPL(iommu_alloc_resv_region);
 
 static int
 request_default_domain_for_dev(struct device *dev, unsigned long type)
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 2/9] iommu/of: Request ACS from the PCI core when configuring IOMMU linkage

[Will Deacon]

To avoid having to export 'pci_request_acs()' to modular IOMMU drivers,
move the call into the 'of_dma_configure()' path in a similar manner to
the way in which ACS is configured when probing via ACPI/IORT.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/of_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 614a93aa5305..78faa9f73a91 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -177,6 +177,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 			.np = master_np,
 		};
 
+		pci_request_acs();
 		err = pci_for_each_dma_alias(to_pci_dev(dev),
 					     of_pci_iommu_init, &info);
 	} else if (dev_is_fsl_mc(dev)) {
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 3/9] PCI: Export pci_ats_disabled() as a GPL symbol to modules

[Will Deacon]

Building drivers for ATS-aware IOMMUs as modules requires access to
pci_ats_disabled(). Export it as a GPL symbol to get things working.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/pci/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a527..4fbe5b576dd8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -123,6 +123,7 @@ bool pci_ats_disabled(void)
 {
 	return pcie_ats_disabled;
 }
+EXPORT_SYMBOL_GPL(pci_ats_disabled);
 
 /* Disable bridge_d3 for all PCIe ports */
 static bool pci_bridge_d3_disable;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 4/9] drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()

[Will Deacon]

To avoid accidental removal of an active IOMMU driver module, take a
reference to the driver module in 'iommu_probe_device()' immediately
prior to invoking the '->add_device()' callback and hold it until the
after the device has been removed by '->remove_device()'.

Suggested-by: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu.c | 19 +++++++++++++++++--
 include/linux/iommu.h |  2 ++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c1aadb570145..4bfecfbbe2cf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -22,6 +22,7 @@
 #include <linux/bitops.h>
 #include <linux/property.h>
 #include <linux/fsl/mc.h>
+#include <linux/module.h>
 #include <trace/events/iommu.h>
 
 static struct kset *iommu_group_kset;
@@ -185,10 +186,21 @@ int iommu_probe_device(struct device *dev)
 	if (!iommu_get_dev_param(dev))
 		return -ENOMEM;
 
+	if (!try_module_get(ops->owner)) {
+		ret = -EINVAL;
+		goto err_free_dev_param;
+	}
+
 	ret = ops->add_device(dev);
 	if (ret)
-		iommu_free_dev_param(dev);
+		goto err_module_put;
+
+	return 0;
 
+err_module_put:
+	module_put(ops->owner);
+err_free_dev_param:
+	iommu_free_dev_param(dev);
 	return ret;
 }
 
@@ -199,7 +211,10 @@ void iommu_release_device(struct device *dev)
 	if (dev->iommu_group)
 		ops->remove_device(dev);
 
-	iommu_free_dev_param(dev);
+	if (dev->iommu_param) {
+		module_put(ops->owner);
+		iommu_free_dev_param(dev);
+	}
 }
 
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 29bac5345563..d9dab5a3e912 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -245,6 +245,7 @@ struct iommu_iotlb_gather {
  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
  * @pgsize_bitmap: bitmap of all possible supported page sizes
+ * @owner: Driver module providing these ops
  */
 struct iommu_ops {
 	bool (*capable)(enum iommu_cap);
@@ -308,6 +309,7 @@ struct iommu_ops {
 			     struct iommu_page_response *msg);
 
 	unsigned long pgsize_bitmap;
+	struct module *owner;
 };
 
 /**
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 5/9] iommu/of: Take a ref to the IOMMU driver during ->of_xlate()

[Will Deacon]

Ensure that we hold a reference to the IOMMU driver module while calling
the '->of_xlate()' callback during early device probing.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/of_iommu.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 78faa9f73a91..25491403a0bd 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -8,6 +8,7 @@
 #include <linux/export.h>
 #include <linux/iommu.h>
 #include <linux/limits.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 #include <linux/of_pci.h>
@@ -89,16 +90,16 @@ static int of_iommu_xlate(struct device *dev,
 {
 	const struct iommu_ops *ops;
 	struct fwnode_handle *fwnode = &iommu_spec->np->fwnode;
-	int err;
+	int ret;
 
 	ops = iommu_ops_from_fwnode(fwnode);
 	if ((ops && !ops->of_xlate) ||
 	    !of_device_is_available(iommu_spec->np))
 		return NO_IOMMU;
 
-	err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops);
-	if (err)
-		return err;
+	ret = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops);
+	if (ret)
+		return ret;
 	/*
 	 * The otherwise-empty fwspec handily serves to indicate the specific
 	 * IOMMU device we're waiting for, which will be useful if we ever get
@@ -107,7 +108,12 @@ static int of_iommu_xlate(struct device *dev,
 	if (!ops)
 		return driver_deferred_probe_check_state(dev);
 
-	return ops->of_xlate(dev, iommu_spec);
+	if (!try_module_get(ops->owner))
+		return -ENODEV;
+
+	ret = ops->of_xlate(dev, iommu_spec);
+	module_put(ops->owner);
+	return ret;
 }
 
 struct of_pci_iommu_alias_info {
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[Will Deacon]

This reverts commit c07b6426df922d21a13a959cf785d46e9c531941.

Let's get the SMMUv3 driver building as a module, which means putting
back some dead code that we used to carry.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/arm-smmu-v3.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8da93e730d6f..2ad8e2ca0583 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -21,8 +21,7 @@
 #include <linux/io-pgtable.h>
 #include <linux/iommu.h>
 #include <linux/iopoll.h>
-#include <linux/init.h>
-#include <linux/moduleparam.h>
+#include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -384,10 +383,6 @@
 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000
 
-/*
- * not really modular, but the easiest way to keep compat with existing
- * bootargs behaviour is to continue using module_param_named here.
- */
 static bool disable_bypass = 1;
 module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_bypass,
@@ -3683,25 +3678,37 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
+static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
 	arm_smmu_device_disable(smmu);
+
+	return 0;
+}
+
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+	arm_smmu_device_remove(pdev);
 }
 
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v3", },
 	{ },
 };
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
 		.name		= "arm-smmu-v3",
 		.of_match_table	= of_match_ptr(arm_smmu_of_match),
-		.suppress_bind_attrs = true,
 	},
 	.probe	= arm_smmu_device_probe,
+	.remove	= arm_smmu_device_remove,
 	.shutdown = arm_smmu_device_shutdown,
 };
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
+MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 7/9] iommu/arm-smmu-v3: Allow building as a module

[Will Deacon]

By removing the redundant call to 'pci_request_acs()' we can allow the
ARM SMMUv3 driver to be built as a module.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/Kconfig       | 2 +-
 drivers/iommu/arm-smmu-v3.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index e3842eabcfdd..7583d47fc4d5 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -388,7 +388,7 @@ config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT
 	  config.
 
 config ARM_SMMU_V3
-	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
+	tristate "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
 	depends on ARM64
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2ad8e2ca0583..d54ceb80c28d 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2733,6 +2733,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.get_resv_regions	= arm_smmu_get_resv_regions,
 	.put_resv_regions	= arm_smmu_put_resv_regions,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
+	.owner			= THIS_MODULE,
 };
 
 /* Probing and initialisation functions */
@@ -3657,7 +3658,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 
 #ifdef CONFIG_PCI
 	if (pci_bus_type.iommu_ops != &arm_smmu_ops) {
-		pci_request_acs();
 		ret = bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 		if (ret)
 			return ret;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 8/9] Revert "iommu/arm-smmu: Make arm-smmu explicitly non-modular"

[Will Deacon]

This reverts commit addb672f200f4e99368270da205320b83efe01a0.

Let's get the SMMU driver building as a module, which means putting
back some dead code that we used to carry.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7c503a6bc585..53bbe0663b9e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -27,8 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
-#include <linux/init.h>
-#include <linux/moduleparam.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -59,10 +58,6 @@
 #define MSI_IOVA_LENGTH			0x100000
 
 static int force_stage;
-/*
- * not really modular, but the easiest way to keep compat with existing
- * bootargs behaviour is to continue using module_param() here.
- */
 module_param(force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
@@ -1878,6 +1873,7 @@ static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "qcom,smmu-v2", .data = &qcom_smmuv2 },
 	{ },
 };
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 #ifdef CONFIG_ACPI
 static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
@@ -2165,12 +2161,12 @@ static int arm_smmu_legacy_bus_init(void)
 }
 device_initcall_sync(arm_smmu_legacy_bus_init);
 
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
+static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
 	if (!smmu)
-		return;
+		return -ENODEV;
 
 	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
 		dev_err(&pdev->dev, "removing device with active domains!\n");
@@ -2186,6 +2182,12 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
 		clk_bulk_disable(smmu->num_clks, smmu->clks);
 
 	clk_bulk_unprepare(smmu->num_clks, smmu->clks);
+	return 0;
+}
+
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+	arm_smmu_device_remove(pdev);
 }
 
 static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
@@ -2235,12 +2237,16 @@ static const struct dev_pm_ops arm_smmu_pm_ops = {
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {
-		.name			= "arm-smmu",
-		.of_match_table		= of_match_ptr(arm_smmu_of_match),
-		.pm			= &arm_smmu_pm_ops,
-		.suppress_bind_attrs	= true,
+		.name		= "arm-smmu",
+		.of_match_table	= of_match_ptr(arm_smmu_of_match),
+		.pm		= &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
+	.remove	= arm_smmu_device_remove,
 	.shutdown = arm_smmu_device_shutdown,
 };
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
+MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v2 9/9] iommu/arm-smmu: Allow building as a module

[Will Deacon]

By conditionally dropping support for the legacy binding and exporting
the newly introduced 'arm_smmu_impl_init()' function we can allow the
ARM SMMU driver to be built as a module.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/Kconfig    | 14 +++++++++-
 drivers/iommu/Makefile   |  3 ++-
 drivers/iommu/arm-smmu.c | 55 ++++++++++++++++++++++++----------------
 3 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 7583d47fc4d5..fc55f7ba0d18 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -350,7 +350,7 @@ config SPAPR_TCE_IOMMU
 
 # ARM IOMMU support
 config ARM_SMMU
-	bool "ARM Ltd. System MMU (SMMU) Support"
+	tristate "ARM Ltd. System MMU (SMMU) Support"
 	depends on (ARM64 || ARM) && MMU
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
@@ -362,6 +362,18 @@ config ARM_SMMU
 	  Say Y here if your SoC includes an IOMMU device implementing
 	  the ARM SMMU architecture.
 
+config ARM_SMMU_LEGACY_DT_BINDINGS
+	bool "Support the legacy \"mmu-masters\" devicetree bindings"
+	depends on ARM_SMMU=y && OF
+	help
+	  Support for the badly designed and deprecated "mmu-masters"
+	  devicetree bindings. This allows some DMA masters to attach
+	  to the SMMU but does not provide any support via the DMA API.
+	  If you're lucky, you might be able to get VFIO up and running.
+
+	  If you say Y here then you'll make me very sad. Instead, say N
+	  and move your firmware to the utopian future that was 2016.
+
 config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT
 	bool "Default to disabling bypass on ARM SMMU v1 and v2"
 	depends on ARM_SMMU
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 4f405f926e73..b52a03d87fc3 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -13,7 +13,8 @@ obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o amd_iommu_quirks.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd_iommu_debugfs.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
-obj-$(CONFIG_ARM_SMMU) += arm-smmu.o arm-smmu-impl.o
+obj-$(CONFIG_ARM_SMMU) += arm-smmu-mod.o
+arm-smmu-mod-objs += arm-smmu.o arm-smmu-impl.o
 obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
 obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o intel-pasid.o
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 53bbe0663b9e..9eb52410d016 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -125,6 +125,12 @@ static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 	return container_of(dom, struct arm_smmu_domain, domain);
 }
 
+static struct platform_driver arm_smmu_driver;
+static struct iommu_ops arm_smmu_ops;
+
+#ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS
+static void arm_smmu_bus_init(void);
+
 static struct device_node *dev_get_dev_node(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
@@ -160,9 +166,6 @@ static int __find_legacy_master_phandle(struct device *dev, void *data)
 	return err == -ENOENT ? 0 : err;
 }
 
-static struct platform_driver arm_smmu_driver;
-static struct iommu_ops arm_smmu_ops;
-
 static int arm_smmu_register_legacy_master(struct device *dev,
 					   struct arm_smmu_device **smmu)
 {
@@ -214,6 +217,27 @@ static int arm_smmu_register_legacy_master(struct device *dev,
 	return err;
 }
 
+/*
+ * With the legacy DT binding in play, we have no guarantees about
+ * probe order, but then we're also not doing default domains, so we can
+ * delay setting bus ops until we're sure every possible SMMU is ready,
+ * and that way ensure that no add_device() calls get missed.
+ */
+static int arm_smmu_legacy_bus_init(void)
+{
+	if (using_legacy_binding)
+		arm_smmu_bus_init();
+	return 0;
+}
+device_initcall_sync(arm_smmu_legacy_bus_init);
+#else
+static int arm_smmu_register_legacy_master(struct device *dev,
+					   struct arm_smmu_device **smmu)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS */
+
 static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
 {
 	int idx;
@@ -1566,6 +1590,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.get_resv_regions	= arm_smmu_get_resv_regions,
 	.put_resv_regions	= arm_smmu_put_resv_regions,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
+	.owner			= THIS_MODULE,
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
@@ -1960,8 +1985,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 
 	legacy_binding = of_find_property(dev->of_node, "mmu-masters", NULL);
 	if (legacy_binding && !using_generic_binding) {
-		if (!using_legacy_binding)
-			pr_notice("deprecated \"mmu-masters\" DT property in use; DMA API support unavailable\n");
+		if (!using_legacy_binding) {
+			pr_notice("deprecated \"mmu-masters\" DT property in use; %s support unavailable\n",
+				  IS_ENABLED(CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS) ? "DMA API" : "SMMU");
+		}
 		using_legacy_binding = true;
 	} else if (!legacy_binding && !using_legacy_binding) {
 		using_generic_binding = true;
@@ -1986,10 +2013,8 @@ static void arm_smmu_bus_init(void)
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 #endif
 #ifdef CONFIG_PCI
-	if (!iommu_present(&pci_bus_type)) {
-		pci_request_acs();
+	if (!iommu_present(&pci_bus_type))
 		bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
-	}
 #endif
 #ifdef CONFIG_FSL_MC_BUS
 	if (!iommu_present(&fsl_mc_bus_type))
@@ -2147,20 +2172,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	return 0;
 }
 
-/*
- * With the legacy DT binding in play, though, we have no guarantees about
- * probe order, but then we're also not doing default domains, so we can
- * delay setting bus ops until we're sure every possible SMMU is ready,
- * and that way ensure that no add_device() calls get missed.
- */
-static int arm_smmu_legacy_bus_init(void)
-{
-	if (using_legacy_binding)
-		arm_smmu_bus_init();
-	return 0;
-}
-device_initcall_sync(arm_smmu_legacy_bus_init);
-
 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[John Garry]

On 08/11/2019 15:16, Will Deacon wrote:
> +MODULE_DEVICE_TABLE(of, arm_smmu_of_match);

Hi Will,

>   
>   static struct platform_driver arm_smmu_driver = {
>   	.driver	= {
>   		.name		= "arm-smmu-v3",
>   		.of_match_table	= of_match_ptr(arm_smmu_of_match),
> -		.suppress_bind_attrs = true,

Does this mean that we can now manually unbind this driver from the SMMU 
device?

Seems dangerous. Here's what happens for me:

root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo 
arm-smmu-v3.0.auto > unbind
[   77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
ho [   78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at 
0x00000146 [hwprod 0x00000146, hwcons 0x00000000]

>   	},
>   	.probe	= arm_smmu_device_probe,
> +	.remove	= arm_smmu_device_remove,
>   	.shutdown = arm_smmu_device_shutdown,
>   };
> -builtin_platform_driver(arm_smmu_driver);
> +module_platform_driver(arm_smmu_driver);
> +


Thanks,
John

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[John Garry]

On 08/11/2019 16:17, John Garry wrote:
> On 08/11/2019 15:16, Will Deacon wrote:
>> +MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> 
> Hi Will,
> 
>>   static struct platform_driver arm_smmu_driver = {
>>       .driver    = {
>>           .name        = "arm-smmu-v3",
>>           .of_match_table    = of_match_ptr(arm_smmu_of_match),
>> -        .suppress_bind_attrs = true,
> 
> Does this mean that we can now manually unbind this driver from the SMMU 
> device?
> 
> Seems dangerous. Here's what happens for me:
> 
> root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
> ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo 
> arm-smmu-v3.0.auto > unbind
> [   77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
> ho [   78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at 
> 0x00000146 [hwprod 0x00000146, hwcons 0x00000000]
> 
>>       },
>>       .probe    = arm_smmu_device_probe,
>> +    .remove    = arm_smmu_device_remove,
>>       .shutdown = arm_smmu_device_shutdown,
>>   };
>> -builtin_platform_driver(arm_smmu_driver);
>> +module_platform_driver(arm_smmu_driver);
>> +

BTW, it now looks like it was your v1 series I was testing there, on 
your branch iommu/module. It would be helpful to update for ease of testing.

Thanks,
John

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[Will Deacon]

On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
> On 08/11/2019 16:17, John Garry wrote:
> > On 08/11/2019 15:16, Will Deacon wrote:
> > > +MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> > 
> > Hi Will,
> > 
> > >   static struct platform_driver arm_smmu_driver = {
> > >       .driver    = {
> > >           .name        = "arm-smmu-v3",
> > >           .of_match_table    = of_match_ptr(arm_smmu_of_match),
> > > -        .suppress_bind_attrs = true,
> > 
> > Does this mean that we can now manually unbind this driver from the SMMU
> > device?
> > 
> > Seems dangerous. Here's what happens for me:
> > 
> > root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
> > ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
> > arm-smmu-v3.0.auto > unbind
> > [   77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
> > ho [   78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
> > 0x00000146 [hwprod 0x00000146, hwcons 0x00000000]
> > 
> > >       },
> > >       .probe    = arm_smmu_device_probe,
> > > +    .remove    = arm_smmu_device_remove,
> > >       .shutdown = arm_smmu_device_shutdown,
> > >   };
> > > -builtin_platform_driver(arm_smmu_driver);
> > > +module_platform_driver(arm_smmu_driver);
> > > +
> 
> BTW, it now looks like it was your v1 series I was testing there, on your
> branch iommu/module. It would be helpful to update for ease of testing.

Yes, sorry about that. I'll update it now (although I'm not sure it will
help with this -- I was going to see what happens with other devices such
as the intel-iommu or storage controllers)

Will

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[John Garry]

On 08/11/2019 16:47, Will Deacon wrote:
> On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
>> On 08/11/2019 16:17, John Garry wrote:
>>> On 08/11/2019 15:16, Will Deacon wrote:
>>>> +MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>
>>> Hi Will,
>>>
>>>>    static struct platform_driver arm_smmu_driver = {
>>>>        .driver    = {
>>>>            .name        = "arm-smmu-v3",
>>>>            .of_match_table    = of_match_ptr(arm_smmu_of_match),
>>>> -        .suppress_bind_attrs = true,
>>>
>>> Does this mean that we can now manually unbind this driver from the SMMU
>>> device?
>>>
>>> Seems dangerous. Here's what happens for me:
>>>
>>> root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
>>> ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
>>> arm-smmu-v3.0.auto > unbind
>>> [   77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
>>> ho [   78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
>>> 0x00000146 [hwprod 0x00000146, hwcons 0x00000000]
>>>
>>>>        },
>>>>        .probe    = arm_smmu_device_probe,
>>>> +    .remove    = arm_smmu_device_remove,
>>>>        .shutdown = arm_smmu_device_shutdown,
>>>>    };
>>>> -builtin_platform_driver(arm_smmu_driver);
>>>> +module_platform_driver(arm_smmu_driver);
>>>> +
>>
>> BTW, it now looks like it was your v1 series I was testing there, on your
>> branch iommu/module. It would be helpful to update for ease of testing.
> 

Hi Will,

> Yes, sorry about that. I'll update it now (although I'm not sure it will
> help with this -- I was going to see what happens with other devices such
> as the intel-iommu or storage controllers)

So I tried your v2 series for this - it has the same issue, as I 
anticipated.

It seems that some iommu drivers do call iommu_device_register(), so 
maybe a decent reference. Or simply stop the driver being unbound.

Cheers,
John

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[Will Deacon]

On Fri, Nov 08, 2019 at 05:25:09PM +0000, John Garry wrote:
> On 08/11/2019 16:47, Will Deacon wrote:
> > On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
> > > BTW, it now looks like it was your v1 series I was testing there, on your
> > > branch iommu/module. It would be helpful to update for ease of testing.
> > 
> > Yes, sorry about that. I'll update it now (although I'm not sure it will
> > help with this -- I was going to see what happens with other devices such
> > as the intel-iommu or storage controllers)
> 
> So I tried your v2 series for this - it has the same issue, as I
> anticipated.

Right, I'm just not sure how resilient drivers are expected to be to force
unbinding like this. You can break lots of stuff with root...

> It seems that some iommu drivers do call iommu_device_register(), so maybe a
> decent reference. Or simply stop the driver being unbound.

I'm not sure what you mean about iommu_device_register() (we call that
already), but I guess we can keep the '.suppress_bind_attrs = true' if
necessary. I'll have a play on my laptop and see how well that works if
you start unbinding stuff.

Will

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[Will Deacon]

On Fri, Nov 08, 2019 at 05:32:48PM +0000, Will Deacon wrote:
> On Fri, Nov 08, 2019 at 05:25:09PM +0000, John Garry wrote:
> > On 08/11/2019 16:47, Will Deacon wrote:
> > > On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
> > > > BTW, it now looks like it was your v1 series I was testing there, on your
> > > > branch iommu/module. It would be helpful to update for ease of testing.
> > > 
> > > Yes, sorry about that. I'll update it now (although I'm not sure it will
> > > help with this -- I was going to see what happens with other devices such
> > > as the intel-iommu or storage controllers)
> > 
> > So I tried your v2 series for this - it has the same issue, as I
> > anticipated.
> 
> Right, I'm just not sure how resilient drivers are expected to be to force
> unbinding like this. You can break lots of stuff with root...
> 
> > It seems that some iommu drivers do call iommu_device_register(), so maybe a
> > decent reference. Or simply stop the driver being unbound.
> 
> I'm not sure what you mean about iommu_device_register() (we call that
> already), but I guess we can keep the '.suppress_bind_attrs = true' if
> necessary. I'll have a play on my laptop and see how well that works if
> you start unbinding stuff.

So unbinding the nvme driver goes bang:

[90139.090158] nvme nvme0: failed to set APST feature (-19)
[90141.966780] Aborting journal on device dm-1-8.
[90141.967124] Buffer I/O error on dev dm-1, logical block 26247168, lost sync page write
[90141.967169] JBD2: Error -5 detected when updating journal superblock for dm-1-8.
[90141.967403] Buffer I/O error on dev dm-1, logical block 0, lost sync page write
[90141.967454] EXT4-fs (dm-1): I/O error while writing superblock
[90141.967467] EXT4-fs error (device dm-1): ext4_journal_check_start:61: Detected aborted journal
[90141.967473] EXT4-fs (dm-1): Remounting filesystem read-only
[90141.967569] Buffer I/O error on dev dm-1, logical block 0, lost sync page write
[90141.967682] EXT4-fs (dm-1): I/O error while writing superblock

and I've not managed to recover the thing yet (it's stuck trying to reboot.)

What state was your system in after unbinding the SMMU?

Will

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[John Garry]

On 08/11/2019 17:32, Will Deacon wrote:
> On Fri, Nov 08, 2019 at 05:25:09PM +0000, John Garry wrote:
>> On 08/11/2019 16:47, Will Deacon wrote:
>>> On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
>>>> BTW, it now looks like it was your v1 series I was testing there, on your
>>>> branch iommu/module. It would be helpful to update for ease of testing.
>>>
>>> Yes, sorry about that. I'll update it now (although I'm not sure it will
>>> help with this -- I was going to see what happens with other devices such
>>> as the intel-iommu or storage controllers)
>>
>> So I tried your v2 series for this - it has the same issue, as I
>> anticipated.
> 
> Right, I'm just not sure how resilient drivers are expected to be to force
> unbinding like this. You can break lots of stuff with root...

For sure, but it is good practice to limit that.

I had to fix something like this recently, so know about it... another 
potential problem is use-after-frees, where your device managed memory 
is freed at removal but still registered somewhere.

> 
>> It seems that some iommu drivers do call iommu_device_register(), so maybe a
>> decent reference. Or simply stop the driver being unbound.
> 
> I'm not sure what you mean about iommu_device_register() (we call that
> already), 

Sorry, I meant to say iommu_device_unregister().

but I guess we can keep the '.suppress_bind_attrs = true' if
> necessary. 

It may be good to add it to older stable kernels also, pre c07b6426df92.

I'll have a play on my laptop and see how well that works if
> you start unbinding stuff.

Cheers,
John

Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"

[John Garry]

On 08/11/2019 17:48, Will Deacon wrote:
> On Fri, Nov 08, 2019 at 05:32:48PM +0000, Will Deacon wrote:
>> On Fri, Nov 08, 2019 at 05:25:09PM +0000, John Garry wrote:
>>> On 08/11/2019 16:47, Will Deacon wrote:
>>>> On Fri, Nov 08, 2019 at 04:44:25PM +0000, John Garry wrote:
>>>>> BTW, it now looks like it was your v1 series I was testing there, on your
>>>>> branch iommu/module. It would be helpful to update for ease of testing.
>>>>
>>>> Yes, sorry about that. I'll update it now (although I'm not sure it will
>>>> help with this -- I was going to see what happens with other devices such
>>>> as the intel-iommu or storage controllers)
>>>
>>> So I tried your v2 series for this - it has the same issue, as I
>>> anticipated.
>>
>> Right, I'm just not sure how resilient drivers are expected to be to force
>> unbinding like this. You can break lots of stuff with root...
>>
>>> It seems that some iommu drivers do call iommu_device_register(), so maybe a
>>> decent reference. Or simply stop the driver being unbound.
>>
>> I'm not sure what you mean about iommu_device_register() (we call that
>> already), but I guess we can keep the '.suppress_bind_attrs = true' if
>> necessary. I'll have a play on my laptop and see how well that works if
>> you start unbinding stuff.
> 
> So unbinding the nvme driver goes bang:
> 
> [90139.090158] nvme nvme0: failed to set APST feature (-19)
> [90141.966780] Aborting journal on device dm-1-8.
> [90141.967124] Buffer I/O error on dev dm-1, logical block 26247168, lost sync page write
> [90141.967169] JBD2: Error -5 detected when updating journal superblock for dm-1-8.
> [90141.967403] Buffer I/O error on dev dm-1, logical block 0, lost sync page write
> [90141.967454] EXT4-fs (dm-1): I/O error while writing superblock
> [90141.967467] EXT4-fs error (device dm-1): ext4_journal_check_start:61: Detected aborted journal
> [90141.967473] EXT4-fs (dm-1): Remounting filesystem read-only
> [90141.967569] Buffer I/O error on dev dm-1, logical block 0, lost sync page write
> [90141.967682] EXT4-fs (dm-1): I/O error while writing superblock
> 
> and I've not managed to recover the thing yet (it's stuck trying to reboot.)
> 

Not surprised. I guess the device backing your root directory disappeared.

> What state was your system in after unbinding the SMMU?

Unusable again. For me the storage controller backing the root directory 
is compromised by disabling the SMMU unsafely.

Thanks,
John