[v1,1/2] vfio: make API return values consistent

  Currently, when VFIO support is not compiled, FreeBSD and Linux have
different return values. Fix Linux implementation to follow FreeBSD one.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    Current minimum support Linux kernel is 4.4, and Meson build file sets the
    RTE_EAL_VFIO config option to `true` simply because we are compiling for Linux.
    So, it looks like VFIO support is pretty much assumed on Linux, so i think we
    can safely drop the fallback dummy implementation from Linux altogether?

 lib/eal/linux/eal_vfio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Hi Anatoly,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: Thursday, September 16, 2021 6:37 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> 
> Currently, when VFIO support is not compiled, FreeBSD and Linux have
> different return values. Fix Linux implementation to follow FreeBSD one.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     Current minimum support Linux kernel is 4.4, and Meson build file sets the

Do you mean currently DPDK support linux >= 4.4? I am not aware of this, could you
show me where it is defined?

And do we need backport? As 'return -1' does not align with the API doxygen.

Thanks,
Chenbo

>     RTE_EAL_VFIO config option to `true` simply because we are compiling for
> Linux.
>     So, it looks like VFIO support is pretty much assumed on Linux, so i think
> we
>     can safely drop the fallback dummy implementation from Linux altogether?
> 
>  lib/eal/linux/eal_vfio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
> index 25add2fa5d..b9e4d3ad3c 100644
> --- a/lib/eal/linux/eal_vfio.c
> +++ b/lib/eal/linux/eal_vfio.c
> @@ -2111,19 +2111,19 @@ rte_vfio_enable(__rte_unused const char *modname)
>  int
>  rte_vfio_is_enabled(__rte_unused const char *modname)
>  {
> -	return -1;
> +	return 0;
>  }
> 
>  int
>  rte_vfio_noiommu_is_enabled(void)
>  {
> -	return -1;
> +	return 0;
>  }
> 
>  int
>  rte_vfio_clear_group(__rte_unused int vfio_group_fd)
>  {
> -	return -1;
> +	return 0;
>  }
> 
>  int
> --
> 2.25.1
  

On 9/22/2021 4:30 AM, Xia, Chenbo wrote:
> Hi Anatoly,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
>> Sent: Thursday, September 16, 2021 6:37 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
>>
>> Currently, when VFIO support is not compiled, FreeBSD and Linux have
>> different return values. Fix Linux implementation to follow FreeBSD one.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>
>> Notes:
>>     Current minimum support Linux kernel is 4.4, and Meson build file sets the
> 
> Do you mean currently DPDK support linux >= 4.4? I am not aware of this, could you
> show me where it is defined?
> 

https://git.dpdk.org/dpdk/tree/doc/guides/linux_gsg/sys_reqs.rst?h=v21.08#n124

Commit d2feae68bf30 ("doc: update minimum supported Linux kernel")

> And do we need backport? As 'return -1' does not align with the API doxygen.
> 
> Thanks,
> Chenbo
> 
>>     RTE_EAL_VFIO config option to `true` simply because we are compiling for
>> Linux.
>>     So, it looks like VFIO support is pretty much assumed on Linux, so i think
>> we
>>     can safely drop the fallback dummy implementation from Linux altogether?
>>
>>  lib/eal/linux/eal_vfio.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
>> index 25add2fa5d..b9e4d3ad3c 100644
>> --- a/lib/eal/linux/eal_vfio.c
>> +++ b/lib/eal/linux/eal_vfio.c
>> @@ -2111,19 +2111,19 @@ rte_vfio_enable(__rte_unused const char *modname)
>>  int
>>  rte_vfio_is_enabled(__rte_unused const char *modname)
>>  {
>> -	return -1;
>> +	return 0;
>>  }
>>
>>  int
>>  rte_vfio_noiommu_is_enabled(void)
>>  {
>> -	return -1;
>> +	return 0;
>>  }
>>
>>  int
>>  rte_vfio_clear_group(__rte_unused int vfio_group_fd)
>>  {
>> -	return -1;
>> +	return 0;
>>  }
>>
>>  int
>> --
>> 2.25.1
>
  

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Wednesday, September 22, 2021 5:27 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> 
> On 9/22/2021 4:30 AM, Xia, Chenbo wrote:
> > Hi Anatoly,
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> >> Sent: Thursday, September 16, 2021 6:37 PM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> >>
> >> Currently, when VFIO support is not compiled, FreeBSD and Linux have
> >> different return values. Fix Linux implementation to follow FreeBSD one.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> ---
> >>
> >> Notes:
> >>     Current minimum support Linux kernel is 4.4, and Meson build file sets
> the
> >
> > Do you mean currently DPDK support linux >= 4.4? I am not aware of this,
> could you
> > show me where it is defined?
> >
> 
> https://git.dpdk.org/dpdk/tree/doc/guides/linux_gsg/sys_reqs.rst?h=v21.08#n124
> 
> Commit d2feae68bf30 ("doc: update minimum supported Linux kernel")

