[PATCH v2 0/4] Simplify the module and kconfig structure in vfio

[PATCH v2 0/4] Simplify the module and kconfig structure in vfio

[Jason Gunthorpe]

This series does a little house cleaning to remove the SPAPR exported
symbols and presense in the public header file and reduce the number of
modules that comprise VFIO.

v2:
 - Add stubs for vfio_virqfd_init()/vfio_virqfd_exit() so that linking
   works even if vfio_pci/etc is not selected
v1: https://lore.kernel.org/r/0-v1-10a2dba77915+c23-vfio_modules_jgg@nvidia.com

Jason Gunthorpe (4):
  vfio/pci: Move all the SPAPR PCI specific logic to vfio_pci_core.ko
  vfio: Move vfio_spapr_iommu_eeh_ioctl into vfio_iommu_spapr_tce.c
  vfio: Remove CONFIG_VFIO_SPAPR_EEH
  vfio: Fold vfio_virqfd.ko into vfio.ko

 drivers/vfio/Kconfig                |   5 --
 drivers/vfio/Makefile               |   5 +-
 drivers/vfio/pci/vfio_pci_priv.h    |  21 ++++++
 drivers/vfio/vfio.h                 |  13 ++++
 drivers/vfio/vfio_iommu_spapr_tce.c |  75 +++++++++++++++++++
 drivers/vfio/vfio_main.c            |   7 ++
 drivers/vfio/vfio_spapr_eeh.c       | 107 ----------------------------
 drivers/vfio/virqfd.c               |  16 +----
 include/linux/vfio.h                |  23 ------
 9 files changed, 119 insertions(+), 153 deletions(-)
 delete mode 100644 drivers/vfio/vfio_spapr_eeh.c


base-commit: 70481365703c5a8654c2b653cd35b8ae8650b6f3
-- 
2.37.3

[PATCH v2 1/4] vfio/pci: Move all the SPAPR PCI specific logic to vfio_pci_core.ko

[Jason Gunthorpe]

The vfio_spapr_pci_eeh_open/release() functions are one line wrappers
around an arch function. Just make them static inline and move them into
vfio_pci_priv.h. This eliminates some weird exported symbols that don't
need to exist.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_priv.h | 21 +++++++++++++++++++++
 drivers/vfio/vfio_spapr_eeh.c    | 13 -------------
 include/linux/vfio.h             | 11 -----------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 5e4fa69aee16c1..24d93b2ac9f52b 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -101,4 +101,25 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
 	return (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
 }
 
+#if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
+#include <asm/eeh.h>
+static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
+{
+	eeh_dev_open(pdev);
+}
+
+static inline void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
+{
+	eeh_dev_release(pdev);
+}
+#else
+static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
+{
+}
+
+static inline void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
+{
+}
+#endif
+
 #endif
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index 67f55ac1d459cc..c9d102aafbcd11 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -15,19 +15,6 @@
 #define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
 #define DRIVER_DESC	"VFIO IOMMU SPAPR EEH"
 
-/* We might build address mapping here for "fast" path later */
-void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
-{
-	eeh_dev_open(pdev);
-}
-EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_open);
-
-void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
-{
-	eeh_dev_release(pdev);
-}
-EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_release);
-
 long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 				unsigned int cmd, unsigned long arg)
 {
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ee399a768070d0..b0557e46b777a2 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -230,21 +230,10 @@ int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
 				       int num_irqs, int max_irq_type,
 				       size_t *data_size);
 
-struct pci_dev;
 #if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
-void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
-void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group, unsigned int cmd,
 				unsigned long arg);
 #else
-static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
-{
-}
-
-static inline void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
-{
-}
-
 static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 					      unsigned int cmd,
 					      unsigned long arg)
-- 
2.37.3

[PATCH v2 2/4] vfio: Move vfio_spapr_iommu_eeh_ioctl into vfio_iommu_spapr_tce.c

