[PATCH] bus/pci: fix vfio mode

[PATCH] bus/pci: fix vfio mode

[Jerin Jacob]

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")

Cc: Gaetan Rivet <gaetan.rivet@6wind.com>

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 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

Re: [PATCH] bus/pci: fix vfio mode

[Gaëtan Rivet]

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

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

 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 <gaetan.rivet@6wind.com>
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>  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

Re: [PATCH] bus/pci: fix vfio mode

[Ferruh Yigit]

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 <linux/version.h>
  +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
   #include <linux/vfio.h>
  +#define VFIO_PRESENT
  +#endif /* >= 3.6.0 */
  +#endif
  +
  +#ifdef VFIO_PRESENT

  ... Need to wrap here in case VFIO disabled ...

  #endif

> 
> 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 <gaetan.rivet@6wind.com>
>>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> ---
>>  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
>>
> 

Re: [PATCH] bus/pci: fix vfio mode

[Gaëtan Rivet]

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 <linux/version.h>
>   +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
>    #include <linux/vfio.h>
>   +#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 <gaetan.rivet@6wind.com>
> >>
> >> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >> ---
> >>  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

Re: [PATCH] bus/pci: fix vfio mode

[Thomas Monjalon]

30/10/2017 10:17, Gaëtan Rivet:
> 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 <linux/version.h>
> >   +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
> >    #include <linux/vfio.h>
> >   +#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.

+1 to avoid implicit disabling.

Re: [PATCH] bus/pci: fix vfio mode

[Thomas Monjalon]

30/10/2017 10:27, Thomas Monjalon:
> 30/10/2017 10:17, Gaëtan Rivet:
> > 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 <linux/version.h>
> > >   +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
> > >    #include <linux/vfio.h>
> > >   +#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.
> 
> +1 to avoid implicit disabling.

To make it clear, we can disable VFIO automatically if not supported by
the kernel at compilation time, but there should be a warning.

Re: [PATCH] bus/pci: fix vfio mode

[Ferruh Yigit]

On 10/30/2017 4:24 AM, Thomas Monjalon wrote:
> 30/10/2017 10:27, Thomas Monjalon:
>> 30/10/2017 10:17, Gaëtan Rivet:
>>> 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 <linux/version.h>
>>>>   +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
>>>>    #include <linux/vfio.h>
>>>>   +#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.
>>
>> +1 to avoid implicit disabling.
> 
> To make it clear, we can disable VFIO automatically if not supported by
> the kernel at compilation time, but there should be a warning.

Gaetan suggest letting build error and push user to disable the feature. If we
let the build error, I think feature should be disabled by default.

And for VFIO, instead of disabling it by default I am for auto disable it if
kernel requirement not met, and +1 for adding a warning.

btw, that kernel version check is already in eal_vfio.h, we are already
disabling some vfio code based on kernel version check, weather add here or not.

Thanks,
ferruh

Re: [PATCH] bus/pci: fix vfio mode

[Gaëtan Rivet]

On Mon, Oct 30, 2017 at 09:57:08AM -0700, Ferruh Yigit wrote:
> On 10/30/2017 4:24 AM, Thomas Monjalon wrote:
> > 30/10/2017 10:27, Thomas Monjalon:
> >> 30/10/2017 10:17, Gaëtan Rivet:
> >>> 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 <linux/version.h>
> >>>>   +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
> >>>>    #include <linux/vfio.h>
> >>>>   +#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.
> >>
> >> +1 to avoid implicit disabling.
> > 
> > To make it clear, we can disable VFIO automatically if not supported by
> > the kernel at compilation time, but there should be a warning.
> 
> Gaetan suggest letting build error and push user to disable the feature. If we
> let the build error, I think feature should be disabled by default.
> 
> And for VFIO, instead of disabling it by default I am for auto disable it if
> kernel requirement not met, and +1 for adding a warning.
> 
> btw, that kernel version check is already in eal_vfio.h, we are already
> disabling some vfio code based on kernel version check, weather add here or not.
> 
> Thanks,
> ferruh

The new version of the patches should behave like you described :) .

I still think VFIO_PRESENT should be changed, but not now.