Thanks Ferruh!

About removing the fallback or not, if we decide to remove, maybe for other files
that check linux kernel version, we also need to change some check because we are
assuming kernel version >= 4.4 here. If that's a strong requirement, I'll vote for
yes...

/Chenbo

> 
> > And do we need backport? As 'return -1' does not align with the API doxygen.
> >
> > Thanks,
> > Chenbo
> >
> >>     RTE_EAL_VFIO config option to `true` simply because we are compiling
> for
> >> Linux.
> >>     So, it looks like VFIO support is pretty much assumed on Linux, so i
> think
> >> we
> >>     can safely drop the fallback dummy implementation from Linux altogether?
> >>
> >>  lib/eal/linux/eal_vfio.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
> >> index 25add2fa5d..b9e4d3ad3c 100644
> >> --- a/lib/eal/linux/eal_vfio.c
> >> +++ b/lib/eal/linux/eal_vfio.c
> >> @@ -2111,19 +2111,19 @@ rte_vfio_enable(__rte_unused const char *modname)
> >>  int
> >>  rte_vfio_is_enabled(__rte_unused const char *modname)
> >>  {
> >> -	return -1;
> >> +	return 0;
> >>  }
> >>
> >>  int
> >>  rte_vfio_noiommu_is_enabled(void)
> >>  {
> >> -	return -1;
> >> +	return 0;
> >>  }
> >>
> >>  int
> >>  rte_vfio_clear_group(__rte_unused int vfio_group_fd)
> >>  {
> >> -	return -1;
> >> +	return 0;
> >>  }
> >>
> >>  int
> >> --
> >> 2.25.1
> >
  

On Wed, Sep 22, 2021 at 1:31 PM Xia, Chenbo <chenbo.xia@intel.com> wrote:
>
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: Wednesday, September 22, 2021 5:27 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; Burakov, Anatoly
> > <anatoly.burakov@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> >
> > On 9/22/2021 4:30 AM, Xia, Chenbo wrote:
> > > Hi Anatoly,
> > >
> > >> -----Original Message-----
> > >> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> > >> Sent: Thursday, September 16, 2021 6:37 PM
> > >> To: dev@dpdk.org
> > >> Subject: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> > >>
> > >> Currently, when VFIO support is not compiled, FreeBSD and Linux have
> > >> different return values. Fix Linux implementation to follow FreeBSD one.
> > >>
> > >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > >> ---
> > >>
> > >> Notes:
> > >>     Current minimum support Linux kernel is 4.4, and Meson build file sets
> > the
> > >
> > > Do you mean currently DPDK support linux >= 4.4? I am not aware of this,
> > could you
> > > show me where it is defined?
> > >
> >
> > https://git.dpdk.org/dpdk/tree/doc/guides/linux_gsg/sys_reqs.rst?h=v21.08#n124
> >
> > Commit d2feae68bf30 ("doc: update minimum supported Linux kernel")
>
> Thanks Ferruh!
>
> About removing the fallback or not, if we decide to remove, maybe for other files
> that check linux kernel version, we also need to change some check because we are
> assuming kernel version >= 4.4 here. If that's a strong requirement, I'll vote for
> yes...

From those comments, it is unclear I can merge this series.
Anatoly?


Thanks.
  

Hi Chenbo,