[Jason Gunthorpe]

This helper function is only used to implement the ioctl for the SPAPR
container, put it logically where it belongs.

The PPC64 kconfig is a bit of a rats nest, but it turns out that if
CONFIG_SPAPR_TCE_IOMMU is on then EEH must be too:

config SPAPR_TCE_IOMMU
	bool "sPAPR TCE IOMMU Support"
	depends on PPC_POWERNV || PPC_PSERIES
	select IOMMU_API
	help
	  Enables bits of IOMMU API required by VFIO. The iommu_ops
	  is not implemented as it is not necessary for VFIO.

config PPC_POWERNV
	select FORCE_PCI

config PPC_PSERIES
	select FORCE_PCI

config EEH
	bool
	depends on (PPC_POWERNV || PPC_PSERIES) && PCI
	default y

So we don't need to worry about the fact that asm/eeh.h doesn't define
enough stuff to compile vfio_spapr_iommu_eeh_ioctl() in !CONFIG_EEH. If
someday someone changes the kconfig around they can also fix the ifdefs in
asm/eeh.h to compile this code too.

This eliminates an unnecessary module and SPAPR code in a global header.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/Makefile               |  1 -
 drivers/vfio/vfio_iommu_spapr_tce.c | 75 +++++++++++++++++++++++
 drivers/vfio/vfio_spapr_eeh.c       | 94 -----------------------------
 include/linux/vfio.h                | 12 ----
 4 files changed, 75 insertions(+), 107 deletions(-)
 delete mode 100644 drivers/vfio/vfio_spapr_eeh.c

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index b693a1169286f8..50b8e8e3fb10dd 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -10,7 +10,6 @@ vfio-y += vfio_main.o \
 obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
-obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
 obj-$(CONFIG_VFIO_PCI) += pci/
 obj-$(CONFIG_VFIO_PLATFORM) += platform/
 obj-$(CONFIG_VFIO_MDEV) += mdev/
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 169f07ac162d9c..47a8b138cf7f6d 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -773,6 +773,81 @@ static long tce_iommu_create_default_window(struct tce_container *container)
 	return ret;
 }
 
