diff mbox series

[v3,2/6] ACPI: Move IOMMU setup code out of IORT

Message ID 20210602154444.1077006-3-jean-philippe@linaro.org
State Superseded
Headers show
Series Add support for ACPI VIOT | expand

Commit Message

Jean-Philippe Brucker June 2, 2021, 3:44 p.m. UTC 
Extract the code that sets up the IOMMU infrastructure from IORT, since
it can be reused by VIOT. Move it one level up into a new
acpi_iommu_configure_id() function, which calls the IORT parsing
function which in turn calls the acpi_iommu_fwspec_init() helper.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

---
 include/acpi/acpi_bus.h   |  3 ++
 include/linux/acpi_iort.h |  8 ++---
 drivers/acpi/arm64/iort.c | 75 +++++----------------------------------
 drivers/acpi/scan.c       | 55 +++++++++++++++++++++++++++-
 4 files changed, 69 insertions(+), 72 deletions(-)

-- 
2.31.1

Comments

kernel test robot June 2, 2021, 8:06 p.m. UTC | #1 
Hi Jean-Philippe,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on iommu/next arm64/for-next/core linus/master v5.13-rc4 next-20210602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jean-Philippe-Brucker/Add-support-for-ACPI-VIOT/20210602-235849
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-a012-20210602 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d41cb6bb2607fa5c7a9df2b3dab361353657d225)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/937da71a81108243877fb1f0f568e56a08a62c50
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Philippe-Brucker/Add-support-for-ACPI-VIOT/20210602-235849
        git checkout 937da71a81108243877fb1f0f568e56a08a62c50
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/acpi/scan.c:1540:26: error: no member named 'ops' in 'struct iommu_fwspec'

           return fwspec ? fwspec->ops : NULL;
                           ~~~~~~  ^
>> drivers/acpi/scan.c:1564:9: error: implicit declaration of function 'iommu_probe_device' [-Werror,-Wimplicit-function-declaration]

                   err = iommu_probe_device(dev);
                         ^
   2 errors generated.


vim +1540 drivers/acpi/scan.c

  1535	
  1536	static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
  1537	{
  1538		struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
  1539	
> 1540		return fwspec ? fwspec->ops : NULL;

  1541	}
  1542	
  1543	static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
  1544							       const u32 *id_in)
  1545	{
  1546		int err;
  1547		const struct iommu_ops *ops;
  1548	
  1549		/*
  1550		 * If we already translated the fwspec there is nothing left to do,
  1551		 * return the iommu_ops.
  1552		 */
  1553		ops = acpi_iommu_fwspec_ops(dev);
  1554		if (ops)
  1555			return ops;
  1556	
  1557		err = iort_iommu_configure_id(dev, id_in);
  1558	
  1559		/*
  1560		 * If we have reason to believe the IOMMU driver missed the initial
  1561		 * add_device callback for dev, replay it to get things in order.
  1562		 */
  1563		if (!err && dev->bus && !device_iommu_mapped(dev))
> 1564			err = iommu_probe_device(dev);

  1565	
  1566		/* Ignore all other errors apart from EPROBE_DEFER */
  1567		if (err == -EPROBE_DEFER) {
  1568			return ERR_PTR(err);
  1569		} else if (err) {
  1570			dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
  1571			return NULL;
  1572		}
  1573		return acpi_iommu_fwspec_ops(dev);
  1574	}
  1575	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jean-Philippe Brucker June 3, 2021, 7:26 a.m. UTC | #2 
On Thu, Jun 03, 2021 at 04:06:18AM +0800, kernel test robot wrote:
> >> drivers/acpi/scan.c:1540:26: error: no member named 'ops' in 'struct iommu_fwspec'

>            return fwspec ? fwspec->ops : NULL;

>                            ~~~~~~  ^

> >> drivers/acpi/scan.c:1564:9: error: implicit declaration of function 'iommu_probe_device' [-Werror,-Wimplicit-function-declaration]

>                    err = iommu_probe_device(dev);

>                          ^


These are only defined when CONFIG_IOMMU_API is set. IORT uses them inside
an #ifdef, I can do the same. Maybe moving these two functions to a new
drivers/acpi/iommu.c would be nicer, though.

Thanks,
Jean

>    2 errors generated.

> 

> 

> vim +1540 drivers/acpi/scan.c

> 

>   1535	

>   1536	static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)

>   1537	{

>   1538		struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

>   1539	

> > 1540		return fwspec ? fwspec->ops : NULL;

>   1541	}

>   1542	

>   1543	static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,

>   1544							       const u32 *id_in)

