[dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Stephen Hemminger]

The VFIO_PRESENT #define was a landmine and we hit it.
The DPDK has a config system and it should be used rather than
silently dropping a feature during build only to have it fail at
run time.

If VFIO is configured, and the kernel headers are not present
the build should fail. Rather than leaving developers puzzling
why the build system (with old kernel headers) produced non
functioning DPDK and their system (with new kernel headers) produced
correctly working DPDK.

As a matter of policy, really no code should be looking at
<linux/version.h> except for kernel drivers with compat files.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/linuxapp/eal/Makefile               |  4 ++--
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 10 +++++-----
 lib/librte_eal/linuxapp/eal/eal_pci.c              |  4 ++--
 lib/librte_eal/linuxapp/eal/eal_pci_init.h         |  2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c         |  3 ---
 lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c |  3 ---
 lib/librte_eal/linuxapp/eal/eal_vfio.h             |  1 -
 7 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 01f7b70..abaf0da 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -64,8 +64,8 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_thread.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_log.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_pci.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_pci_uio.c
-SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_pci_vfio.c
-SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_pci_vfio_mp_sync.c
+SRCS-$(CONFIG_RTE_EAL_VFIO) += eal_pci_vfio.c
+SRCS-$(CONFIG_RTE_EAL_VFIO) += eal_pci_vfio_mp_sync.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_debug.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_lcore.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_timer.c
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 66deda2..55f41d2 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -87,7 +87,7 @@ union intr_pipefds{
  */
 union rte_intr_read_buffer {
 	int uio_intr_count;              /* for uio device */
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 	uint64_t vfio_intr_count;        /* for vfio device */
 #endif
 	uint64_t timerfd_num;            /* for timerfd */
@@ -123,7 +123,7 @@ static struct rte_intr_source_list intr_sources;
 static pthread_t intr_thread;
 
 /* VFIO interrupts */
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 
 #define IRQ_SET_BUF_LEN  (sizeof(struct vfio_irq_set) + sizeof(int))
 
@@ -559,7 +559,7 @@ rte_intr_enable(struct rte_intr_handle *intr_handle)
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_ALARM:
 		return -1;
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 	case RTE_INTR_HANDLE_VFIO_MSIX:
 		if (vfio_enable_msix(intr_handle))
 			return -1;
@@ -599,7 +599,7 @@ rte_intr_disable(struct rte_intr_handle *intr_handle)
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_ALARM:
 		return -1;
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 	case RTE_INTR_HANDLE_VFIO_MSIX:
 		if (vfio_disable_msix(intr_handle))
 			return -1;
@@ -667,7 +667,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 		case RTE_INTR_HANDLE_ALARM:
 			bytes_read = sizeof(buf.timerfd_num);
 			break;
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 		case RTE_INTR_HANDLE_VFIO_MSIX:
 		case RTE_INTR_HANDLE_VFIO_MSI:
 		case RTE_INTR_HANDLE_VFIO_LEGACY:
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 9cb0ffd..5bc0254 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -571,7 +571,7 @@ pci_map_device(struct rte_pci_device *dev)
 	/* try mapping the NIC resources using VFIO if it exists */
 	switch (dev->kdrv) {
 	case RTE_KDRV_VFIO:
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 		if (pci_vfio_is_enabled())
 			ret = pci_vfio_map_resource(dev);
 #endif
@@ -771,7 +771,7 @@ rte_eal_pci_init(void)
 		RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
 		return -1;
 	}
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 	pci_vfio_enable();
 
 	if (pci_vfio_is_enabled()) {
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
index aa7b755..e285204 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
@@ -78,7 +78,7 @@ void pci_unmap_resource(void *requested_addr, size_t size);
 void pci_uio_unmap_resource(struct rte_pci_device *dev);
 #endif /* RTE_LIBRTE_EAL_HOTPLUG */
 
-#ifdef VFIO_PRESENT
+#ifdef RTE_EAL_VFIO
 
 #define VFIO_MAX_GROUPS 64
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index aea1fb1..6a0fd08 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -59,8 +59,6 @@
  * This file is only compiled if CONFIG_RTE_EAL_VFIO is set to "y".
  */
 
-#ifdef VFIO_PRESENT
-
 #define PAGE_SIZE   (sysconf(_SC_PAGESIZE))
 #define PAGE_MASK   (~(PAGE_SIZE - 1))
 
@@ -909,4 +907,3 @@ pci_vfio_is_enabled(void)
 {
 	return vfio_cfg.vfio_enabled;
 }
-#endif
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
index fec7080..657fc2c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
@@ -62,8 +62,6 @@
  * This file is only compiled if CONFIG_RTE_EAL_VFIO is set to "y".
  */
 
-#ifdef VFIO_PRESENT
-
 #define SOCKET_PATH_FMT "%s/.%s_mp_socket"
 #define CMSGLEN (CMSG_LEN(sizeof(int)))
 #define FD_TO_CMSGHDR(fd, chdr) \
@@ -391,4 +389,3 @@ pci_vfio_mp_sync_setup(void)
 	return 0;
 }
 
-#endif
diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
index 72ec3f6..0b4e362 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
@@ -52,7 +52,6 @@
 #define RTE_PCI_MSIX_FLAGS_QSIZE  PCI_MSIX_FLAGS_QSIZE
 #endif
 
-#define VFIO_PRESENT
 #endif /* kernel version */
 #endif /* RTE_EAL_VFIO */
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Burakov, Anatoly]

Hi Stephen,

> The VFIO_PRESENT #define was a landmine and we hit it.
> The DPDK has a config system and it should be used rather than silently
> dropping a feature during build only to have it fail at run time.
> 
> If VFIO is configured, and the kernel headers are not present the build
> should fail. Rather than leaving developers puzzling why the build system
> (with old kernel headers) produced non functioning DPDK and their system
> (with new kernel headers) produced correctly working DPDK.
> 
> As a matter of policy, really no code should be looking at <linux/version.h>
> except for kernel drivers with compat files.

In theory, I agree with you. In practice however, this change will unconditionally break builds on pre-VFIO kernels (<3.6). This may be OK now, but wasn't OK at the time it was developed because pre-VFIO kernels were still very much prevalent. AFAIK, VFIO is enabled by default, so maybe we should disable it in the default configs?

Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Stephen Hemminger]