+static long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
+				unsigned int cmd, unsigned long arg)
+{
+	struct eeh_pe *pe;
+	struct vfio_eeh_pe_op op;
+	unsigned long minsz;
+	long ret = -EINVAL;
+
+	if (!IS_ENABLED(CONFIG_EEH))
+		return -ENOTTY;
+
+	switch (cmd) {
+	case VFIO_CHECK_EXTENSION:
+		if (arg == VFIO_EEH)
+			ret = eeh_enabled() ? 1 : 0;
+		else
+			ret = 0;
+		break;
+	case VFIO_EEH_PE_OP:
+		pe = eeh_iommu_group_to_pe(group);
+		if (!pe)
+			return -ENODEV;
+
+		minsz = offsetofend(struct vfio_eeh_pe_op, op);
+		if (copy_from_user(&op, (void __user *)arg, minsz))
+			return -EFAULT;
+		if (op.argsz < minsz || op.flags)
+			return -EINVAL;
+
+		switch (op.op) {
+		case VFIO_EEH_PE_DISABLE:
+			ret = eeh_pe_set_option(pe, EEH_OPT_DISABLE);
+			break;
+		case VFIO_EEH_PE_ENABLE:
+			ret = eeh_pe_set_option(pe, EEH_OPT_ENABLE);
+			break;
+		case VFIO_EEH_PE_UNFREEZE_IO:
+			ret = eeh_pe_set_option(pe, EEH_OPT_THAW_MMIO);
+			break;
+		case VFIO_EEH_PE_UNFREEZE_DMA:
+			ret = eeh_pe_set_option(pe, EEH_OPT_THAW_DMA);
+			break;
+		case VFIO_EEH_PE_GET_STATE:
+			ret = eeh_pe_get_state(pe);
+			break;
+		case VFIO_EEH_PE_RESET_DEACTIVATE:
+			ret = eeh_pe_reset(pe, EEH_RESET_DEACTIVATE, true);
+			break;
+		case VFIO_EEH_PE_RESET_HOT:
+			ret = eeh_pe_reset(pe, EEH_RESET_HOT, true);
+			break;
+		case VFIO_EEH_PE_RESET_FUNDAMENTAL:
+			ret = eeh_pe_reset(pe, EEH_RESET_FUNDAMENTAL, true);
+			break;
+		case VFIO_EEH_PE_CONFIGURE:
+			ret = eeh_pe_configure(pe);
+			break;
+		case VFIO_EEH_PE_INJECT_ERR:
+			minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
+			if (op.argsz < minsz)
+				return -EINVAL;
+			if (copy_from_user(&op, (void __user *)arg, minsz))
+				return -EFAULT;
+
+			ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
+						op.err.addr, op.err.mask);
+			break;
+		default:
+			ret = -EINVAL;
+		}
+	}
+
+	return ret;
+}
+
 static long tce_iommu_ioctl(void *iommu_data,
 				 unsigned int cmd, unsigned long arg)
 {
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
deleted file mode 100644
index c9d102aafbcd11..00000000000000
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ /dev/null
@@ -1,94 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * EEH functionality support for VFIO devices. The feature is only
- * available on sPAPR compatible platforms.
- *
- * Copyright Gavin Shan, IBM Corporation 2014.
- */
-
-#include <linux/module.h>
-#include <linux/uaccess.h>
-#include <linux/vfio.h>
-#include <asm/eeh.h>
-
-#define DRIVER_VERSION	"0.1"
-#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
-#define DRIVER_DESC	"VFIO IOMMU SPAPR EEH"
-
-long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
-				unsigned int cmd, unsigned long arg)
-{
-	struct eeh_pe *pe;
-	struct vfio_eeh_pe_op op;
-	unsigned long minsz;
-	long ret = -EINVAL;
-
-	switch (cmd) {
-	case VFIO_CHECK_EXTENSION:
-		if (arg == VFIO_EEH)
-			ret = eeh_enabled() ? 1 : 0;
-		else
-			ret = 0;
-		break;
-	case VFIO_EEH_PE_OP:
-		pe = eeh_iommu_group_to_pe(group);
-		if (!pe)
-			return -ENODEV;
-
-		minsz = offsetofend(struct vfio_eeh_pe_op, op);
-		if (copy_from_user(&op, (void __user *)arg, minsz))
-			return -EFAULT;
-		if (op.argsz < minsz || op.flags)
-			return -EINVAL;
-
-		switch (op.op) {
-		case VFIO_EEH_PE_DISABLE:
-			ret = eeh_pe_set_option(pe, EEH_OPT_DISABLE);
-			break;
-		case VFIO_EEH_PE_ENABLE:
-			ret = eeh_pe_set_option(pe, EEH_OPT_ENABLE);
-			break;
-		case VFIO_EEH_PE_UNFREEZE_IO:
-			ret = eeh_pe_set_option(pe, EEH_OPT_THAW_MMIO);
-			break;
-		case VFIO_EEH_PE_UNFREEZE_DMA:
-			ret = eeh_pe_set_option(pe, EEH_OPT_THAW_DMA);
-			break;
-		case VFIO_EEH_PE_GET_STATE:
-			ret = eeh_pe_get_state(pe);
-			break;
-		case VFIO_EEH_PE_RESET_DEACTIVATE:
-			ret = eeh_pe_reset(pe, EEH_RESET_DEACTIVATE, true);
-			break;
-		case VFIO_EEH_PE_RESET_HOT:
-			ret = eeh_pe_reset(pe, EEH_RESET_HOT, true);
-			break;
-		case VFIO_EEH_PE_RESET_FUNDAMENTAL:
-			ret = eeh_pe_reset(pe, EEH_RESET_FUNDAMENTAL, true);
-			break;
-		case VFIO_EEH_PE_CONFIGURE:
-			ret = eeh_pe_configure(pe);
-			break;
-		case VFIO_EEH_PE_INJECT_ERR:
-			minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
-			if (op.argsz < minsz)
-				return -EINVAL;
-			if (copy_from_user(&op, (void __user *)arg, minsz))
-				return -EFAULT;
-
-			ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
-						op.err.addr, op.err.mask);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(vfio_spapr_iommu_eeh_ioctl);
-
-MODULE_VERSION(DRIVER_VERSION);
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR(DRIVER_AUTHOR);
-MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index b0557e46b777a2..73bcb92179a224 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -230,18 +230,6 @@ int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
 				       int num_irqs, int max_irq_type,
 				       size_t *data_size);
 
-#if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
-long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group, unsigned int cmd,
-				unsigned long arg);
-#else
-static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
-					      unsigned int cmd,
-					      unsigned long arg)
-{
-	return -ENOTTY;
-}
-#endif /* CONFIG_VFIO_SPAPR_EEH */
-
 /*
  * IRQfd - generic
  */