-- 
Gaëtan Rivet
6WIND

Re: [PATCH] bus/pci: fix vfio mode

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 30 Oct 2017 09:06:54 +0100
> From: Gaëtan Rivet <gaetan.rivet@6wind.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH] bus/pci: fix vfio mode
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> Hi Jerin,

Hi Gaëtan,

> 
> 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:

That will work. But, I think, We can push this patch as a hot fix as master is
broken now.

[PATCH v2 1/2] bus/pci: fix vfio mode

[Gaetan Rivet]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Revert back to using VFIO_PRESENT as a marker to enable compilation
of VFIO-related segments.

Fixes: 279b581c897d ("vfio: expose functions")

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---

In addition to simply re-defining VFIO_PRESENT, the previous #ifdef
removed in favor of RTE_EAL_VFIO must be switched back as well.

Not doing so would break compilation on kernels prior to 3.6.

Additionally, the following patch adds a warning for users.

This fix is left as-is, instead of a more involved solution like
Ferruh's. The reasoning behind it is that it should be made abundantly
clear that the VFIO_PRESENT symbol is defined in eal_vfio.h and
eal_vfio.h only, that it is a private EAL symbol that is currently
bleeding out of the EAL. It should be visible when those private symbols
will be removed from libs external to the EAL.

 drivers/bus/pci/linux/pci.c      | 1 +
 drivers/bus/pci/linux/pci_init.h | 2 +-
 drivers/bus/pci/linux/pci_vfio.c | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index cdf8106..d0ce020 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"
diff --git a/drivers/bus/pci/linux/pci_init.h b/drivers/bus/pci/linux/pci_init.h
index 99d7a2e..f342c47 100644
--- a/drivers/bus/pci/linux/pci_init.h
+++ b/drivers/bus/pci/linux/pci_init.h
@@ -74,7 +74,7 @@ void pci_uio_ioport_write(struct rte_pci_ioport *p,
 			  const void *data, size_t len, off_t offset);
 int pci_uio_ioport_unmap(struct rte_pci_ioport *p);
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 10, 0)
 #define RTE_PCI_MSIX_TABLE_BIR    0x7
diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 360eed3..13d52a8 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -62,7 +62,7 @@
  * This file is only compiled if CONFIG_RTE_EAL_VFIO is set to "y".
  */
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #define PAGE_SIZE   (sysconf(_SC_PAGESIZE))
 #define PAGE_MASK   (~(PAGE_SIZE - 1))
-- 
2.1.4

[PATCH v2 2/2] eal: warn user that VFIO is disabled

[Gaetan Rivet]

If VFIO is not compatible with the kernel used, print a soft warning to
the user if RTE_EAL_VFIO was still enabled in the configuration.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_vfio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
index 766d004..fadd274 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
@@ -183,6 +183,8 @@ int vfio_mp_sync_setup(void);
 #define SOCKET_ERR 0xFF
 
 #define VFIO_PRESENT
+#else
+#pragma message("VFIO configured but not supported by this kernel, disabling.")
 #endif /* kernel version */
 #endif /* RTE_EAL_VFIO */
 
-- 
2.1.4

[PATCH v3 1/2] bus/pci: fix vfio mode

[Gaetan Rivet]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Revert back to using VFIO_PRESENT as a marker to enable compilation
of VFIO-related segments.

Fixes: 279b581c897d ("vfio: expose functions")

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---

In addition to simply re-defining VFIO_PRESENT, the previous #ifdef
removed in favor of RTE_EAL_VFIO must be switched back as well.

Not doing so would break compilation on kernels prior to 3.6.

Additionally, the following patch adds a warning for users.

This fix is left as-is, instead of a more involved solution like
Ferruh's. The reasoning behind it is that it should be made abundantly
clear that the VFIO_PRESENT symbol is defined in eal_vfio.h and
eal_vfio.h only, that it is a private EAL symbol that is currently
bleeding out of the EAL. It should be visible when those private symbols
will be removed from libs external to the EAL.

v3:

  - Fix build by include eal_vfio.h in pci_init.h and pci.c as well.

 drivers/bus/pci/linux/pci.c      | 1 +
 drivers/bus/pci/linux/pci_init.h | 4 +++-
 drivers/bus/pci/linux/pci_vfio.c | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index cdf8106..d0ce020 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"