> And do we need backport? As 'return -1' does not align with the API doxygen.
> 
> Thanks,
> Chenbo
> 
Maybe it's the FreeBSD implementation that needs to be adjusted then, 
because none of those functions are valid on FreeBSD, and the 
documentation for VFIO functions explicitly mentions that on FreeBSD, 
they should return an error. I went with adjusting Linux implementation 
to minimize the amount of changes we have to make (and only change code 
path that no one uses in the first place), but maybe that was a wrong 
decision.

I'm not sure if changing the API return value to match what was 
documented counts as an API change, so maybe backport to stable is not 
advised here.
  

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Thursday, October 28, 2021 6:30 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> 
> Hi Chenbo,
> 
> > And do we need backport? As 'return -1' does not align with the API doxygen.
> >
> > Thanks,
> > Chenbo
> >
> Maybe it's the FreeBSD implementation that needs to be adjusted then,
> because none of those functions are valid on FreeBSD, and the
> documentation for VFIO functions explicitly mentions that on FreeBSD,
> they should return an error. I went with adjusting Linux implementation
> to minimize the amount of changes we have to make (and only change code
> path that no one uses in the first place), but maybe that was a wrong
> decision.
> 
> I'm not sure if changing the API return value to match what was
> documented counts as an API change, so maybe backport to stable is not
> advised here.

It's not a API change. My point is whether VFIO is present, users just use
the API to check if vfio support is there. In a kernel version that does not
support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the doxygen
says its return value should be 1 as true or 0 as false. He will get true (-1)
but VFIO is not there. That's why I think it's a bug and should be backported.

But I think we can first discuss if we should drop the dummy implementation
as DPDK requires Linux kernel version >= 4.4 now so VFIO is always present.
I think it depends on by saying 'DPDK requires kernel version >= 4.4'. It's
a real _requirement_ or only a recommendation? 

Ferruh, David & Thomas, What do you think?

Thanks,
Chenbo

> 
> --
> Thanks,
> Anatoly
  

On 10/28/2021 12:11 PM, Xia, Chenbo wrote:
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Thursday, October 28, 2021 6:30 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
>>
>> Hi Chenbo,
>>
>>> And do we need backport? As 'return -1' does not align with the API doxygen.
>>>
>>> Thanks,
>>> Chenbo
>>>
>> Maybe it's the FreeBSD implementation that needs to be adjusted then,
>> because none of those functions are valid on FreeBSD, and the
>> documentation for VFIO functions explicitly mentions that on FreeBSD,
>> they should return an error. I went with adjusting Linux implementation
>> to minimize the amount of changes we have to make (and only change code
>> path that no one uses in the first place), but maybe that was a wrong
>> decision.
>>
>> I'm not sure if changing the API return value to match what was
>> documented counts as an API change, so maybe backport to stable is not
>> advised here.
> 
> It's not a API change. My point is whether VFIO is present, users just use
> the API to check if vfio support is there. In a kernel version that does not
> support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the doxygen
> says its return value should be 1 as true or 0 as false. He will get true (-1)
> but VFIO is not there. That's why I think it's a bug and should be backported.
> 
> But I think we can first discuss if we should drop the dummy implementation
> as DPDK requires Linux kernel version >= 4.4 now so VFIO is always present.
> I think it depends on by saying 'DPDK requires kernel version >= 4.4'. It's
> a real _requirement_ or only a recommendation?
> 
> Ferruh, David & Thomas, What do you think?
> 

My understanding is, it is a requirement. DPDK does not guarantee support for
kernels < 4.4.
  