-- 
2.37.3

[PATCH v2 3/4] vfio: Remove CONFIG_VFIO_SPAPR_EEH

[Jason Gunthorpe]

We don't need a kconfig symbol for this, just directly test CONFIG_EEH in
the one remaining place that needs it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/Kconfig             | 5 -----
 drivers/vfio/pci/vfio_pci_priv.h | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 86c381ceb9a1e9..d25b91adfd64cd 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -20,11 +20,6 @@ config VFIO_IOMMU_SPAPR_TCE
 	depends on SPAPR_TCE_IOMMU
 	default VFIO
 
-config VFIO_SPAPR_EEH
-	tristate
-	depends on EEH && VFIO_IOMMU_SPAPR_TCE
-	default VFIO
-
 config VFIO_VIRQFD
 	tristate
 	select EVENTFD
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 24d93b2ac9f52b..d0fdc0b824c743 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -101,7 +101,7 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
 	return (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
 }
 
-#if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
+#if IS_ENABLED(CONFIG_EEH) && IS_ENABLED(CONFIG_VFIO_IOMMU_SPAPR_TCE)
 #include <asm/eeh.h>
 static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
 {
-- 
2.37.3

[PATCH v2 4/4] vfio: Fold vfio_virqfd.ko into vfio.ko

[Jason Gunthorpe]

This is only 1.8k, putting it in its own module is going to waste more
space rounding up to a PAGE_SIZE than it is worth. Put it in the main
vfio.ko module now that kbuild can support multiple .c files.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/Makefile    |  4 +---
 drivers/vfio/vfio.h      | 13 +++++++++++++
 drivers/vfio/vfio_main.c |  7 +++++++
 drivers/vfio/virqfd.c    | 16 ++--------------
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 50b8e8e3fb10dd..0721ed4831c92f 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -1,13 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
-vfio_virqfd-y := virqfd.o
-
 obj-$(CONFIG_VFIO) += vfio.o
 
 vfio-y += vfio_main.o \
 	  iova_bitmap.o \
 	  container.o
+vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
 
-obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
 obj-$(CONFIG_VFIO_PCI) += pci/
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 4a1bac1359a952..4d2de02f2ced6e 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -125,6 +125,19 @@ long vfio_container_ioctl_check_extension(struct vfio_container *container,
 int __init vfio_container_init(void);
 void vfio_container_cleanup(void);
 
+#if IS_ENABLED(CONFIG_VFIO_VIRQFD)
+int __init vfio_virqfd_init(void);
+void vfio_virqfd_exit(void);
+#else
+static inline int __init vfio_virqfd_init(void)
+{
+	return 0;
+}
+static inline void vfio_virqfd_exit(void)
+{
+}
+#endif
+
 #ifdef CONFIG_VFIO_NOIOMMU
 extern bool vfio_noiommu __read_mostly;
 #else
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 9207e6c0e3cb26..9b1e5fd5f7b73c 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -1777,6 +1777,10 @@ static int __init vfio_init(void)
 	if (ret)
 		return ret;
 
+	ret = vfio_virqfd_init();
+	if (ret)
+		goto err_virqfd;
+
 	/* /dev/vfio/$GROUP */
 	vfio.class = class_create(THIS_MODULE, "vfio");
 	if (IS_ERR(vfio.class)) {
@@ -1807,6 +1811,8 @@ static int __init vfio_init(void)
 	class_destroy(vfio.class);
 	vfio.class = NULL;
 err_group_class:
+	vfio_virqfd_exit();
+err_virqfd:
 	vfio_container_cleanup();
 	return ret;
 }
@@ -1821,6 +1827,7 @@ static void __exit vfio_cleanup(void)
 	class_destroy(vfio.device_class);
 	vfio.device_class = NULL;
 	class_destroy(vfio.class);
+	vfio_virqfd_exit();
 	vfio_container_cleanup();
 	vfio.class = NULL;
 	xa_destroy(&vfio_device_set_xa);
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 414e98d82b02e5..0ff3c1519df0bd 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -13,14 +13,10 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 
-#define DRIVER_VERSION  "0.1"
-#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
-#define DRIVER_DESC     "IRQFD support for VFIO bus drivers"
-
 static struct workqueue_struct *vfio_irqfd_cleanup_wq;
 static DEFINE_SPINLOCK(virqfd_lock);
 
-static int __init vfio_virqfd_init(void)
+int __init vfio_virqfd_init(void)
 {
 	vfio_irqfd_cleanup_wq =
 		create_singlethread_workqueue("vfio-irqfd-cleanup");
@@ -30,7 +26,7 @@ static int __init vfio_virqfd_init(void)
 	return 0;
 }
 
-static void __exit vfio_virqfd_exit(void)
+void vfio_virqfd_exit(void)
 {
 	destroy_workqueue(vfio_irqfd_cleanup_wq);
 }
@@ -216,11 +212,3 @@ void vfio_virqfd_disable(struct virqfd **pvirqfd)
 	flush_workqueue(vfio_irqfd_cleanup_wq);
 }
 EXPORT_SYMBOL_GPL(vfio_virqfd_disable);
-
-module_init(vfio_virqfd_init);
-module_exit(vfio_virqfd_exit);
-
-MODULE_VERSION(DRIVER_VERSION);
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR(DRIVER_AUTHOR);
-MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.37.3

Re: [PATCH v2 4/4] vfio: Fold vfio_virqfd.ko into vfio.ko

[Cornelia Huck]

On Mon, Oct 03 2022, Jason Gunthorpe <jgg@nvidia.com> wrote:

> This is only 1.8k, putting it in its own module is going to waste more
> space rounding up to a PAGE_SIZE than it is worth. Put it in the main
> vfio.ko module now that kbuild can support multiple .c files.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/Makefile    |  4 +---
>  drivers/vfio/vfio.h      | 13 +++++++++++++
>  drivers/vfio/vfio_main.c |  7 +++++++
>  drivers/vfio/virqfd.c    | 16 ++--------------
>  4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 50b8e8e3fb10dd..0721ed4831c92f 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -1,13 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0
> -vfio_virqfd-y := virqfd.o
> -
>  obj-$(CONFIG_VFIO) += vfio.o
>  
>  vfio-y += vfio_main.o \
>  	  iova_bitmap.o \
>  	  container.o
> +vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o

I think you need to make VFIO_VIRQFD bool instead of tristate now?

>  
> -obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
>  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
>  obj-$(CONFIG_VFIO_PCI) += pci/

(...)

> diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
> index 414e98d82b02e5..0ff3c1519df0bd 100644
> --- a/drivers/vfio/virqfd.c
> +++ b/drivers/vfio/virqfd.c
> @@ -13,14 +13,10 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>

And this needs an #include "vfio.h", I think?

>  
> -#define DRIVER_VERSION  "0.1"
> -#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> -#define DRIVER_DESC     "IRQFD support for VFIO bus drivers"
> -
>  static struct workqueue_struct *vfio_irqfd_cleanup_wq;
>  static DEFINE_SPINLOCK(virqfd_lock);
>  
> -static int __init vfio_virqfd_init(void)
> +int __init vfio_virqfd_init(void)
>  {
>  	vfio_irqfd_cleanup_wq =
>  		create_singlethread_workqueue("vfio-irqfd-cleanup");
> @@ -30,7 +26,7 @@ static int __init vfio_virqfd_init(void)
>  	return 0;
>  }
>  
> -static void __exit vfio_virqfd_exit(void)
> +void vfio_virqfd_exit(void)
>  {
>  	destroy_workqueue(vfio_irqfd_cleanup_wq);
>  }
> @@ -216,11 +212,3 @@ void vfio_virqfd_disable(struct virqfd **pvirqfd)
>  	flush_workqueue(vfio_irqfd_cleanup_wq);
>  }
>  EXPORT_SYMBOL_GPL(vfio_virqfd_disable);
> -
> -module_init(vfio_virqfd_init);
> -module_exit(vfio_virqfd_exit);
> -
> -MODULE_VERSION(DRIVER_VERSION);
> -MODULE_LICENSE("GPL v2");
> -MODULE_AUTHOR(DRIVER_AUTHOR);
> -MODULE_DESCRIPTION(DRIVER_DESC);