diff --git a/drivers/bus/pci/linux/pci_init.h b/drivers/bus/pci/linux/pci_init.h
index 99d7a2e..3290cee 100644
--- a/drivers/bus/pci/linux/pci_init.h
+++ b/drivers/bus/pci/linux/pci_init.h
@@ -38,6 +38,8 @@
 
 #include <rte_vfio.h>
 
+#include "eal_vfio.h"
+
 /** IO resource type: */
 #define IORESOURCE_IO         0x00000100
 #define IORESOURCE_MEM        0x00000200
@@ -74,7 +76,7 @@ void pci_uio_ioport_write(struct rte_pci_ioport *p,
 			  const void *data, size_t len, off_t offset);
 int pci_uio_ioport_unmap(struct rte_pci_ioport *p);
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 10, 0)
 #define RTE_PCI_MSIX_TABLE_BIR    0x7
diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 360eed3..38d3792 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -48,6 +48,7 @@
 #include <rte_vfio.h>
 
 #include "eal_filesystem.h"
+#include "eal_vfio.h"
 
 #include "pci_init.h"
 #include "private.h"
@@ -62,7 +63,7 @@
  * This file is only compiled if CONFIG_RTE_EAL_VFIO is set to "y".
  */
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #define PAGE_SIZE   (sysconf(_SC_PAGESIZE))
 #define PAGE_MASK   (~(PAGE_SIZE - 1))
-- 
2.1.4

[PATCH v3 2/2] eal: warn user that VFIO is disabled

[Gaetan Rivet]

If VFIO is not compatible with the kernel used, print a soft warning to
the user if RTE_EAL_VFIO was still enabled in the configuration.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_vfio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
index 766d004..fadd274 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
@@ -183,6 +183,8 @@ int vfio_mp_sync_setup(void);
 #define SOCKET_ERR 0xFF
 
 #define VFIO_PRESENT
+#else
+#pragma message("VFIO configured but not supported by this kernel, disabling.")
 #endif /* kernel version */
 #endif /* RTE_EAL_VFIO */
 
-- 
2.1.4

Re: [PATCH v3 2/2] eal: warn user that VFIO is disabled

[Ferruh Yigit]

On 10/30/2017 9:36 AM, Gaetan Rivet wrote:
> If VFIO is not compatible with the kernel used, print a soft warning to
> the user if RTE_EAL_VFIO was still enabled in the configuration.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_vfio.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
> index 766d004..fadd274 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
> @@ -183,6 +183,8 @@ int vfio_mp_sync_setup(void);
>  #define SOCKET_ERR 0xFF
>  
>  #define VFIO_PRESENT
> +#else
> +#pragma message("VFIO configured but not supported by this kernel, disabling.")

There is a flaw here, but it seems it was already there, nothing coming for this
patch.

There is a RTE_KERNELDIR variable, to compile DPDK with a given kernel, instead
of default one. I am using this a lot for kni and igb_uio.

Default RTE_KERNELDIR is:
RTE_KERNELDIR ?= /lib/modules/$(shell uname -r)/build

For vfio case, it is not taking this variable into account and always using the
default one, so RTE_KERNELDIR is broken here.

>  #endif /* kernel version */
>  #endif /* RTE_EAL_VFIO */
>  
> 

Re: [PATCH v3 1/2] bus/pci: fix vfio mode

[Ferruh Yigit]

On 10/30/2017 9:36 AM, Gaetan Rivet wrote:
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> Revert back to using VFIO_PRESENT as a marker to enable compilation
> of VFIO-related segments.
> 
> Fixes: 279b581c897d ("vfio: expose functions")
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [PATCH v3 1/2] bus/pci: fix vfio mode

[Ferruh Yigit]

On 10/30/2017 10:26 AM, Ferruh Yigit wrote:
> On 10/30/2017 9:36 AM, Gaetan Rivet wrote:
>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>
>> Revert back to using VFIO_PRESENT as a marker to enable compilation
>> of VFIO-related segments.
>>
>> Fixes: 279b581c897d ("vfio: expose functions")
>>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> 
> Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

