DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
@ 2021-09-16 10:36 Anatoly Burakov
  2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-09-16 10:36 UTC (permalink / raw)
  To: dev

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

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms
  2021-09-16 10:36 [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Anatoly Burakov
@ 2021-09-16 10:36 ` Anatoly Burakov
  2021-09-16 10:39   ` Bruce Richardson
  2021-09-22  3:31   ` Xia, Chenbo
  2021-09-22  3:30 ` [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Xia, Chenbo
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2 siblings, 2 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-09-16 10:36 UTC (permalink / raw)
  To: dev, Bruce Richardson

Currently, when code is running on FreeBSD (or using fallback Linux
implementation), there is no way to distinguish between a geniune error
and a "VFIO is unsupported" error. Fix the dummy implemnetations to also
set the rte_errno flag.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/freebsd/eal.c    | 12 ++++++++++++
 lib/eal/linux/eal_vfio.c | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index 6cee5ae369..6d249edb11 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -1000,6 +1000,7 @@ int rte_vfio_setup_device(__rte_unused const char *sysfs_base,
 		      __rte_unused int *vfio_dev_fd,
 		      __rte_unused struct vfio_device_info *device_info)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1007,11 +1008,13 @@ int rte_vfio_release_device(__rte_unused const char *sysfs_base,
 			__rte_unused const char *dev_addr,
 			__rte_unused int fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int rte_vfio_enable(__rte_unused const char *modname)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1035,30 +1038,35 @@ rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
 		       __rte_unused const char *dev_addr,
 		       __rte_unused int *iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_container_fd(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_create(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_destroy(__rte_unused int container_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1066,6 +1074,7 @@ int
 rte_vfio_container_group_bind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1073,6 +1082,7 @@ int
 rte_vfio_container_group_unbind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1082,6 +1092,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1091,5 +1102,6 @@ rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index b9e4d3ad3c..bb4c225aed 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -2092,6 +2092,7 @@ rte_vfio_setup_device(__rte_unused const char *sysfs_base,
 		__rte_unused int *vfio_dev_fd,
 		__rte_unused struct vfio_device_info *device_info)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2099,12 +2100,14 @@ int
 rte_vfio_release_device(__rte_unused const char *sysfs_base,
 		__rte_unused const char *dev_addr, __rte_unused int fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_enable(__rte_unused const char *modname)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2131,30 +2134,35 @@ rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
 		__rte_unused const char *dev_addr,
 		__rte_unused int *iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_container_fd(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_create(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_destroy(__rte_unused int container_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2162,6 +2170,7 @@ int
 rte_vfio_container_group_bind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2169,6 +2178,7 @@ int
 rte_vfio_container_group_unbind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2178,6 +2188,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
 		__rte_unused uint64_t iova,
 		__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -2187,6 +2198,7 @@ rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 		__rte_unused uint64_t iova,
 		__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms
  2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-09-16 10:39   ` Bruce Richardson
  2021-09-22  3:31   ` Xia, Chenbo
  1 sibling, 0 replies; 30+ messages in thread
From: Bruce Richardson @ 2021-09-16 10:39 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev

On Thu, Sep 16, 2021 at 10:36:38AM +0000, Anatoly Burakov wrote:
> Currently, when code is running on FreeBSD (or using fallback Linux
> implementation), there is no way to distinguish between a geniune error
> and a "VFIO is unsupported" error. Fix the dummy implemnetations to also
> set the rte_errno flag.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/eal/freebsd/eal.c    | 12 ++++++++++++
>  lib/eal/linux/eal_vfio.c | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-09-16 10:36 [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Anatoly Burakov
  2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-09-22  3:30 ` Xia, Chenbo
  2021-09-22  9:26   ` Ferruh Yigit
  2021-10-28 10:29   ` Burakov, Anatoly
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2 siblings, 2 replies; 30+ messages in thread
From: Xia, Chenbo @ 2021-09-22  3:30 UTC (permalink / raw)
  To: Burakov, Anatoly, dev

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms
  2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
  2021-09-16 10:39   ` Bruce Richardson
@ 2021-09-22  3:31   ` Xia, Chenbo
  1 sibling, 0 replies; 30+ messages in thread
From: Xia, Chenbo @ 2021-09-22  3:31 UTC (permalink / raw)
  To: Burakov, Anatoly, dev, Richardson, Bruce

> -----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; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms
> 
> Currently, when code is running on FreeBSD (or using fallback Linux
> implementation), there is no way to distinguish between a geniune error
> and a "VFIO is unsupported" error. Fix the dummy implemnetations to also

Should be 'implementation'

With this fixed:

Acked-by: Chenbo Xia <chenbo.xia@intel.com>

> set the rte_errno flag.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/eal/freebsd/eal.c    | 12 ++++++++++++
>  lib/eal/linux/eal_vfio.c | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index 6cee5ae369..6d249edb11 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -1000,6 +1000,7 @@ int rte_vfio_setup_device(__rte_unused const char
> *sysfs_base,
>  		      __rte_unused int *vfio_dev_fd,
>  		      __rte_unused struct vfio_device_info *device_info)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1007,11 +1008,13 @@ int rte_vfio_release_device(__rte_unused const char
> *sysfs_base,
>  			__rte_unused const char *dev_addr,
>  			__rte_unused int fd)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int rte_vfio_enable(__rte_unused const char *modname)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1035,30 +1038,35 @@ rte_vfio_get_group_num(__rte_unused const char
> *sysfs_base,
>  		       __rte_unused const char *dev_addr,
>  		       __rte_unused int *iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_get_container_fd(void)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_container_create(void)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_container_destroy(__rte_unused int container_fd)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1066,6 +1074,7 @@ int
>  rte_vfio_container_group_bind(__rte_unused int container_fd,
>  		__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1073,6 +1082,7 @@ int
>  rte_vfio_container_group_unbind(__rte_unused int container_fd,
>  		__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1082,6 +1092,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
>  			__rte_unused uint64_t iova,
>  			__rte_unused uint64_t len)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -1091,5 +1102,6 @@ rte_vfio_container_dma_unmap(__rte_unused int
> container_fd,
>  			__rte_unused uint64_t iova,
>  			__rte_unused uint64_t len)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
> index b9e4d3ad3c..bb4c225aed 100644
> --- a/lib/eal/linux/eal_vfio.c
> +++ b/lib/eal/linux/eal_vfio.c
> @@ -2092,6 +2092,7 @@ rte_vfio_setup_device(__rte_unused const char
> *sysfs_base,
>  		__rte_unused int *vfio_dev_fd,
>  		__rte_unused struct vfio_device_info *device_info)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2099,12 +2100,14 @@ int
>  rte_vfio_release_device(__rte_unused const char *sysfs_base,
>  		__rte_unused const char *dev_addr, __rte_unused int fd)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_enable(__rte_unused const char *modname)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2131,30 +2134,35 @@ rte_vfio_get_group_num(__rte_unused const char
> *sysfs_base,
>  		__rte_unused const char *dev_addr,
>  		__rte_unused int *iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_get_container_fd(void)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_container_create(void)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
>  int
>  rte_vfio_container_destroy(__rte_unused int container_fd)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2162,6 +2170,7 @@ int
>  rte_vfio_container_group_bind(__rte_unused int container_fd,
>  		__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2169,6 +2178,7 @@ int
>  rte_vfio_container_group_unbind(__rte_unused int container_fd,
>  		__rte_unused int iommu_group_num)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2178,6 +2188,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
>  		__rte_unused uint64_t iova,
>  		__rte_unused uint64_t len)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> @@ -2187,6 +2198,7 @@ rte_vfio_container_dma_unmap(__rte_unused int
> container_fd,
>  		__rte_unused uint64_t iova,
>  		__rte_unused uint64_t len)
>  {
> +	rte_errno = ENOTSUP;
>  	return -1;
>  }
> 
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-09-22  3:30 ` [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Xia, Chenbo
@ 2021-09-22  9:26   ` Ferruh Yigit
  2021-09-22 11:30     ` Xia, Chenbo
  2021-10-28 10:29   ` Burakov, Anatoly
  1 sibling, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2021-09-22  9:26 UTC (permalink / raw)
  To: Xia, Chenbo, Burakov, Anatoly, dev

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
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-09-22  9:26   ` Ferruh Yigit
@ 2021-09-22 11:30     ` Xia, Chenbo
  2021-10-28  8:21       ` David Marchand
  0 siblings, 1 reply; 30+ messages in thread
From: Xia, Chenbo @ 2021-09-22 11:30 UTC (permalink / raw)
  To: Yigit, Ferruh, Burakov, Anatoly, dev

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-09-22 11:30     ` Xia, Chenbo
@ 2021-10-28  8:21       ` David Marchand
  0 siblings, 0 replies; 30+ messages in thread
From: David Marchand @ 2021-10-28  8:21 UTC (permalink / raw)
  To: Xia, Chenbo, Burakov, Anatoly; +Cc: Yigit, Ferruh, dev

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.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-09-22  3:30 ` [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Xia, Chenbo
  2021-09-22  9:26   ` Ferruh Yigit
@ 2021-10-28 10:29   ` Burakov, Anatoly
  2021-10-28 11:11     ` Xia, Chenbo
  1 sibling, 1 reply; 30+ messages in thread
From: Burakov, Anatoly @ 2021-10-28 10:29 UTC (permalink / raw)
  To: Xia, Chenbo, dev

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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 10:29   ` Burakov, Anatoly
@ 2021-10-28 11:11     ` Xia, Chenbo
  2021-10-28 11:32       ` Ferruh Yigit
  0 siblings, 1 reply; 30+ messages in thread
From: Xia, Chenbo @ 2021-10-28 11:11 UTC (permalink / raw)
  To: Burakov, Anatoly, dev, Yigit, Ferruh, David Marchand, Thomas Monjalon

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 11:11     ` Xia, Chenbo
@ 2021-10-28 11:32       ` Ferruh Yigit
  2021-10-28 13:00         ` Burakov, Anatoly
  2021-10-28 14:53         ` Thomas Monjalon
  0 siblings, 2 replies; 30+ messages in thread
From: Ferruh Yigit @ 2021-10-28 11:32 UTC (permalink / raw)
  To: Xia, Chenbo, Burakov, Anatoly, dev, David Marchand, Thomas Monjalon

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 11:32       ` Ferruh Yigit
@ 2021-10-28 13:00         ` Burakov, Anatoly
  2021-10-28 14:53         ` Thomas Monjalon
  1 sibling, 0 replies; 30+ messages in thread
From: Burakov, Anatoly @ 2021-10-28 13:00 UTC (permalink / raw)
  To: Ferruh Yigit, Xia, Chenbo, dev, David Marchand, Thomas Monjalon

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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation
  2021-09-16 10:36 [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Anatoly Burakov
  2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
  2021-09-22  3:30 ` [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Xia, Chenbo
@ 2021-10-28 13:49 ` Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
                     ` (3 more replies)
  2 siblings, 4 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 13:49 UTC (permalink / raw)
  To: dev

Currently, VFIO support for Linux is compiled unconditionally, and
supported kernel versions start with 4.4, so VFIO is assumed to always
be enabled. There is no way of disabling VFIO support at compile time
anyway, so just drop the "VFIO not available" fallback code altogether.

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 | 110 ---------------------------------------
 1 file changed, 110 deletions(-)

diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index aa2087a2da..549b86ae1d 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -20,8 +20,6 @@
 #include "eal_private.h"
 #include "eal_internal_cfg.h"
 
-#ifdef VFIO_PRESENT
-
 #define VFIO_MEM_EVENT_CLB_NAME "vfio_mem_event_clb"
 
 /* hot plug/unplug of VFIO groups may cause all DMA maps to be dropped. we can
@@ -2201,111 +2199,3 @@ rte_vfio_container_dma_unmap(int container_fd, uint64_t vaddr, uint64_t iova,
 
 	return container_dma_unmap(vfio_cfg, vaddr, iova, len);
 }
-
-#else
-
-int
-rte_vfio_setup_device(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr,
-		__rte_unused int *vfio_dev_fd,
-		__rte_unused struct vfio_device_info *device_info)
-{
-	return -1;
-}
-
-int
-rte_vfio_release_device(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr, __rte_unused int fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_enable(__rte_unused const char *modname)
-{
-	return -1;
-}
-
-int
-rte_vfio_is_enabled(__rte_unused const char *modname)
-{
-	return -1;
-}
-
-int
-rte_vfio_noiommu_is_enabled(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_clear_group(__rte_unused int vfio_group_fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr,
-		__rte_unused int *iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_container_fd(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_create(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_destroy(__rte_unused int container_fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_group_bind(__rte_unused int container_fd,
-		__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_group_unbind(__rte_unused int container_fd,
-		__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_dma_map(__rte_unused int container_fd,
-		__rte_unused uint64_t vaddr,
-		__rte_unused uint64_t iova,
-		__rte_unused uint64_t len)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_dma_unmap(__rte_unused int container_fd,
-		__rte_unused uint64_t vaddr,
-		__rte_unused uint64_t iova,
-		__rte_unused uint64_t len)
-{
-	return -1;
-}
-
-#endif /* VFIO_PRESENT */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v2 2/4] vfio: fix wrong return value for FreeBSD
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
@ 2021-10-28 13:49   ` Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 13:49 UTC (permalink / raw)
  To: dev, Bruce Richardson; +Cc: stable

On FreeBSD, `rte_vfio_clear_group()` was returning 0 even though this
function is not valid for FreeBSD, and is called out to return error in
doxygen comments. Fix the return value to match documentation.

Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/freebsd/eal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index 9935356ed4..dada210b19 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -1032,7 +1032,7 @@ int rte_vfio_noiommu_is_enabled(void)
 
 int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
 {
-	return 0;
+	return -1;
 }
 
 int
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v2 3/4] vfio: fix documentation to match intended behavior
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
@ 2021-10-28 13:49   ` Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  3 siblings, 0 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 13:49 UTC (permalink / raw)
  To: dev; +Cc: stable

On FreeBSD, `rte_vfio_is_enabled()` and `rte_vfio_noiommu_is_enabled()`
API calls will not return error, and will instead return 0. This is
intentional, because the caller of this API does not care whether VFIO
is supported at all, and will instead be interested in whether VFIO is
enabled or not. However, the doxygen comments for these functions state
that they will return an error on FreeBSD, which is incorrect.

Fix the doxygen comment to call out the fact that these
functions are only relevant on Linux, but remove the reference to
returning errors.

Cc: stable@dpdk.org
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/include/rte_vfio.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/eal/include/rte_vfio.h b/lib/eal/include/rte_vfio.h
index 2d90b36480..7bdb8932b2 100644
--- a/lib/eal/include/rte_vfio.h
+++ b/lib/eal/include/rte_vfio.h
@@ -150,8 +150,7 @@ int rte_vfio_enable(const char *modname);
 /**
  * Check whether a VFIO-related kmod is enabled.
  *
- * This function is only relevant to linux and will return
- * an error on BSD.
+ * This function is only relevant to Linux.
  *
  * @param modname
  *   kernel module name.
@@ -165,8 +164,7 @@ int rte_vfio_is_enabled(const char *modname);
 /**
  * Whether VFIO NOIOMMU mode is enabled.
  *
- * This function is only relevant to linux and will return
- * an error on BSD.
+ * This function is only relevant to Linux.
  *
  * @return
  *   1 if true.
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
@ 2021-10-28 13:49   ` Anatoly Burakov
  2021-10-28 14:13     ` Burakov, Anatoly
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  3 siblings, 1 reply; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 13:49 UTC (permalink / raw)
  To: dev, Bruce Richardson; +Cc: Chenbo Xia

Currently, when code is running on FreeBSD, there is no way to
distinguish between a geniune error and a "VFIO is unsupported" error.
Fix the dummy implementations to also set the rte_errno flag.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Chenbo Xia <chenbo.xia@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/eal/freebsd/eal.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index dada210b19..fce13accc8 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -1005,6 +1005,7 @@ int rte_vfio_setup_device(__rte_unused const char *sysfs_base,
 		      __rte_unused int *vfio_dev_fd,
 		      __rte_unused struct vfio_device_info *device_info)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1012,11 +1013,13 @@ int rte_vfio_release_device(__rte_unused const char *sysfs_base,
 			__rte_unused const char *dev_addr,
 			__rte_unused int fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int rte_vfio_enable(__rte_unused const char *modname)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1032,6 +1035,7 @@ int rte_vfio_noiommu_is_enabled(void)
 
 int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1040,30 +1044,35 @@ rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
 		       __rte_unused const char *dev_addr,
 		       __rte_unused int *iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_container_fd(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_create(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_destroy(__rte_unused int container_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1071,6 +1080,7 @@ int
 rte_vfio_container_group_bind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1078,6 +1088,7 @@ int
 rte_vfio_container_group_unbind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1087,6 +1098,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1096,5 +1108,6 @@ rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-10-28 14:13     ` Burakov, Anatoly
  0 siblings, 0 replies; 30+ messages in thread
From: Burakov, Anatoly @ 2021-10-28 14:13 UTC (permalink / raw)
  To: dev, Bruce Richardson; +Cc: Chenbo Xia

On 28-Oct-21 2:49 PM, Anatoly Burakov wrote:
> Currently, when code is running on FreeBSD, there is no way to
> distinguish between a geniune error and a "VFIO is unsupported" error.
> Fix the dummy implementations to also set the rte_errno flag.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Chenbo Xia <chenbo.xia@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   lib/eal/freebsd/eal.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 

Missed two functions on Windows.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
  2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
                     ` (2 preceding siblings ...)
  2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-10-28 14:15   ` Anatoly Burakov
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
                       ` (3 more replies)
  3 siblings, 4 replies; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 14:15 UTC (permalink / raw)
  To: dev

Currently, VFIO support for Linux is compiled unconditionally, and
supported kernel versions start with 4.4, so VFIO is assumed to always
be enabled. There is no way of disabling VFIO support at compile time
anyway, so just drop the "VFIO not available" fallback code altogether.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/linux/eal_vfio.c | 110 ---------------------------------------
 1 file changed, 110 deletions(-)

diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index aa2087a2da..549b86ae1d 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -20,8 +20,6 @@
 #include "eal_private.h"
 #include "eal_internal_cfg.h"
 
-#ifdef VFIO_PRESENT
-
 #define VFIO_MEM_EVENT_CLB_NAME "vfio_mem_event_clb"
 
 /* hot plug/unplug of VFIO groups may cause all DMA maps to be dropped. we can
@@ -2201,111 +2199,3 @@ rte_vfio_container_dma_unmap(int container_fd, uint64_t vaddr, uint64_t iova,
 
 	return container_dma_unmap(vfio_cfg, vaddr, iova, len);
 }
-
-#else
-
-int
-rte_vfio_setup_device(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr,
-		__rte_unused int *vfio_dev_fd,
-		__rte_unused struct vfio_device_info *device_info)
-{
-	return -1;
-}
-
-int
-rte_vfio_release_device(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr, __rte_unused int fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_enable(__rte_unused const char *modname)
-{
-	return -1;
-}
-
-int
-rte_vfio_is_enabled(__rte_unused const char *modname)
-{
-	return -1;
-}
-
-int
-rte_vfio_noiommu_is_enabled(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_clear_group(__rte_unused int vfio_group_fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
-		__rte_unused const char *dev_addr,
-		__rte_unused int *iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_container_fd(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_create(void)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_destroy(__rte_unused int container_fd)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_group_bind(__rte_unused int container_fd,
-		__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_group_unbind(__rte_unused int container_fd,
-		__rte_unused int iommu_group_num)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_dma_map(__rte_unused int container_fd,
-		__rte_unused uint64_t vaddr,
-		__rte_unused uint64_t iova,
-		__rte_unused uint64_t len)
-{
-	return -1;
-}
-
-int
-rte_vfio_container_dma_unmap(__rte_unused int container_fd,
-		__rte_unused uint64_t vaddr,
-		__rte_unused uint64_t iova,
-		__rte_unused uint64_t len)
-{
-	return -1;
-}
-
-#endif /* VFIO_PRESENT */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
@ 2021-10-28 14:15     ` Anatoly Burakov
  2021-11-05  2:50       ` Xia, Chenbo
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 14:15 UTC (permalink / raw)
  To: dev, Bruce Richardson; +Cc: stable

On FreeBSD, `rte_vfio_clear_group()` was returning 0 even though this
function is not valid for FreeBSD, and is called out to return error in
doxygen comments. Fix the return value to match documentation.

Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/freebsd/eal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index 9935356ed4..dada210b19 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -1032,7 +1032,7 @@ int rte_vfio_noiommu_is_enabled(void)
 
 int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
 {
-	return 0;
+	return -1;
 }
 
 int
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v3 3/4] vfio: fix documentation to match intended behavior
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
@ 2021-10-28 14:15     ` Anatoly Burakov
  2021-11-05  2:57       ` [dpdk-dev] [dpdk-stable] " Xia, Chenbo
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
  2021-11-01  6:27     ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Xia, Chenbo
  3 siblings, 1 reply; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 14:15 UTC (permalink / raw)
  To: dev; +Cc: stable

On FreeBSD, `rte_vfio_is_enabled()` and `rte_vfio_noiommu_is_enabled()`
API calls will not return error, and will instead return 0. This is
intentional, because the caller of this API does not care whether VFIO
is supported at all, and will instead be interested in whether VFIO is
enabled or not. However, the doxygen comments for these functions state
that they will return an error on FreeBSD, which is incorrect.

Fix the doxygen comment to call out the fact that these
functions are only relevant on Linux, but remove the reference to
returning errors.

Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/eal/include/rte_vfio.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/eal/include/rte_vfio.h b/lib/eal/include/rte_vfio.h
index 2d90b36480..7bdb8932b2 100644
--- a/lib/eal/include/rte_vfio.h
+++ b/lib/eal/include/rte_vfio.h
@@ -150,8 +150,7 @@ int rte_vfio_enable(const char *modname);
 /**
  * Check whether a VFIO-related kmod is enabled.
  *
- * This function is only relevant to linux and will return
- * an error on BSD.
+ * This function is only relevant to Linux.
  *
  * @param modname
  *   kernel module name.
@@ -165,8 +164,7 @@ int rte_vfio_is_enabled(const char *modname);
 /**
  * Whether VFIO NOIOMMU mode is enabled.
  *
- * This function is only relevant to linux and will return
- * an error on BSD.
+ * This function is only relevant to Linux.
  *
  * @return
  *   1 if true.
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH v3 4/4] vfio: add errno on unsupported platforms
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
@ 2021-10-28 14:15     ` Anatoly Burakov
  2021-11-08 15:55       ` [dpdk-dev] [dpdk-stable] " David Marchand
  2021-11-01  6:27     ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Xia, Chenbo
  3 siblings, 1 reply; 30+ messages in thread
From: Anatoly Burakov @ 2021-10-28 14:15 UTC (permalink / raw)
  To: dev, Bruce Richardson, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam
  Cc: stable, Chenbo Xia

Currently, when code is running on FreeBSD or Windows,, there is no way
to distinguish between a geniune error and a "VFIO is unsupported"
error. Fix the dummy implementations to also set the rte_errno flag.

Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Chenbo Xia <chenbo.xia@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/eal/freebsd/eal.c | 13 +++++++++++++
 lib/eal/windows/eal.c |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index dada210b19..fce13accc8 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -1005,6 +1005,7 @@ int rte_vfio_setup_device(__rte_unused const char *sysfs_base,
 		      __rte_unused int *vfio_dev_fd,
 		      __rte_unused struct vfio_device_info *device_info)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1012,11 +1013,13 @@ int rte_vfio_release_device(__rte_unused const char *sysfs_base,
 			__rte_unused const char *dev_addr,
 			__rte_unused int fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int rte_vfio_enable(__rte_unused const char *modname)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1032,6 +1035,7 @@ int rte_vfio_noiommu_is_enabled(void)
 
 int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1040,30 +1044,35 @@ rte_vfio_get_group_num(__rte_unused const char *sysfs_base,
 		       __rte_unused const char *dev_addr,
 		       __rte_unused int *iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_container_fd(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_get_group_fd(__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_create(void)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
 int
 rte_vfio_container_destroy(__rte_unused int container_fd)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1071,6 +1080,7 @@ int
 rte_vfio_container_group_bind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1078,6 +1088,7 @@ int
 rte_vfio_container_group_unbind(__rte_unused int container_fd,
 		__rte_unused int iommu_group_num)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1087,6 +1098,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -1096,5 +1108,6 @@ rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index f7ce1b6671..67db7f099a 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -481,6 +481,7 @@ rte_vfio_container_dma_map(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
@@ -490,6 +491,7 @@ rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 			__rte_unused uint64_t iova,
 			__rte_unused uint64_t len)
 {
+	rte_errno = ENOTSUP;
 	return -1;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 11:32       ` Ferruh Yigit
  2021-10-28 13:00         ` Burakov, Anatoly
@ 2021-10-28 14:53         ` Thomas Monjalon
  2021-10-28 15:40           ` Burakov, Anatoly
  1 sibling, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2021-10-28 14:53 UTC (permalink / raw)
  To: Xia, Chenbo, Burakov, Anatoly, David Marchand, Ferruh Yigit; +Cc: dev

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.



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 14:53         ` Thomas Monjalon
@ 2021-10-28 15:40           ` Burakov, Anatoly
  2021-10-28 16:38             ` Stephen Hemminger
  0 siblings, 1 reply; 30+ messages in thread
From: Burakov, Anatoly @ 2021-10-28 15:40 UTC (permalink / raw)
  To: Thomas Monjalon, Xia, Chenbo, David Marchand, Ferruh Yigit; +Cc: dev

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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent
  2021-10-28 15:40           ` Burakov, Anatoly
@ 2021-10-28 16:38             ` Stephen Hemminger
  0 siblings, 0 replies; 30+ messages in thread
From: Stephen Hemminger @ 2021-10-28 16:38 UTC (permalink / raw)
  To: Burakov, Anatoly
  Cc: Thomas Monjalon, Xia, Chenbo, David Marchand, Ferruh Yigit, dev

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.




^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
  2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
                       ` (2 preceding siblings ...)
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-11-01  6:27     ` Xia, Chenbo
  2021-11-01 13:57       ` Burakov, Anatoly
  3 siblings, 1 reply; 30+ messages in thread
From: Xia, Chenbo @ 2021-11-01  6:27 UTC (permalink / raw)
  To: Burakov, Anatoly, dev

Hi Anatoly,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: Thursday, October 28, 2021 10:15 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
> 
> Currently, VFIO support for Linux is compiled unconditionally, and
> supported kernel versions start with 4.4, so VFIO is assumed to always
> be enabled. There is no way of disabling VFIO support at compile time
> anyway, so just drop the "VFIO not available" fallback code altogether.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/eal/linux/eal_vfio.c | 110 ---------------------------------------
>  1 file changed, 110 deletions(-)
> 

If we make the 'kernel >= 4.4' assumption, should other 'VFIO_PRESENT' in eal
and drivers be deleted as well? It seems strange to me as eal_vfio already
assumes it supports vfio but drivers and other eal components still have the
check.

Thanks,
Chenbo

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
  2021-11-01  6:27     ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Xia, Chenbo
@ 2021-11-01 13:57       ` Burakov, Anatoly
  2021-11-05  2:48         ` Xia, Chenbo
  0 siblings, 1 reply; 30+ messages in thread
From: Burakov, Anatoly @ 2021-11-01 13:57 UTC (permalink / raw)
  To: Xia, Chenbo, dev

On 01-Nov-21 6:27 AM, Xia, Chenbo wrote:
> Hi Anatoly,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
>> Sent: Thursday, October 28, 2021 10:15 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
>>
>> Currently, VFIO support for Linux is compiled unconditionally, and
>> supported kernel versions start with 4.4, so VFIO is assumed to always
>> be enabled. There is no way of disabling VFIO support at compile time
>> anyway, so just drop the "VFIO not available" fallback code altogether.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/eal/linux/eal_vfio.c | 110 ---------------------------------------
>>   1 file changed, 110 deletions(-)
>>
> 
> If we make the 'kernel >= 4.4' assumption, should other 'VFIO_PRESENT' in eal
> and drivers be deleted as well? It seems strange to me as eal_vfio already
> assumes it supports vfio but drivers and other eal components still have the
> check.

We still need those checks because those drivers are also compiled on 
FreeBSD etc. - so we're not removing the VFIO check, we're just removing 
the fallback implementation for Linux that was, at one point, possible, 
but now isn't.

> 
> Thanks,
> Chenbo
> 


-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
  2021-11-01 13:57       ` Burakov, Anatoly
@ 2021-11-05  2:48         ` Xia, Chenbo
  0 siblings, 0 replies; 30+ messages in thread
From: Xia, Chenbo @ 2021-11-05  2:48 UTC (permalink / raw)
  To: Burakov, Anatoly, dev

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, November 1, 2021 9:57 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux
> implementation
> 
> On 01-Nov-21 6:27 AM, Xia, Chenbo wrote:
> > Hi Anatoly,
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> >> Sent: Thursday, October 28, 2021 10:15 PM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation
> >>
> >> Currently, VFIO support for Linux is compiled unconditionally, and
> >> supported kernel versions start with 4.4, so VFIO is assumed to always
> >> be enabled. There is no way of disabling VFIO support at compile time
> >> anyway, so just drop the "VFIO not available" fallback code altogether.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> ---
> >>   lib/eal/linux/eal_vfio.c | 110 ---------------------------------------
> >>   1 file changed, 110 deletions(-)
> >>
> >
> > If we make the 'kernel >= 4.4' assumption, should other 'VFIO_PRESENT' in
> eal
> > and drivers be deleted as well? It seems strange to me as eal_vfio already
> > assumes it supports vfio but drivers and other eal components still have the
> > check.
> 
> We still need those checks because those drivers are also compiled on
> FreeBSD etc. - so we're not removing the VFIO check, we're just removing
> the fallback implementation for Linux that was, at one point, possible,
> but now isn't.

Make sense to me.

Acked-by: Chenbo Xia <chenbo.xia@intel.com>

> 
> >
> > Thanks,
> > Chenbo
> >
> 
> 
> --
> Thanks,
> Anatoly

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
@ 2021-11-05  2:50       ` Xia, Chenbo
  0 siblings, 0 replies; 30+ messages in thread
From: Xia, Chenbo @ 2021-11-05  2:50 UTC (permalink / raw)
  To: Burakov, Anatoly, dev, Richardson, Bruce; +Cc: stable

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: Thursday, October 28, 2021 10:15 PM
> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD
> 
> On FreeBSD, `rte_vfio_clear_group()` was returning 0 even though this
> function is not valid for FreeBSD, and is called out to return error in
> doxygen comments. Fix the return value to match documentation.
> 

Fix tag?

/Chenbo

> Cc: stable@dpdk.org
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/eal/freebsd/eal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index 9935356ed4..dada210b19 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -1032,7 +1032,7 @@ int rte_vfio_noiommu_is_enabled(void)
> 
>  int rte_vfio_clear_group(__rte_unused int vfio_group_fd)
>  {
> -	return 0;
> +	return -1;
>  }
> 
>  int
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/4] vfio: fix documentation to match intended behavior
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
@ 2021-11-05  2:57       ` Xia, Chenbo
  0 siblings, 0 replies; 30+ messages in thread
From: Xia, Chenbo @ 2021-11-05  2:57 UTC (permalink / raw)
  To: Burakov, Anatoly, dev; +Cc: stable

> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: Thursday, October 28, 2021 10:15 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: [dpdk-stable] [PATCH v3 3/4] vfio: fix documentation to match
> intended behavior
> 
> On FreeBSD, `rte_vfio_is_enabled()` and `rte_vfio_noiommu_is_enabled()`
> API calls will not return error, and will instead return 0. This is
> intentional, because the caller of this API does not care whether VFIO
> is supported at all, and will instead be interested in whether VFIO is
> enabled or not. However, the doxygen comments for these functions state
> that they will return an error on FreeBSD, which is incorrect.
> 
> Fix the doxygen comment to call out the fact that these
> functions are only relevant on Linux, but remove the reference to
> returning errors.
> 

Fix tag?

With above fixed:

Acked-by: Chenbo Xia <chenbo.xia@intel.com>

> Cc: stable@dpdk.org
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/eal/include/rte_vfio.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/eal/include/rte_vfio.h b/lib/eal/include/rte_vfio.h
> index 2d90b36480..7bdb8932b2 100644
> --- a/lib/eal/include/rte_vfio.h
> +++ b/lib/eal/include/rte_vfio.h
> @@ -150,8 +150,7 @@ int rte_vfio_enable(const char *modname);
>  /**
>   * Check whether a VFIO-related kmod is enabled.
>   *
> - * This function is only relevant to linux and will return
> - * an error on BSD.
> + * This function is only relevant to Linux.
>   *
>   * @param modname
>   *   kernel module name.
> @@ -165,8 +164,7 @@ int rte_vfio_is_enabled(const char *modname);
>  /**
>   * Whether VFIO NOIOMMU mode is enabled.
>   *
> - * This function is only relevant to linux and will return
> - * an error on BSD.
> + * This function is only relevant to Linux.
>   *
>   * @return
>   *   1 if true.
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v3 4/4] vfio: add errno on unsupported platforms
  2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
@ 2021-11-08 15:55       ` David Marchand
  0 siblings, 0 replies; 30+ messages in thread
From: David Marchand @ 2021-11-08 15:55 UTC (permalink / raw)
  To: Anatoly Burakov
  Cc: dev, Bruce Richardson, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam, dpdk stable, Chenbo Xia

On Thu, Oct 28, 2021 at 4:15 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> Currently, when code is running on FreeBSD or Windows,, there is no way
> to distinguish between a geniune error and a "VFIO is unsupported"
> error. Fix the dummy implementations to also set the rte_errno flag.
>
> Cc: stable@dpdk.org

> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Chenbo Xia <chenbo.xia@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Added (what I hope are correct) Fixes: line in patches 2, 3 and 4.
Series applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-11-08 15:55 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 10:36 [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Anatoly Burakov
2021-09-16 10:36 ` [dpdk-dev] [PATCH v1 2/2] vfio: add errno on unsupported platforms Anatoly Burakov
2021-09-16 10:39   ` Bruce Richardson
2021-09-22  3:31   ` Xia, Chenbo
2021-09-22  3:30 ` [dpdk-dev] [PATCH v1 1/2] vfio: make API return values consistent Xia, Chenbo
2021-09-22  9:26   ` Ferruh Yigit
2021-09-22 11:30     ` Xia, Chenbo
2021-10-28  8:21       ` David Marchand
2021-10-28 10:29   ` Burakov, Anatoly
2021-10-28 11:11     ` Xia, Chenbo
2021-10-28 11:32       ` Ferruh Yigit
2021-10-28 13:00         ` Burakov, Anatoly
2021-10-28 14:53         ` Thomas Monjalon
2021-10-28 15:40           ` Burakov, Anatoly
2021-10-28 16:38             ` Stephen Hemminger
2021-10-28 13:49 ` [dpdk-dev] [PATCH v2 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
2021-10-28 13:49   ` [dpdk-dev] [PATCH v2 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
2021-10-28 14:13     ` Burakov, Anatoly
2021-10-28 14:15   ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Anatoly Burakov
2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 2/4] vfio: fix wrong return value for FreeBSD Anatoly Burakov
2021-11-05  2:50       ` Xia, Chenbo
2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 3/4] vfio: fix documentation to match intended behavior Anatoly Burakov
2021-11-05  2:57       ` [dpdk-dev] [dpdk-stable] " Xia, Chenbo
2021-10-28 14:15     ` [dpdk-dev] [PATCH v3 4/4] vfio: add errno on unsupported platforms Anatoly Burakov
2021-11-08 15:55       ` [dpdk-dev] [dpdk-stable] " David Marchand
2021-11-01  6:27     ` [dpdk-dev] [PATCH v3 1/4] vfio: drop fallback Linux implementation Xia, Chenbo
2021-11-01 13:57       ` Burakov, Anatoly
2021-11-05  2:48         ` Xia, Chenbo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).