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

2015-03-19 Thread 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.william...@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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-19 Thread 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.william...@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.william...@redhat.com);
MODULE_DESCRIPTION(IRQFD support for VFIO bus drivers);

;)

Thanks,

Alex

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-19 Thread 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.william...@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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-03-18 Thread 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.william...@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.william...@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