[PATCH] vfio: Split virqfd into a separate module for vfio bus drivers

[PATCH] vfio: Split virqfd into a separate module for vfio bus drivers

[Alex Williamson]

An unintended consequence of commit 42ac9bd18d4f ("vfio: initialize
the virqfd workqueue in VFIO generic code") is that the vfio module
is renamed to vfio_core so that it can include both vfio and virqfd.
That's a user visible change that may break module loading scritps
and it imposes eventfd support as a dependency on the core vfio code,
which it's really not.  virqfd is intended to be provided as a service
to vfio bus drivers, so instead of wrapping it into vfio.ko, we can
make it a stand-alone module toggled by vfio bus drivers.  This has
the additional benefit of removing initialization and exit from the
core vfio code.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Posted in reply to "[PATCH v14 19/20] vfio: initialize the virqfd
workqueue in VFIO generic code", reposting as an official proposal.
I removed the Kconfig select from AMBA support.  AMBA depends on
PLATFORM, which does the select and therefore seems sufficient.
Commit log and vfio_virqfd driver description also reworded.

 drivers/vfio/Kconfig          |    5 +++++
 drivers/vfio/Makefile         |    5 +++--
 drivers/vfio/pci/Kconfig      |    1 +
 drivers/vfio/platform/Kconfig |    1 +
 drivers/vfio/vfio.c           |    8 --------
 drivers/vfio/virqfd.c         |   17 +++++++++++++++--
 include/linux/vfio.h          |    2 --
 7 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index d5322a4..7d092dd 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -13,6 +13,11 @@ config VFIO_SPAPR_EEH
 	depends on EEH && VFIO_IOMMU_SPAPR_TCE
 	default n
 
+config VFIO_VIRQFD
+	tristate
+	depends on VFIO && EVENTFD
+	default n
+
 menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
 	depends on IOMMU_API
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index d798b09..7b8a31f 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -1,6 +1,7 @@
-vfio_core-y := vfio.o virqfd.o
+vfio_virqfd-y := virqfd.o
 
-obj-$(CONFIG_VFIO) += vfio_core.o
+obj-$(CONFIG_VFIO) += vfio.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
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index c6bb5da..579d83b 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,6 +1,7 @@
 config VFIO_PCI
 	tristate "VFIO support for PCI devices"
 	depends on VFIO && PCI && EVENTFD
+	select VFIO_VIRQFD
 	help
 	  Support for the PCI VFIO bus driver.  This is required to make
 	  use of PCI drivers using the VFIO framework.
diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index c0a3bff..9a4403e 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -1,6 +1,7 @@
 config VFIO_PLATFORM
 	tristate "VFIO support for platform devices"
 	depends on VFIO && EVENTFD && ARM
+	select VFIO_VIRQFD
 	help
 	  Support for platform devices with VFIO. This is required to make
 	  use of platform devices present on the system using the VFIO
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 86aac7e..0d33662 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1552,11 +1552,6 @@ static int __init vfio_init(void)
 	if (ret)
 		goto err_cdev_add;
 
-	/* Start the virqfd cleanup handler used by some VFIO bus drivers */
-	ret = vfio_virqfd_init();
-	if (ret)
-		goto err_virqfd;
-
 	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
 
 	/*
@@ -1569,8 +1564,6 @@ static int __init vfio_init(void)
 
 	return 0;
 
-err_virqfd:
-	cdev_del(&vfio.group_cdev);
 err_cdev_add:
 	unregister_chrdev_region(vfio.group_devt, MINORMASK);
 err_alloc_chrdev:
@@ -1585,7 +1578,6 @@ static void __exit vfio_cleanup(void)
 {
 	WARN_ON(!list_empty(&vfio.group_list));
 
-	vfio_virqfd_exit();
 	idr_destroy(&vfio.group_idr);
 	cdev_del(&vfio.group_cdev);
 	unregister_chrdev_region(vfio.group_devt, MINORMASK);
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 3d19aaf..27c89cd 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -13,12 +13,17 @@
 #include <linux/vfio.h>
 #include <linux/eventfd.h>
 #include <linux/file.h>
+#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);
 
-int __init vfio_virqfd_init(void)
+static int __init vfio_virqfd_init(void)
 {
 	vfio_irqfd_cleanup_wq =
 		create_singlethread_workqueue("vfio-irqfd-cleanup");
@@ -28,7 +33,7 @@ int __init vfio_virqfd_init(void)
 	return 0;
 }
 
-void vfio_virqfd_exit(void)
+static void __exit vfio_virqfd_exit(void)
 {
 	destroy_workqueue(vfio_irqfd_cleanup_wq);
 }
@@ -211,3 +216,11 @@ 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);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 683b514..cbed15f 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -142,8 +142,6 @@ struct virqfd {
 	struct virqfd		**pvirqfd;
 };
 
-extern int vfio_virqfd_init(void);
-extern void vfio_virqfd_exit(void);
 extern int vfio_virqfd_enable(void *opaque,
 			      int (*handler)(void *, void *),
 			      void (*thread)(void *, void *),

Re: [PATCH] vfio: Split virqfd into a separate module for vfio bus drivers

[Paul Bolle]

On Wed, 2015-03-18 at 11:27 -0600, Alex Williamson wrote:
> --- a/drivers/vfio/virqfd.c
> +++ b/drivers/vfio/virqfd.c

> +#define DRIVER_VERSION  "0.1"
> +#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> +#define DRIVER_DESC     "IRQFD support for VFIO bus drivers"

> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);

Why bother with those three defines? They're all used just once, aren't
they?


Paul Bolle

Re: [PATCH] vfio: Split virqfd into a separate module for vfio bus drivers

[Alex Williamson]

On Thu, 2015-03-19 at 20:32 +0100, Paul Bolle wrote:
> On Wed, 2015-03-18 at 11:27 -0600, Alex Williamson wrote:
> > --- a/drivers/vfio/virqfd.c
> > +++ b/drivers/vfio/virqfd.c
> 
> > +#define DRIVER_VERSION  "0.1"
> > +#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> > +#define DRIVER_DESC     "IRQFD support for VFIO bus drivers"
> 
> > +MODULE_VERSION(DRIVER_VERSION);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR(DRIVER_AUTHOR);
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> 
> Why bother with those three defines? They're all used just once, aren't
> they?

Well, at some point when I was doing vfio it seemed like a good idea and
I copied it from another driver.  The core vfio module printed the
description and version on driver loading, but the other modules that
comprise vfio just followed along for consistency.  Now we even have a
few more modules that comprise vfio, and they all follow this same
standard, even though they don't use the defines elsewhere.  The macro
names are pretty standard, so I don't think I'm infringing on any name
space by using them, it seems harmless otherwise and maintains
consistency within the driver.  Is it more valuable to remove a few
lines of source code with no net effect on the resulting output?
Besides, look at how much more aesthetically pleasing the above is
versus this:

MODULE_VERSION("0.1");
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Alex Williamson <alex.williamson@redhat.com>");
MODULE_DESCRIPTION("IRQFD support for VFIO bus drivers");

;)

Thanks,

Alex

Re: [PATCH] vfio: Split virqfd into a separate module for vfio bus drivers

[Paul Bolle]

On Thu, 2015-03-19 at 14:12 -0600, Alex Williamson wrote:
> Well, at some point when I was doing vfio it seemed like a good idea and
> I copied it from another driver.

For some time now I have this idea that there's a Linux kernel module
template somewhere that uses defines like the ones you're using. That
all started when I noticed drivers defining DRIVER_LICENSE. (I really
like the name of that define.) Because every driver defining it ends up
using it only once.

> Is it more valuable to remove a few lines of source code with no net
> effect on the resulting output?

We should discuss this from the opposite direction: why is this patch
adding a few lines with no obvious benefit?

> Besides, look at how much more aesthetically pleasing the above is
> versus this:
> 
> MODULE_VERSION("0.1");
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Alex Williamson <alex.williamson@redhat.com>");
> MODULE_DESCRIPTION("IRQFD support for VFIO bus drivers");
> 
> ;)

I don't do smileys. Perhaps that's why I never know what to think when
someone uses them. Anyhow, sure, my comment is extremely trivial, but I
do think I should raise this point just once. Because, well, ...
because!


Paul Bolle