From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH] bus/pci: fix vfio mode Date: Mon, 30 Oct 2017 10:17:36 +0100 Message-ID: <20171030091736.GH10890@bidouze.vm.6wind.com> References: <20171028062053.6615-1-jerin.jacob@caviumnetworks.com> <20171030080654.GF10890@bidouze.vm.6wind.com> <6e9922c5-9659-a42a-4213-700ebe1e5714@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Jerin Jacob , dev@dpdk.org, thomas@monjalon.net To: Ferruh Yigit Return-path: Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) by dpdk.org (Postfix) with ESMTP id BBA731B2C7 for ; Mon, 30 Oct 2017 10:17:49 +0100 (CET) Received: by mail-wm0-f65.google.com with SMTP id t139so14893504wmt.1 for ; Mon, 30 Oct 2017 02:17:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: <6e9922c5-9659-a42a-4213-700ebe1e5714@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Ferruh, On Mon, Oct 30, 2017 at 02:00:43AM -0700, Ferruh Yigit wrote: > On 10/30/2017 1:06 AM, Gaëtan Rivet wrote: > > Hi Jerin, > > > > On Sat, Oct 28, 2017 at 11:50:52AM +0530, Jerin Jacob wrote: > >> The definition of VFIO_PRESENT is "eal_vfio.h", Fail to > >> include eal_vfio.h will result in disabling vfio. > >> > >> Fixes: 279b581c897d ("vfio: expose functions") > >> > > > > Thanks for the fix, sorry for VFIO. > > I tried to let go of VFIO_PRESENT in the PCI patchset, unfortunately I did > > not do a good-enough job. > > > > Instead of reinstating the dependency on the private eal_vfio.h header, > > I'd suggest replacing all VFIO_PRESENT references within the PCI bus by > > RTE_EAL_VFIO, and make the pci_vfio.c compilation depend on it within > > the linux Makefile. Something like: > > > > ---8<--- > > > > grep -rl VFIO_PRESENT drivers/bus/pci/linux/ |while read -r file > > do sed -i 's;VFIO_PRESENT;RTE_EAL_VFIO;' $file > > done > > VFIO_PRESENT is the combination of the if user enabled VFIO and if Linux kernel > supports it. > > Why not add same check and VFIO_PRESENT definition to rte_vfio.h: > > --- a/lib/librte_eal/common/include/rte_vfio.h > +++ b/lib/librte_eal/common/include/rte_vfio.h > @@ -34,7 +34,13 @@ > #ifndef _RTE_VFIO_H_ > #define _RTE_VFIO_H_ > > +#if !defined(VFIO_PRESENT) && defined(RTE_EAL_VFIO) > +#include > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0) > #include > +#define VFIO_PRESENT > +#endif /* >= 3.6.0 */ > +#endif > + > +#ifdef VFIO_PRESENT > > ... Need to wrap here in case VFIO disabled ... > > #endif > It would work indeed. I mostly dislike having whole compilation units disabled silently on a linux version check. I think that if someone wanted to support kernels < 3.6, they ought to do the work of disabling RTE_EAL_VFIO. A build error could be thrown to help those users toward the right solution, but I think that the meaning of having this option enabled should be enforced: if it is enabled, it is compiled. If dependencies are not met, then the option should be disabled. > > > > patch -p1 <<___HERE > > diff --git a/drivers/bus/pci/linux/Makefile b/drivers/bus/pci/linux/Makefile > > index 77c5f97..b5b9c54 100644 > > --- a/drivers/bus/pci/linux/Makefile > > +++ b/drivers/bus/pci/linux/Makefile > > @@ -31,6 +31,8 @@ > > > > SRCS += pci.c > > SRCS += pci_uio.c > > +ifeq (\$(CONFIG_RTE_EAL_VFIO),y) > > SRCS += pci_vfio.c > > +endif > > +1 > > Also ? > +ifeq ($(CONFIG_RTE_EAL_VFIO),y) > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_vfio.c > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_vfio_mp_sync.c > +endif > > Build shouldn't broken when VFIO disabled form config > > > > > CFLAGS += -D_GNU_SOURCE > > ___HERE > > > > --->8--- > > > > Do you think it could work? If so, I think it would be preferable > > to re-including eal_vfio.h. Private EAL headers are bound to be removed > > from other subsystems at some point. I tried to start with VFIO, others > > should follow. > > > >> Cc: Gaetan Rivet > >> > >> Signed-off-by: Jerin Jacob > >> --- > >> drivers/bus/pci/linux/pci.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c > >> index cdf810693..d0ce0207a 100644 > >> --- a/drivers/bus/pci/linux/pci.c > >> +++ b/drivers/bus/pci/linux/pci.c > >> @@ -46,6 +46,7 @@ > >> > >> #include "eal_private.h" > >> #include "eal_filesystem.h" > >> +#include "eal_vfio.h" > >> > >> #include "private.h" > >> #include "pci_init.h" > >> -- > >> 2.14.3 > >> > > > -- Gaëtan Rivet 6WIND