>   1545	{

>   1546		int err;

>   1547		const struct iommu_ops *ops;

>   1548	

>   1549		/*

>   1550		 * If we already translated the fwspec there is nothing left to do,

>   1551		 * return the iommu_ops.

>   1552		 */

>   1553		ops = acpi_iommu_fwspec_ops(dev);

>   1554		if (ops)

>   1555			return ops;

>   1556	

>   1557		err = iort_iommu_configure_id(dev, id_in);

>   1558	

>   1559		/*

>   1560		 * If we have reason to believe the IOMMU driver missed the initial

>   1561		 * add_device callback for dev, replay it to get things in order.

>   1562		 */

>   1563		if (!err && dev->bus && !device_iommu_mapped(dev))

> > 1564			err = iommu_probe_device(dev);

>   1565	

>   1566		/* Ignore all other errors apart from EPROBE_DEFER */

>   1567		if (err == -EPROBE_DEFER) {

>   1568			return ERR_PTR(err);

>   1569		} else if (err) {

>   1570			dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);

>   1571			return NULL;

>   1572		}

>   1573		return acpi_iommu_fwspec_ops(dev);

>   1574	}

>   1575	

> 

> ---

> 0-DAY CI Kernel Test Service, Intel Corporation

> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Joerg Roedel June 4, 2021, 3:22 p.m. UTC | #3 
On Thu, Jun 03, 2021 at 09:26:39AM +0200, Jean-Philippe Brucker wrote:
> These are only defined when CONFIG_IOMMU_API is set. IORT uses them inside

> an #ifdef, I can do the same. Maybe moving these two functions to a new

> drivers/acpi/iommu.c would be nicer, though.


Not sure what the ACPI maintainers and reviewers prefer, but I would
just #ifdef the functions and provide stubs in the #else path if
necessary.

Regards,

	Joerg
Jean-Philippe Brucker June 10, 2021, 7:26 a.m. UTC | #4 
On Fri, Jun 04, 2021 at 05:22:07PM +0200, Joerg Roedel wrote:
> On Thu, Jun 03, 2021 at 09:26:39AM +0200, Jean-Philippe Brucker wrote:

> > These are only defined when CONFIG_IOMMU_API is set. IORT uses them inside

> > an #ifdef, I can do the same. Maybe moving these two functions to a new

> > drivers/acpi/iommu.c would be nicer, though.

> 

> Not sure what the ACPI maintainers and reviewers prefer, but I would

> just #ifdef the functions and provide stubs in the #else path if

> necessary.


Yes, I'll resend with that

Thanks,
Jean
diff mbox series

Patch

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 3a82faac5767..41f092a269f6 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -588,6 +588,9 @@  struct acpi_pci_root {
 
 bool acpi_dma_supported(struct acpi_device *adev);
 enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
+int acpi_iommu_fwspec_init(struct device *dev, u32 id,
+			   struct fwnode_handle *fwnode,
+			   const struct iommu_ops *ops);
 int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
 		       u64 *size);
 int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index f7f054833afd..f1f0842a2cb2 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -35,8 +35,7 @@  void acpi_configure_pmsi_domain(struct device *dev);
 int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
 /* IOMMU interface */
 int iort_dma_get_ranges(struct device *dev, u64 *size);
-const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
-						const u32 *id_in);
+int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
 int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
 #else
@@ -50,9 +49,8 @@  static inline void acpi_configure_pmsi_domain(struct device *dev) { }
 /* IOMMU interface */
 static inline int iort_dma_get_ranges(struct device *dev, u64 *size)
 { return -ENODEV; }
-static inline const struct iommu_ops *iort_iommu_configure_id(
-				      struct device *dev, const u32 *id_in)
-{ return NULL; }
+static inline int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
+{ return -ENODEV; }
 static inline
 int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 { return 0; }
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a940be1cf2af..b5b021e064b6 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -806,23 +806,6 @@  static struct acpi_iort_node *iort_get_msi_resv_iommu(struct device *dev)
 	return NULL;
 }
 
-static inline const struct iommu_ops *iort_fwspec_iommu_ops(struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-	return (fwspec && fwspec->ops) ? fwspec->ops : NULL;
-}
-
-static inline int iort_add_device_replay(struct device *dev)
-{
-	int err = 0;
-
-	if (dev->bus && !device_iommu_mapped(dev))
-		err = iommu_probe_device(dev);
-
-	return err;
-}
-
 /**
  * iort_iommu_msi_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
@@ -900,18 +883,6 @@  static inline bool iort_iommu_driver_enabled(u8 type)
 	}
 }
 
-static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
-			       struct fwnode_handle *fwnode,
-			       const struct iommu_ops *ops)
-{
-	int ret = iommu_fwspec_init(dev, fwnode, ops);
-
-	if (!ret)
-		ret = iommu_fwspec_add_ids(dev, &streamid, 1);
-
-	return ret;
-}
-
 static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
 {
 	struct acpi_iort_root_complex *pci_rc;
@@ -946,7 +917,7 @@  static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
 		return iort_iommu_driver_enabled(node->type) ?
 		       -EPROBE_DEFER : -ENODEV;
 
-	return arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
+	return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops);
 }
 
 struct iort_pci_alias_info {
@@ -1020,24 +991,14 @@  static int iort_nc_iommu_map_id(struct device *dev,
  * @dev: device to configure
  * @id_in: optional input id const value pointer
  *
- * Returns: iommu_ops pointer on configuration success
- *          NULL on configuration failure
+ * Returns: 0 on success, <0 on failure
  */