Re: [PATCH v2 4/4] vfio: Fold vfio_virqfd.ko into vfio.ko

[Jason Gunthorpe]

On Wed, Oct 05, 2022 at 06:37:14PM +0200, Cornelia Huck wrote:
> On Mon, Oct 03 2022, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > This is only 1.8k, putting it in its own module is going to waste more
> > space rounding up to a PAGE_SIZE than it is worth. Put it in the main
> > vfio.ko module now that kbuild can support multiple .c files.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/vfio/Makefile    |  4 +---
> >  drivers/vfio/vfio.h      | 13 +++++++++++++
> >  drivers/vfio/vfio_main.c |  7 +++++++
> >  drivers/vfio/virqfd.c    | 16 ++--------------
> >  4 files changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > index 50b8e8e3fb10dd..0721ed4831c92f 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -1,13 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -vfio_virqfd-y := virqfd.o
> > -
> >  obj-$(CONFIG_VFIO) += vfio.o
> >  
> >  vfio-y += vfio_main.o \
> >  	  iova_bitmap.o \
> >  	  container.o
> > +vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
> 
> I think you need to make VFIO_VIRQFD bool instead of tristate now?
> 
> >  
> > -obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
> >  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> >  obj-$(CONFIG_VFIO_PCI) += pci/
> 
> (...)
> 
> > diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
> > index 414e98d82b02e5..0ff3c1519df0bd 100644
> > --- a/drivers/vfio/virqfd.c
> > +++ b/drivers/vfio/virqfd.c
> > @@ -13,14 +13,10 @@
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> 
> And this needs an #include "vfio.h", I think?