On 28-Oct-21 12:32 PM, Ferruh Yigit wrote:
> On 10/28/2021 12:11 PM, Xia, Chenbo wrote:
>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Thursday, October 28, 2021 6:30 PM
>>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values 
>>> consistent
>>>
>>> Hi Chenbo,
>>>
>>>> And do we need backport? As 'return -1' does not align with the API 
>>>> doxygen.
>>>>
>>>> Thanks,
>>>> Chenbo
>>>>
>>> Maybe it's the FreeBSD implementation that needs to be adjusted then,
>>> because none of those functions are valid on FreeBSD, and the
>>> documentation for VFIO functions explicitly mentions that on FreeBSD,
>>> they should return an error. I went with adjusting Linux implementation
>>> to minimize the amount of changes we have to make (and only change code
>>> path that no one uses in the first place), but maybe that was a wrong
>>> decision.
>>>
>>> I'm not sure if changing the API return value to match what was
>>> documented counts as an API change, so maybe backport to stable is not
>>> advised here.
>>
>> It's not a API change. My point is whether VFIO is present, users just 
>> use
>> the API to check if vfio support is there. In a kernel version that 
>> does not
>> support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the 
>> doxygen
>> says its return value should be 1 as true or 0 as false. He will get 
>> true (-1)
>> but VFIO is not there. That's why I think it's a bug and should be 
>> backported.
>>
>> But I think we can first discuss if we should drop the dummy 
>> implementation
>> as DPDK requires Linux kernel version >= 4.4 now so VFIO is always 
>> present.
>> I think it depends on by saying 'DPDK requires kernel version >= 4.4'. 
>> It's
>> a real _requirement_ or only a recommendation?
>>
>> Ferruh, David & Thomas, What do you think?
>>
> 
> My understanding is, it is a requirement. DPDK does not guarantee 
> support for
> kernels < 4.4.

I'll resubmit with some changes then. I'll drop the fallback entirely, 
and check FreeBSD implementations for bugginess, and maybe correct the 
doxygen comment where necessary.
  

28/10/2021 13:32, Ferruh Yigit:
> On 10/28/2021 12:11 PM, Xia, Chenbo wrote:
> >> -----Original Message-----
> >> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >> Sent: Thursday, October 28, 2021 6:30 PM
> >> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> >>
> >> Hi Chenbo,
> >>
> >>> And do we need backport? As 'return -1' does not align with the API doxygen.
> >>>
> >>> Thanks,
> >>> Chenbo
> >>>
> >> Maybe it's the FreeBSD implementation that needs to be adjusted then,
> >> because none of those functions are valid on FreeBSD, and the
> >> documentation for VFIO functions explicitly mentions that on FreeBSD,
> >> they should return an error. I went with adjusting Linux implementation
> >> to minimize the amount of changes we have to make (and only change code
> >> path that no one uses in the first place), but maybe that was a wrong
> >> decision.
> >>
> >> I'm not sure if changing the API return value to match what was
> >> documented counts as an API change, so maybe backport to stable is not
> >> advised here.
> > 
> > It's not a API change. My point is whether VFIO is present, users just use
> > the API to check if vfio support is there. In a kernel version that does not
> > support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the doxygen
> > says its return value should be 1 as true or 0 as false. He will get true (-1)
> > but VFIO is not there. That's why I think it's a bug and should be backported.
> > 
> > But I think we can first discuss if we should drop the dummy implementation
> > as DPDK requires Linux kernel version >= 4.4 now so VFIO is always present.
> > I think it depends on by saying 'DPDK requires kernel version >= 4.4'. It's
> > a real _requirement_ or only a recommendation?
> > 
> > Ferruh, David & Thomas, What do you think?
> > 
> 
> My understanding is, it is a requirement. DPDK does not guarantee support for
> kernels < 4.4.

Do we have a kernel version check at runtime?
I think we should add a warning if running too old kernel.
  