Opps, this still will break the build with kernel < 3.6

Because rte_vfio.h includes "linux/vfio.h" unconditionally, I am still for
adding a version check in it to fix build for old kernels.

[PATCH v4] bus/pci: fix vfio mode

[Ferruh Yigit]

Revert back to using VFIO_PRESENT as a marker to enable compilation
of VFIO-related segments.

VFIO_PRESENT is the combination of user configuration RTE_EAL_VFIO and
kernel version support check.

eal_vfio.h VFIO_PRESENT related check ordered to be compatible with
rte_vfio.h one, no functional modification.

Fixes: 279b581c897d ("vfio: expose functions")

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v4:

* Alternative to v3, not superseding it, need to decide which one to
get. This one:
  * fixes vfio for new kernels
  * fixes build for old kernels for vfio enabled in config case
  * print warning if vfio enabled in config but not supported by kernel

* Independent from this patch, following may need fixing later
  * Kernel version check for vfio is broken for RTE_KERNELDIR
  * Kernel VFIO support check can be improved
---
 drivers/bus/pci/linux/pci_init.h             |  2 +-
 drivers/bus/pci/linux/pci_vfio.c             |  2 +-
 lib/librte_eal/common/include/rte_vfio.h     | 14 ++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_interrupts.c |  1 -
 lib/librte_eal/linuxapp/eal/eal_vfio.h       | 14 ++++++++++----
 5 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_init.h b/drivers/bus/pci/linux/pci_init.h
index 99d7a2e86..f342c47de 100644
--- a/drivers/bus/pci/linux/pci_init.h
+++ b/drivers/bus/pci/linux/pci_init.h
@@ -74,7 +74,7 @@ void pci_uio_ioport_write(struct rte_pci_ioport *p,
 			  const void *data, size_t len, off_t offset);
 int pci_uio_ioport_unmap(struct rte_pci_ioport *p);
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 10, 0)
 #define RTE_PCI_MSIX_TABLE_BIR    0x7
diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 360eed380..13d52a8b1 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -62,7 +62,7 @@
  * This file is only compiled if CONFIG_RTE_EAL_VFIO is set to "y".
  */
 
-#ifdef RTE_EAL_VFIO
+#ifdef VFIO_PRESENT
 
 #define PAGE_SIZE   (sysconf(_SC_PAGESIZE))
 #define PAGE_MASK   (~(PAGE_SIZE - 1))
diff --git a/lib/librte_eal/common/include/rte_vfio.h b/lib/librte_eal/common/include/rte_vfio.h
index a9b705741..d5934cdd3 100644
--- a/lib/librte_eal/common/include/rte_vfio.h
+++ b/lib/librte_eal/common/include/rte_vfio.h
@@ -34,6 +34,18 @@
 #ifndef _RTE_VFIO_H_
 #define _RTE_VFIO_H_
 
+/*
+ * determine if VFIO is present on the system
+ */
+#if !defined(VFIO_PRESENT) && defined(RTE_EAL_VFIO)
+#include <linux/version.h>
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
+#define VFIO_PRESENT
+#endif /* kernel version >= 3.6.0 */
+#endif /* RTE_EAL_VFIO */
+
+#ifdef VFIO_PRESENT
+
 #include <linux/vfio.h>
 
 #define VFIO_DIR "/dev/vfio"
@@ -136,4 +148,6 @@ int vfio_is_enabled(const char *modname);
  */
 int vfio_noiommu_is_enabled(void);
 
+#endif /* VFIO_PRESENT */
+
 #endif /* _RTE_VFIO_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index cdd74ee10..0bebf004b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -64,7 +64,6 @@
 #include <rte_errno.h>
 #include <rte_spinlock.h>
 #include <rte_pause.h>
-#include <rte_vfio.h>
 
 #include "eal_private.h"
 #include "eal_vfio.h"
diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
index 766d0042d..ba7892b72 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
@@ -37,9 +37,17 @@
 /*
  * determine if VFIO is present on the system
  */