Yes, thanks, I picked those up from zero day and will resend on rc1

Jason

Re: [PATCH v2 1/4] vfio/pci: Move all the SPAPR PCI specific logic to vfio_pci_core.ko

[Christoph Hellwig]

On Mon, Oct 03, 2022 at 12:39:30PM -0300, Jason Gunthorpe wrote:
> The vfio_spapr_pci_eeh_open/release() functions are one line wrappers
> around an arch function. Just make them static inline and move them into
> vfio_pci_priv.h.

Please just kill them entirely - the vfio spapr code depends on EEH
anyway.  In fact I have an old patch to do that floating around
somewhere, but it's probably less work to just recreate it.

Re: [PATCH v2 2/4] vfio: Move vfio_spapr_iommu_eeh_ioctl into vfio_iommu_spapr_tce.c

[Christoph Hellwig]

On Mon, Oct 03, 2022 at 12:39:31PM -0300, Jason Gunthorpe wrote:
> So we don't need to worry about the fact that asm/eeh.h doesn't define
> enough stuff to compile vfio_spapr_iommu_eeh_ioctl() in !CONFIG_EEH. If
> someday someone changes the kconfig around they can also fix the ifdefs in
> asm/eeh.h to compile this code too.

Please just break it up by opencoding the VFIO_CHECK_EXTENSION
cases and just having a helper for VFIO_EEH_PE_OP.  I could look
for my patch doing that but again it should be trivial to redo.