On 28-Oct-21 3:53 PM, Thomas Monjalon wrote:
> 28/10/2021 13:32, Ferruh Yigit:
>> On 10/28/2021 12:11 PM, Xia, Chenbo wrote:
>>>> -----Original Message-----
>>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>>> Sent: Thursday, October 28, 2021 6:30 PM
>>>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
>>>>
>>>> Hi Chenbo,
>>>>
>>>>> And do we need backport? As 'return -1' does not align with the API doxygen.
>>>>>
>>>>> Thanks,
>>>>> Chenbo
>>>>>
>>>> Maybe it's the FreeBSD implementation that needs to be adjusted then,
>>>> because none of those functions are valid on FreeBSD, and the
>>>> documentation for VFIO functions explicitly mentions that on FreeBSD,
>>>> they should return an error. I went with adjusting Linux implementation
>>>> to minimize the amount of changes we have to make (and only change code
>>>> path that no one uses in the first place), but maybe that was a wrong
>>>> decision.
>>>>
>>>> I'm not sure if changing the API return value to match what was
>>>> documented counts as an API change, so maybe backport to stable is not
>>>> advised here.
>>>
>>> It's not a API change. My point is whether VFIO is present, users just use
>>> the API to check if vfio support is there. In a kernel version that does not
>>> support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the doxygen
>>> says its return value should be 1 as true or 0 as false. He will get true (-1)
>>> but VFIO is not there. That's why I think it's a bug and should be backported.
>>>
>>> But I think we can first discuss if we should drop the dummy implementation
>>> as DPDK requires Linux kernel version >= 4.4 now so VFIO is always present.
>>> I think it depends on by saying 'DPDK requires kernel version >= 4.4'. It's
>>> a real _requirement_ or only a recommendation?
>>>
>>> Ferruh, David & Thomas, What do you think?
>>>
>>
>> My understanding is, it is a requirement. DPDK does not guarantee support for
>> kernels < 4.4.
> 
> Do we have a kernel version check at runtime?
> I think we should add a warning if running too old kernel.
> 

Perhaps we should (there's a `uname()` call, should be fairly 
straightforward to implement), but obviously this would be outside the 
scope for this patchset.
  

On Thu, 28 Oct 2021 16:40:28 +0100
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> On 28-Oct-21 3:53 PM, Thomas Monjalon wrote:
> > 28/10/2021 13:32, Ferruh Yigit:  
> >> On 10/28/2021 12:11 PM, Xia, Chenbo wrote:  
> >>>> -----Original Message-----
> >>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >>>> Sent: Thursday, October 28, 2021 6:30 PM
> >>>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
> >>>>
> >>>> Hi Chenbo,
> >>>>  
> >>>>> And do we need backport? As 'return -1' does not align with the API doxygen.
> >>>>>
> >>>>> Thanks,
> >>>>> Chenbo
> >>>>>  
> >>>> Maybe it's the FreeBSD implementation that needs to be adjusted then,
> >>>> because none of those functions are valid on FreeBSD, and the
> >>>> documentation for VFIO functions explicitly mentions that on FreeBSD,
> >>>> they should return an error. I went with adjusting Linux implementation
> >>>> to minimize the amount of changes we have to make (and only change code
> >>>> path that no one uses in the first place), but maybe that was a wrong
> >>>> decision.
> >>>>
> >>>> I'm not sure if changing the API return value to match what was
> >>>> documented counts as an API change, so maybe backport to stable is not
> >>>> advised here.  
> >>>
> >>> It's not a API change. My point is whether VFIO is present, users just use
> >>> the API to check if vfio support is there. In a kernel version that does not
> >>> support VFIO, he uses 'if(rte_vfio_is_enabled(XXX))' to check as the doxygen
> >>> says its return value should be 1 as true or 0 as false. He will get true (-1)
> >>> but VFIO is not there. That's why I think it's a bug and should be backported.
> >>>
> >>> But I think we can first discuss if we should drop the dummy implementation
> >>> as DPDK requires Linux kernel version >= 4.4 now so VFIO is always present.
> >>> I think it depends on by saying 'DPDK requires kernel version >= 4.4'. It's
> >>> a real _requirement_ or only a recommendation?
> >>>
> >>> Ferruh, David & Thomas, What do you think?
> >>>  
> >>
> >> My understanding is, it is a requirement. DPDK does not guarantee support for
> >> kernels < 4.4.  
> > 
> > Do we have a kernel version check at runtime?
> > I think we should add a warning if running too old kernel.
> >   
> 
> Perhaps we should (there's a `uname()` call, should be fairly 
> straightforward to implement), but obviously this would be outside the 
> scope for this patchset.
> 

Checking kernel version at runtime with uname() is a bad idea because
enterprise distro vendors support things without updating the kernel version.
It is always better to try for the feature and handle errors that could be
reported by older kernels.

Also DPDK System Requirements says kernel >= 4.4 on Linux.