-#ifdef RTE_EAL_VFIO
+#if !defined(VFIO_PRESENT) && defined(RTE_EAL_VFIO)
 #include <linux/version.h>
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 6, 0)
+#define VFIO_PRESENT
+#else
+#pragma message("VFIO configured but not supported by this kernel, disabling.")
+#endif /* kernel version >= 3.6.0 */
+#endif /* RTE_EAL_VFIO */
+
+#ifdef VFIO_PRESENT
+
 #include <linux/vfio.h>
 
 #define RTE_VFIO_TYPE1 VFIO_TYPE1_IOMMU
@@ -182,8 +190,6 @@ int vfio_mp_sync_setup(void);
 #define SOCKET_NO_FD 0x1
 #define SOCKET_ERR 0xFF
 
-#define VFIO_PRESENT
-#endif /* kernel version */
-#endif /* RTE_EAL_VFIO */
+#endif /* VFIO_PRESENT */
 
 #endif /* EAL_VFIO_H_ */
-- 
2.13.6

Re: [PATCH v4] bus/pci: fix vfio mode

[Bruce Richardson]

On Mon, Oct 30, 2017 at 10:32:27PM +0000, Ferruh Yigit wrote:
> Revert back to using VFIO_PRESENT as a marker to enable compilation
> of VFIO-related segments.
> 
> VFIO_PRESENT is the combination of user configuration RTE_EAL_VFIO and
> kernel version support check.
> 
> eal_vfio.h VFIO_PRESENT related check ordered to be compatible with
> rte_vfio.h one, no functional modification.
> 
> Fixes: 279b581c897d ("vfio: expose functions")
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> v4:
> 
> * Alternative to v3, not superseding it, need to decide which one to
> get. This one:
>   * fixes vfio for new kernels
>   * fixes build for old kernels for vfio enabled in config case
>   * print warning if vfio enabled in config but not supported by kernel
> 
> * Independent from this patch, following may need fixing later
>   * Kernel version check for vfio is broken for RTE_KERNELDIR
>   * Kernel VFIO support check can be improved
> ---
>  drivers/bus/pci/linux/pci_init.h             |  2 +-
>  drivers/bus/pci/linux/pci_vfio.c             |  2 +-
>  lib/librte_eal/common/include/rte_vfio.h     | 14 ++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c |  1 -
>  lib/librte_eal/linuxapp/eal/eal_vfio.h       | 14 ++++++++++----
>  5 files changed, 26 insertions(+), 7 deletions(-)
> 
Hi,

just to confirm that applying this patch fixes DPDK on my system at
least.

>From the discussion, I understood that this was a "compilation problem",
but in practice it manifests itself as compile working ok but your app
won't work with ports bound to vfio-pci. So please apply some fix for
this soon.

Thanks for the work on this, all.

Tested-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v4] bus/pci: fix vfio mode

[Ferruh Yigit]

On 10/31/2017 5:03 AM, Bruce Richardson wrote:
> On Mon, Oct 30, 2017 at 10:32:27PM +0000, Ferruh Yigit wrote:
>> Revert back to using VFIO_PRESENT as a marker to enable compilation
>> of VFIO-related segments.
>>
>> VFIO_PRESENT is the combination of user configuration RTE_EAL_VFIO and
>> kernel version support check.
>>
>> eal_vfio.h VFIO_PRESENT related check ordered to be compatible with
>> rte_vfio.h one, no functional modification.
>>
>> Fixes: 279b581c897d ("vfio: expose functions")
>>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> v4:
>>
>> * Alternative to v3, not superseding it, need to decide which one to
>> get. This one:
>>   * fixes vfio for new kernels
>>   * fixes build for old kernels for vfio enabled in config case
>>   * print warning if vfio enabled in config but not supported by kernel
>>
>> * Independent from this patch, following may need fixing later
>>   * Kernel version check for vfio is broken for RTE_KERNELDIR
>>   * Kernel VFIO support check can be improved
>> ---
>>  drivers/bus/pci/linux/pci_init.h             |  2 +-
>>  drivers/bus/pci/linux/pci_vfio.c             |  2 +-
>>  lib/librte_eal/common/include/rte_vfio.h     | 14 ++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_interrupts.c |  1 -
>>  lib/librte_eal/linuxapp/eal/eal_vfio.h       | 14 ++++++++++----
>>  5 files changed, 26 insertions(+), 7 deletions(-)
>>
> Hi,
> 
> just to confirm that applying this patch fixes DPDK on my system at
> least.
> 
> From the discussion, I understood that this was a "compilation problem",
> but in practice it manifests itself as compile working ok but your app
> won't work with ports bound to vfio-pci. So please apply some fix for
> this soon.