Re: [PATCH v2 4/4] vfio: Fold vfio_virqfd.ko into vfio.ko

[Christoph Hellwig]

On Mon, Oct 03, 2022 at 12:39:33PM -0300, Jason Gunthorpe wrote:
> This is only 1.8k, putting it in its own module is going to waste more
> space rounding up to a PAGE_SIZE than it is worth. Put it in the main
> vfio.ko module now that kbuild can support multiple .c files.

Assuming you actually need it (only vfio_platform and vfio_pci actually
need it) and you don't otherwise need EVENTFD support.  While I guess
the configfs that do not fit the above aren't the most common they
are real and are a real tradeoff.

Re: [PATCH v2 1/4] vfio/pci: Move all the SPAPR PCI specific logic to vfio_pci_core.ko

[Jason Gunthorpe]

On Mon, Oct 10, 2022 at 12:07:15AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 03, 2022 at 12:39:30PM -0300, Jason Gunthorpe wrote:
> > The vfio_spapr_pci_eeh_open/release() functions are one line wrappers
> > around an arch function. Just make them static inline and move them into
> > vfio_pci_priv.h.
> 
> Please just kill them entirely - the vfio spapr code depends on EEH
> anyway.  In fact I have an old patch to do that floating around
> somewhere, but it's probably less work to just recreate it.

How do you mean? You want to put the #ifdef in the vfio_pci_core.c at
the only call site?

Jason

Re: [PATCH v2 2/4] vfio: Move vfio_spapr_iommu_eeh_ioctl into vfio_iommu_spapr_tce.c

[Jason Gunthorpe]

On Mon, Oct 10, 2022 at 12:09:27AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 03, 2022 at 12:39:31PM -0300, Jason Gunthorpe wrote:
> > So we don't need to worry about the fact that asm/eeh.h doesn't define
> > enough stuff to compile vfio_spapr_iommu_eeh_ioctl() in !CONFIG_EEH. If
> > someday someone changes the kconfig around they can also fix the ifdefs in
> > asm/eeh.h to compile this code too.
> 
> Please just break it up by opencoding the VFIO_CHECK_EXTENSION
> cases and just having a helper for VFIO_EEH_PE_OP.  I could look
> for my patch doing that but again it should be trivial to redo.

You mean to fold the case branches into the existing switch statements
in tce_iommu_ioctl()? Like below with indenting fixed?

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 47a8b138cf7f6d..b6426109db5e0f 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -773,8 +773,8 @@ static long tce_iommu_create_default_window(struct tce_container *container)
 	return ret;
 }
 
-static long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
-				unsigned int cmd, unsigned long arg)
+static long vfio_spapr_ioctl_eeh_pe_op(struct iommu_group *group,
+				       unsigned long arg)
 {
 	struct eeh_pe *pe;
 	struct vfio_eeh_pe_op op;
@@ -784,14 +784,6 @@ static long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 	if (!IS_ENABLED(CONFIG_EEH))
 		return -ENOTTY;
 
-	switch (cmd) {
-	case VFIO_CHECK_EXTENSION:
-		if (arg == VFIO_EEH)
-			ret = eeh_enabled() ? 1 : 0;
-		else
-			ret = 0;
-		break;
-	case VFIO_EEH_PE_OP:
 	pe = eeh_iommu_group_to_pe(group);
 	if (!pe)
 		return -ENODEV;
@@ -843,8 +835,6 @@ static long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 	default:
 		ret = -EINVAL;
 	}
-	}
-
 	return ret;
 }
 