-const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
-						const u32 *id_in)
+int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
 {
 	struct acpi_iort_node *node;
-	const struct iommu_ops *ops;
+	const struct iommu_ops *ops = NULL;
 	int err = -ENODEV;
 
-	/*
-	 * If we already translated the fwspec there
-	 * is nothing left to do, return the iommu_ops.
-	 */
-	ops = iort_fwspec_iommu_ops(dev);
-	if (ops)
-		return ops;
-
 	if (dev_is_pci(dev)) {
 		struct iommu_fwspec *fwspec;
 		struct pci_bus *bus = to_pci_dev(dev)->bus;
@@ -1046,7 +1007,7 @@  const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 		node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
 				      iort_match_node_callback, &bus->dev);
 		if (!node)
-			return NULL;
+			return -ENODEV;
 
 		info.node = node;
 		err = pci_for_each_dma_alias(to_pci_dev(dev),
@@ -1059,7 +1020,7 @@  const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
 				      iort_match_node_callback, dev);
 		if (!node)
-			return NULL;
+			return -ENODEV;
 
 		err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) :
 			      iort_nc_iommu_map(dev, node);
@@ -1068,32 +1029,14 @@  const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 			iort_named_component_init(dev, node);
 	}
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * add_device callback for dev, replay it to get things in order.
-	 */
-	if (!err) {
-		ops = iort_fwspec_iommu_ops(dev);
-		err = iort_add_device_replay(dev);
-	}
-
-	/* Ignore all other errors apart from EPROBE_DEFER */
-	if (err == -EPROBE_DEFER) {
-		ops = ERR_PTR(err);
-	} else if (err) {
-		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
-		ops = NULL;
-	}
-
-	return ops;
+	return err;
 }
 
 #else
 int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 { return 0; }
-const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
-						const u32 *input_id)
-{ return NULL; }
+int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
+{ return -ENODEV; }
 #endif
 
 static int nc_dma_get_range(struct device *dev, u64 *size)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ea613df8f913..696b81432ae7 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -9,6 +9,7 @@ 
 #include <linux/kernel.h>
 #include <linux/acpi.h>
 #include <linux/acpi_iort.h>
+#include <linux/iommu.h>
 #include <linux/signal.h>
 #include <linux/kthread.h>
 #include <linux/dmi.h>
@@ -1520,6 +1521,58 @@  int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
 	return ret >= 0 ? 0 : ret;
 }
 
+int acpi_iommu_fwspec_init(struct device *dev, u32 id,
+			   struct fwnode_handle *fwnode,
+			   const struct iommu_ops *ops)
+{
+	int ret = iommu_fwspec_init(dev, fwnode, ops);
+
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &id, 1);
+
+	return ret;
+}
+
+static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+	return fwspec ? fwspec->ops : NULL;
+}
+
+static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
+						       const u32 *id_in)
+{
+	int err;
+	const struct iommu_ops *ops;
+
+	/*
+	 * If we already translated the fwspec there is nothing left to do,
+	 * return the iommu_ops.
+	 */
+	ops = acpi_iommu_fwspec_ops(dev);
+	if (ops)
+		return ops;
+
+	err = iort_iommu_configure_id(dev, id_in);
+
+	/*
+	 * If we have reason to believe the IOMMU driver missed the initial
+	 * add_device callback for dev, replay it to get things in order.
+	 */
+	if (!err && dev->bus && !device_iommu_mapped(dev))
+		err = iommu_probe_device(dev);
+
+	/* Ignore all other errors apart from EPROBE_DEFER */
+	if (err == -EPROBE_DEFER) {
+		return ERR_PTR(err);
+	} else if (err) {
+		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
+		return NULL;
+	}
+	return acpi_iommu_fwspec_ops(dev);
+}
+
 /**
  * acpi_dma_configure_id - Set-up DMA configuration for the device.
  * @dev: The pointer to the device
@@ -1539,7 +1592,7 @@  int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 
 	acpi_arch_dma_setup(dev, &dma_addr, &size);
 
-	iommu = iort_iommu_configure_id(dev, input_id);
+	iommu = acpi_iommu_configure_id(dev, input_id);
 	if (PTR_ERR(iommu) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;