The main reason of the patch is vfio issue. All three of patches from Jerin,
Gaetan, and this are fixing that issue.

Difference is:
- Create or not dependency to eal_vfio.h again
- Break or not build for old kernels

> 
> Thanks for the work on this, all.
> 
> Tested-by: Bruce Richardson <bruce.richardson@intel.com>
> 

Re: [PATCH v4] bus/pci: fix vfio mode

[Thomas Monjalon]

31/10/2017 13:03, Bruce Richardson:
> On Mon, Oct 30, 2017 at 10:32:27PM +0000, Ferruh Yigit wrote:
> > Revert back to using VFIO_PRESENT as a marker to enable compilation
> > of VFIO-related segments.
> > 
> > VFIO_PRESENT is the combination of user configuration RTE_EAL_VFIO and
> > kernel version support check.
> > 
> > eal_vfio.h VFIO_PRESENT related check ordered to be compatible with
> > rte_vfio.h one, no functional modification.
> > 
> > Fixes: 279b581c897d ("vfio: expose functions")
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > v4:
> > 
> > * Alternative to v3, not superseding it, need to decide which one to
> > get. This one:
> >   * fixes vfio for new kernels
> >   * fixes build for old kernels for vfio enabled in config case
> >   * print warning if vfio enabled in config but not supported by kernel
> > 
> > * Independent from this patch, following may need fixing later
> >   * Kernel version check for vfio is broken for RTE_KERNELDIR
> >   * Kernel VFIO support check can be improved
> > ---
> >  drivers/bus/pci/linux/pci_init.h             |  2 +-
> >  drivers/bus/pci/linux/pci_vfio.c             |  2 +-
> >  lib/librte_eal/common/include/rte_vfio.h     | 14 ++++++++++++++
> >  lib/librte_eal/linuxapp/eal/eal_interrupts.c |  1 -
> >  lib/librte_eal/linuxapp/eal/eal_vfio.h       | 14 ++++++++++----
> >  5 files changed, 26 insertions(+), 7 deletions(-)
> > 
> Hi,
> 
> just to confirm that applying this patch fixes DPDK on my system at
> least.
> 
> From the discussion, I understood that this was a "compilation problem",
> but in practice it manifests itself as compile working ok but your app
> won't work with ports bound to vfio-pci. So please apply some fix for
> this soon.
> 
> Thanks for the work on this, all.
> 
> Tested-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks

Re: [PATCH v4] bus/pci: fix vfio mode

[Gaëtan Rivet]

Hi Ferruh,

On Mon, Oct 30, 2017 at 10:32:27PM +0000, Ferruh Yigit wrote:
> Revert back to using VFIO_PRESENT as a marker to enable compilation
> of VFIO-related segments.
> 
> VFIO_PRESENT is the combination of user configuration RTE_EAL_VFIO and
> kernel version support check.
> 
> eal_vfio.h VFIO_PRESENT related check ordered to be compatible with
> rte_vfio.h one, no functional modification.
> 
> Fixes: 279b581c897d ("vfio: expose functions")
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> v4:
> 
> * Alternative to v3, not superseding it, need to decide which one to
> get. This one:
>   * fixes vfio for new kernels
>   * fixes build for old kernels for vfio enabled in config case
>   * print warning if vfio enabled in config but not supported by kernel
> 

Thanks for the v4.

Well, as I stated before, I am more of the opinion of letting the
VFIO_PRESENT define only within eal_vfio.h.

But in the end, as long as everything is working, I'm fine with it.
Unless someone is explicitly interested in going with my version, let's
keep yours. If needed I can always submit a new v4 that would actually
work with old kernels.

Sorry about the mess concerning VFIO. I found late in the work that I
needed to touch it, and did not have the test-bed for it.