@@ -860,14 +850,14 @@ static long tce_iommu_ioctl(void *iommu_data,
 		switch (arg) {
 		case VFIO_SPAPR_TCE_IOMMU:
 		case VFIO_SPAPR_TCE_v2_IOMMU:
-			ret = 1;
-			break;
+			return 1;
+		case VFIO_EEH:
+			if (IS_ENABLED(CONFIG_EEH) && eeh_enabled)
+				return 1;
+			return 0;
 		default:
-			ret = vfio_spapr_iommu_eeh_ioctl(NULL, cmd, arg);
-			break;
+			return 0;
 		}
-
-		return (ret < 0) ? 0 : ret;
 	}
 
 	/*
@@ -1121,8 +1111,7 @@ static long tce_iommu_ioctl(void *iommu_data,
 
 		ret = 0;
 		list_for_each_entry(tcegrp, &container->group_list, next) {
-			ret = vfio_spapr_iommu_eeh_ioctl(tcegrp->grp,
-					cmd, arg);
+			ret = vfio_spapr_ioctl_eeh_pe_op(tcegrp->grp, arg);
 			if (ret)
 				return ret;
 		}

Re: [PATCH v2 1/4] vfio/pci: Move all the SPAPR PCI specific logic to vfio_pci_core.ko

[Christoph Hellwig]

On Mon, Oct 10, 2022 at 03:32:30PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 10, 2022 at 12:07:15AM -0700, Christoph Hellwig wrote:
> > On Mon, Oct 03, 2022 at 12:39:30PM -0300, Jason Gunthorpe wrote:
> > > The vfio_spapr_pci_eeh_open/release() functions are one line wrappers
> > > around an arch function. Just make them static inline and move them into
> > > vfio_pci_priv.h.
> > 
> > Please just kill them entirely - the vfio spapr code depends on EEH
> > anyway.  In fact I have an old patch to do that floating around
> > somewhere, but it's probably less work to just recreate it.
> 
> How do you mean? You want to put the #ifdef in the vfio_pci_core.c at
> the only call site?

Yes.

Re: [PATCH v2 2/4] vfio: Move vfio_spapr_iommu_eeh_ioctl into vfio_iommu_spapr_tce.c

[Christoph Hellwig]

On Mon, Oct 10, 2022 at 03:42:03PM -0300, Jason Gunthorpe wrote:
> You mean to fold the case branches into the existing switch statements
> in tce_iommu_ioctl()? Like below with indenting fixed?

Yes.

Re: [PATCH v2 4/4] vfio: Fold vfio_virqfd.ko into vfio.ko

[Jason Gunthorpe]

On Mon, Oct 10, 2022 at 12:13:01AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 03, 2022 at 12:39:33PM -0300, Jason Gunthorpe wrote:
> > This is only 1.8k, putting it in its own module is going to waste more
> > space rounding up to a PAGE_SIZE than it is worth. Put it in the main
> > vfio.ko module now that kbuild can support multiple .c files.
> 
> Assuming you actually need it (only vfio_platform and vfio_pci actually
> need it) and you don't otherwise need EVENTFD support.  While I guess
> the configfs that do not fit the above aren't the most common they
> are real and are a real tradeoff.

Well, the config still exists, if someone is building a stripped down
kernel they can disable it and save the space. By the time you fully
load VFIO this is just noise. I don't have a specific preference here,
but I would like to reduce the number of modules just for sanity's
sake.

Jason