On Tue, 21 Apr 2015 09:34:27 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> Hi Stephen,
> 
> > The VFIO_PRESENT #define was a landmine and we hit it.
> > The DPDK has a config system and it should be used rather than silently
> > dropping a feature during build only to have it fail at run time.
> > 
> > If VFIO is configured, and the kernel headers are not present the build
> > should fail. Rather than leaving developers puzzling why the build system
> > (with old kernel headers) produced non functioning DPDK and their system
> > (with new kernel headers) produced correctly working DPDK.
> > 
> > As a matter of policy, really no code should be looking at <linux/version.h>
> > except for kernel drivers with compat files.
> 
> In theory, I agree with you. In practice however, this change will unconditionally break builds on pre-VFIO kernels (<3.6).

If someone is building with an older kernel, then they should change the config.

> This may be OK now, but wasn't OK at the time it was developed because pre-VFIO kernels were still very much prevalent. AFAIK, VFIO is enabled by default, so maybe we should disable it in the default configs?

But failing at runtime is much worse.

Re: [dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Burakov, Anatoly]

Hi Stephen,

> > Hi Stephen,
> >
> > > The VFIO_PRESENT #define was a landmine and we hit it.
> > > The DPDK has a config system and it should be used rather than
> > > silently dropping a feature during build only to have it fail at run time.
> > >
> > > If VFIO is configured, and the kernel headers are not present the
> > > build should fail. Rather than leaving developers puzzling why the
> > > build system (with old kernel headers) produced non functioning DPDK
> > > and their system (with new kernel headers) produced correctly working
> DPDK.
> > >
> > > As a matter of policy, really no code should be looking at
> > > <linux/version.h> except for kernel drivers with compat files.
> >
> > In theory, I agree with you. In practice however, this change will
> unconditionally break builds on pre-VFIO kernels (<3.6).
> 
> If someone is building with an older kernel, then they should change the
> config.
> 

Well, it's not like this is immediately obvious to anyone building DPDK. With your patch, the "out-of-the-box" experience is no longer there, since one now has to figure out why it's not building, go edit config files, et al, which is easy for you and me, but may not be easy for someone dealing with DPDK for the first time.

I agree that both situations aren't ideal, it just seems to me that not building VFIO by default is better in that sense (EAL will show a warning message saying that VFIO isn't enabled, but the code will actually compile). Of course, the best of both worlds would be something like a configure script.

Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Stephen Hemminger]

On Wed, 22 Apr 2015 08:59:51 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> Hi Stephen,
> 
> > > Hi Stephen,
> > >
> > > > The VFIO_PRESENT #define was a landmine and we hit it.
> > > > The DPDK has a config system and it should be used rather than
> > > > silently dropping a feature during build only to have it fail at run time.
> > > >
> > > > If VFIO is configured, and the kernel headers are not present the
> > > > build should fail. Rather than leaving developers puzzling why the
> > > > build system (with old kernel headers) produced non functioning DPDK
> > > > and their system (with new kernel headers) produced correctly working
> > DPDK.
> > > >
> > > > As a matter of policy, really no code should be looking at
> > > > <linux/version.h> except for kernel drivers with compat files.
> > >
> > > In theory, I agree with you. In practice however, this change will
> > unconditionally break builds on pre-VFIO kernels (<3.6).
> > 
> > If someone is building with an older kernel, then they should change the
> > config.
> > 
> 
> Well, it's not like this is immediately obvious to anyone building DPDK. With your patch, the "out-of-the-box" experience is no longer there, since one now has to figure out why it's not building, go edit config files, et al, which is easy for you and me, but may not be easy for someone dealing with DPDK for the first time.
> 
> I agree that both situations aren't ideal, it just seems to me that not building VFIO by default is better in that sense (EAL will show a warning message saying that VFIO isn't enabled, but the code will actually compile). Of course, the best of both worlds would be something like a configure script.
> 
> Thanks,
> Anatoly

What we ended up doing is a second patch to have a "compat_vfio.h" file to allow
building on older systems. I will send that as a follow on.

The problem is compounded by the fact that VFIO or not has to be decided by startup
scripts prior to starting the DPDK application. The startup scripts have no way of knowing
that the DPDK application was built with broken VFIO support. That is why I think
the original method with VFIO_PRESENT was not the correct way to handle this.

Re: [dpdk-dev] [PATCH] vfio: don't silently drop VFIO support

[Burakov, Anatoly]

Hi Stephen,

> What we ended up doing is a second patch to have a "compat_vfio.h" file to
> allow building on older systems. I will send that as a follow on.

I would argue it should be part of this series, as without them the patch breaks compilation on older kernels.

> The problem is compounded by the fact that VFIO or not has to be decided
> by startup scripts prior to starting the DPDK application. The startup scripts
> have no way of knowing that the DPDK application was built with broken
> VFIO support. That is why I think the original method with VFIO_PRESENT
> was not the correct way to handle this.

That makes sense, so no more objections from me :-)

Thanks,
Anatoly