[PATCH] bus/cdx: provide driver flag for optional resource mapping

[PATCH] bus/cdx: provide driver flag for optional resource mapping

[Abhijit Gangurde]

Provide driver flag which gives an option to map the cdx
device resource before probing the device driver.
Also, make rte_cdx_map_device() API as public to map
device resource separately.

Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
---
 drivers/bus/cdx/bus_cdx_driver.h | 26 ++---------------
 drivers/bus/cdx/cdx.c            | 11 ++++---
 drivers/bus/cdx/rte_bus_cdx.h    | 50 ++++++++++++++++++++++++++++++++
 drivers/bus/cdx/version.map      | 11 +++++--
 4 files changed, 69 insertions(+), 29 deletions(-)
 create mode 100644 drivers/bus/cdx/rte_bus_cdx.h

diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
index fcacdb5896..1571131417 100644
--- a/drivers/bus/cdx/bus_cdx_driver.h
+++ b/drivers/bus/cdx/bus_cdx_driver.h
@@ -37,6 +37,9 @@ struct rte_cdx_bus;
 static const char DRV_EXP_TAG(name, cdx_tbl_export)[] __rte_used = \
 RTE_STR(table)
 
+/** Device needs resource mapping */
+#define RTE_CDX_DRV_NEED_MAPPING 0x0001
+
 /**
  * A structure describing an ID for a CDX driver. Each driver provides a
  * table of these IDs for each device that it supports.
@@ -107,29 +110,6 @@ struct rte_cdx_driver {
 	uint32_t drv_flags;			/**< Flags RTE_CDX_DRV_*. */
 };
 
-/**
- * Map the CDX device resources in user space virtual memory address.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- *
- * @return
- *   0 on success, <0 on error.
- */
-__rte_internal
-int rte_cdx_map_device(struct rte_cdx_device *dev);
-
-/**
- * Unmap this device.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- */
-__rte_internal
-void rte_cdx_unmap_device(struct rte_cdx_device *dev);
-
 /**
  * Register a CDX driver.
  *
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index 28bbf92ed5..47dc4eabe7 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -74,6 +74,7 @@
 #include <rte_kvargs.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_bus_cdx.h>
 
 #include <eal_filesystem.h>
 
@@ -380,10 +381,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
 	CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
 		dr->driver.name);
 
-	ret = cdx_vfio_map_resource(dev);
-	if (ret != 0) {
-		CDX_BUS_ERR("CDX map device failed: %d", ret);
-		goto error_map_device;
+	if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
+		ret = cdx_vfio_map_resource(dev);
+		if (ret != 0) {
+			CDX_BUS_ERR("CDX map device failed: %d", ret);
+			goto error_map_device;
+		}
 	}
 
 	/* call the driver probe() function */
diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
new file mode 100644
index 0000000000..b9280b5f7f
--- /dev/null
+++ b/drivers/bus/cdx/rte_bus_cdx.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (C) 2023, Advanced Micro Devices, Inc.
+ */
+
+#ifndef _RTE_BUS_CDX_H_
+#define _RTE_BUS_CDX_H_
+
+/**
+ * @file
+ * CDX device & driver interface
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Forward declarations */
+struct rte_cdx_device;
+
+/**
+ * Map the CDX device resources in user space virtual memory address
+ *
+ * Note that driver should not call this function when flag
+ * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
+ * you when it's on.
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use
+ *
+ * @return
+ *   0 on success, negative on error and positive if no driver
+ *   is found for the device.
+ */
+int rte_cdx_map_device(struct rte_cdx_device *dev);
+
+/**
+ * Unmap this device
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use
+ */
+void rte_cdx_unmap_device(struct rte_cdx_device *dev);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BUS_CDX_H_ */
diff --git a/drivers/bus/cdx/version.map b/drivers/bus/cdx/version.map
index 0a15d39ae8..cc7b1f821b 100644
--- a/drivers/bus/cdx/version.map
+++ b/drivers/bus/cdx/version.map
@@ -1,9 +1,16 @@
-INTERNAL {
+DPDK_23 {
 	global:
 
 	rte_cdx_map_device;
-	rte_cdx_register;
 	rte_cdx_unmap_device;
+
+	local: *;
+};
+
+INTERNAL {
+	global:
+
+	rte_cdx_register;
 	rte_cdx_unregister;
 	rte_cdx_vfio_intr_disable;
 	rte_cdx_vfio_intr_enable;
-- 
2.25.1

Re: [PATCH] bus/cdx: provide driver flag for optional resource mapping

[Gupta, Nipun]

On 6/26/2023 4:09 PM, Abhijit Gangurde wrote:
> Provide driver flag which gives an option to map the cdx
> device resource before probing the device driver.
> Also, make rte_cdx_map_device() API as public to map
> device resource separately.
> 
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> ---
>   drivers/bus/cdx/bus_cdx_driver.h | 26 ++---------------
>   drivers/bus/cdx/cdx.c            | 11 ++++---
>   drivers/bus/cdx/rte_bus_cdx.h    | 50 ++++++++++++++++++++++++++++++++
>   drivers/bus/cdx/version.map      | 11 +++++--
>   4 files changed, 69 insertions(+), 29 deletions(-)
>   create mode 100644 drivers/bus/cdx/rte_bus_cdx.h
> 
> diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
> index fcacdb5896..1571131417 100644
> --- a/drivers/bus/cdx/bus_cdx_driver.h
> +++ b/drivers/bus/cdx/bus_cdx_driver.h
> @@ -37,6 +37,9 @@ struct rte_cdx_bus;
>   static const char DRV_EXP_TAG(name, cdx_tbl_export)[] __rte_used = \
>   RTE_STR(table)
>   
> +/** Device needs resource mapping */
> +#define RTE_CDX_DRV_NEED_MAPPING 0x0001
> +
>   /**
>    * A structure describing an ID for a CDX driver. Each driver provides a
>    * table of these IDs for each device that it supports.
> @@ -107,29 +110,6 @@ struct rte_cdx_driver {
>   	uint32_t drv_flags;			/**< Flags RTE_CDX_DRV_*. */
>   };
>   
> -/**
> - * Map the CDX device resources in user space virtual memory address.
> - *
> - * @param dev
> - *   A pointer to a rte_cdx_device structure describing the device
> - *   to use.
> - *
> - * @return
> - *   0 on success, <0 on error.
> - */
> -__rte_internal
> -int rte_cdx_map_device(struct rte_cdx_device *dev);
> -
> -/**
> - * Unmap this device.
> - *
> - * @param dev
> - *   A pointer to a rte_cdx_device structure describing the device
> - *   to use.
> - */
> -__rte_internal
> -void rte_cdx_unmap_device(struct rte_cdx_device *dev);
> -
>   /**
>    * Register a CDX driver.
>    *
> diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
> index 28bbf92ed5..47dc4eabe7 100644
> --- a/drivers/bus/cdx/cdx.c
> +++ b/drivers/bus/cdx/cdx.c
> @@ -74,6 +74,7 @@
>   #include <rte_kvargs.h>
>   #include <rte_malloc.h>
>   #include <rte_vfio.h>
> +#include <rte_bus_cdx.h>
>   
>   #include <eal_filesystem.h>
>   
> @@ -380,10 +381,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
>   	CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
>   		dr->driver.name);
>   
> -	ret = cdx_vfio_map_resource(dev);
> -	if (ret != 0) {
> -		CDX_BUS_ERR("CDX map device failed: %d", ret);
> -		goto error_map_device;
> +	if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
> +		ret = cdx_vfio_map_resource(dev);
> +		if (ret != 0) {
> +			CDX_BUS_ERR("CDX map device failed: %d", ret);
> +			goto error_map_device;
> +		}
>   	}
>   
>   	/* call the driver probe() function */
> diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
> new file mode 100644
> index 0000000000..b9280b5f7f
> --- /dev/null
> +++ b/drivers/bus/cdx/rte_bus_cdx.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef _RTE_BUS_CDX_H_
> +#define _RTE_BUS_CDX_H_

nit: there are no underscores before or after in other cdx header files. 
please change to RTE_BUS_CDX_H

> +
> +/**
> + * @file
> + * CDX device & driver interface
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* Forward declarations */
> +struct rte_cdx_device;
> +
> +/**
> + * Map the CDX device resources in user space virtual memory address

nit: all doxygen comments should end with a dot.

> + *
> + * Note that driver should not call this function when flag
> + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> + * you when it's on.
> + *
> + * @param dev
> + *   A pointer to a rte_cdx_device structure describing the device
> + *   to use
> + *
> + * @return
> + *   0 on success, negative on error and positive if no driver
> + *   is found for the device.
> + */
> +int rte_cdx_map_device(struct rte_cdx_device *dev);
> +
> +/**
> + * Unmap this device
> + *
> + * @param dev
> + *   A pointer to a rte_cdx_device structure describing the device
> + *   to use
> + */
> +void rte_cdx_unmap_device(struct rte_cdx_device *dev);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_BUS_CDX_H_ */
> diff --git a/drivers/bus/cdx/version.map b/drivers/bus/cdx/version.map
> index 0a15d39ae8..cc7b1f821b 100644
> --- a/drivers/bus/cdx/version.map
> +++ b/drivers/bus/cdx/version.map
> @@ -1,9 +1,16 @@
> -INTERNAL {
> +DPDK_23 {
>   	global:
>   
>   	rte_cdx_map_device;
> -	rte_cdx_register;
>   	rte_cdx_unmap_device;
> +
> +	local: *;
> +};
> +
> +INTERNAL {
> +	global:
> +
> +	rte_cdx_register;
>   	rte_cdx_unregister;
>   	rte_cdx_vfio_intr_disable;
>   	rte_cdx_vfio_intr_enable;

[PATCH v2] bus/cdx: provide driver flag for optional resource mapping

[Abhijit Gangurde]

Provide driver flag which gives an option to map the cdx
device resource before probing the device driver.
Also, make rte_cdx_map_device() API as public to map
device resource separately.

Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
---
 v2:
 - Corrected _RTE_BUS_CDX_H_ to RTE_BUS_CDX_H
 - Improved code comments.
 
 drivers/bus/cdx/bus_cdx_driver.h | 26 ++---------------
 drivers/bus/cdx/cdx.c            | 11 ++++---
 drivers/bus/cdx/rte_bus_cdx.h    | 50 ++++++++++++++++++++++++++++++++
 drivers/bus/cdx/version.map      | 11 +++++--
 4 files changed, 69 insertions(+), 29 deletions(-)
 create mode 100644 drivers/bus/cdx/rte_bus_cdx.h

diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
index fcacdb5896..1571131417 100644
--- a/drivers/bus/cdx/bus_cdx_driver.h
+++ b/drivers/bus/cdx/bus_cdx_driver.h
@@ -37,6 +37,9 @@ struct rte_cdx_bus;
 static const char DRV_EXP_TAG(name, cdx_tbl_export)[] __rte_used = \
 RTE_STR(table)
 
+/** Device needs resource mapping */
+#define RTE_CDX_DRV_NEED_MAPPING 0x0001
+
 /**
  * A structure describing an ID for a CDX driver. Each driver provides a
  * table of these IDs for each device that it supports.
@@ -107,29 +110,6 @@ struct rte_cdx_driver {
 	uint32_t drv_flags;			/**< Flags RTE_CDX_DRV_*. */
 };
 
-/**
- * Map the CDX device resources in user space virtual memory address.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- *
- * @return
- *   0 on success, <0 on error.
- */
-__rte_internal
-int rte_cdx_map_device(struct rte_cdx_device *dev);
-
-/**
- * Unmap this device.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- */
-__rte_internal
-void rte_cdx_unmap_device(struct rte_cdx_device *dev);
-
 /**
  * Register a CDX driver.
  *
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index 28bbf92ed5..47dc4eabe7 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -74,6 +74,7 @@
 #include <rte_kvargs.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_bus_cdx.h>
 
 #include <eal_filesystem.h>
 
@@ -380,10 +381,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
 	CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
 		dr->driver.name);
 
-	ret = cdx_vfio_map_resource(dev);
-	if (ret != 0) {
-		CDX_BUS_ERR("CDX map device failed: %d", ret);
-		goto error_map_device;
+	if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
+		ret = cdx_vfio_map_resource(dev);
+		if (ret != 0) {
+			CDX_BUS_ERR("CDX map device failed: %d", ret);
+			goto error_map_device;
+		}
 	}
 
 	/* call the driver probe() function */
diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
new file mode 100644
index 0000000000..80acd668c0
--- /dev/null
+++ b/drivers/bus/cdx/rte_bus_cdx.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (C) 2023, Advanced Micro Devices, Inc.
+ */
+
+#ifndef RTE_BUS_CDX_H
+#define RTE_BUS_CDX_H
+
+/**
+ * @file
+ * CDX device & driver interface
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Forward declarations */
+struct rte_cdx_device;
+
+/**
+ * Map the CDX device resources in user space virtual memory address.
+ *
+ * Note that driver should not call this function when flag
+ * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
+ * you when it's on.
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use.
+ *
+ * @return
+ *   0 on success, negative on error and positive if no driver
+ *   is found for the device.
+ */
+int rte_cdx_map_device(struct rte_cdx_device *dev);
+
+/**
+ * Unmap this device.
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use.
+ */
+void rte_cdx_unmap_device(struct rte_cdx_device *dev);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_BUS_CDX_H */
diff --git a/drivers/bus/cdx/version.map b/drivers/bus/cdx/version.map
index 0a15d39ae8..cc7b1f821b 100644
--- a/drivers/bus/cdx/version.map
+++ b/drivers/bus/cdx/version.map
@@ -1,9 +1,16 @@
-INTERNAL {
+DPDK_23 {
 	global:
 
 	rte_cdx_map_device;
-	rte_cdx_register;
 	rte_cdx_unmap_device;
+
+	local: *;
+};
+
+INTERNAL {
+	global:
+
+	rte_cdx_register;
 	rte_cdx_unregister;
 	rte_cdx_vfio_intr_disable;
 	rte_cdx_vfio_intr_enable;
-- 
2.25.1

RE: [PATCH] bus/cdx: provide driver flag for optional resource mapping

[Gupta, Nipun]

> -----Original Message-----
> From: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Sent: Monday, June 26, 2023 4:10 PM
> To: Gupta, Nipun <Nipun.Gupta@amd.com>; Agarwal, Nikhil
> <nikhil.agarwal@amd.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; Yigit, Ferruh
> <Ferruh.Yigit@amd.com>; Gangurde, Abhijit <abhijit.gangurde@amd.com>
> Subject: [PATCH] bus/cdx: provide driver flag for optional resource mapping
> 
> Provide driver flag which gives an option to map the cdx
> device resource before probing the device driver.
> Also, make rte_cdx_map_device() API as public to map
> device resource separately.
> 
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> ---
>  drivers/bus/cdx/bus_cdx_driver.h | 26 ++---------------
>  drivers/bus/cdx/cdx.c            | 11 ++++---
>  drivers/bus/cdx/rte_bus_cdx.h    | 50 ++++++++++++++++++++++++++++++++
>  drivers/bus/cdx/version.map      | 11 +++++--
>  4 files changed, 69 insertions(+), 29 deletions(-)
>  create mode 100644 drivers/bus/cdx/rte_bus_cdx.h
> 

<snip>

> +/* Forward declarations */
> +struct rte_cdx_device;
> +
> +/**
> + * Map the CDX device resources in user space virtual memory address
> + *
> + * Note that driver should not call this function when flag
> + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> + * you when it's on.
> + *
> + * @param dev
> + *   A pointer to a rte_cdx_device structure describing the device
> + *   to use
> + *
> + * @return
> + *   0 on success, negative on error and positive if no driver
> + *   is found for the device.
> + */
> +int rte_cdx_map_device(struct rte_cdx_device *dev);

rte_experimental?

[PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[Abhijit Gangurde]

Provide driver flag which gives an option to map the cdx
device resource before probing the device driver.
Also, make rte_cdx_map_device() API as public to map
device resource separately.

Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
---
 v3:
 - Changed APIs to __rte_experimental.

 v2:
 - Corrected _RTE_BUS_CDX_H_ to RTE_BUS_CDX_H
 - Improved code comments.

 drivers/bus/cdx/bus_cdx_driver.h | 26 ++--------------
 drivers/bus/cdx/cdx.c            | 11 ++++---
 drivers/bus/cdx/rte_bus_cdx.h    | 52 ++++++++++++++++++++++++++++++++
 drivers/bus/cdx/version.map      | 11 +++++--
 4 files changed, 71 insertions(+), 29 deletions(-)
 create mode 100644 drivers/bus/cdx/rte_bus_cdx.h

diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
index fcacdb5896..1571131417 100644
--- a/drivers/bus/cdx/bus_cdx_driver.h
+++ b/drivers/bus/cdx/bus_cdx_driver.h
@@ -37,6 +37,9 @@ struct rte_cdx_bus;
 static const char DRV_EXP_TAG(name, cdx_tbl_export)[] __rte_used = \
 RTE_STR(table)
 
+/** Device needs resource mapping */
+#define RTE_CDX_DRV_NEED_MAPPING 0x0001
+
 /**
  * A structure describing an ID for a CDX driver. Each driver provides a
  * table of these IDs for each device that it supports.
@@ -107,29 +110,6 @@ struct rte_cdx_driver {
 	uint32_t drv_flags;			/**< Flags RTE_CDX_DRV_*. */
 };
 
-/**
- * Map the CDX device resources in user space virtual memory address.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- *
- * @return
- *   0 on success, <0 on error.
- */
-__rte_internal
-int rte_cdx_map_device(struct rte_cdx_device *dev);
-
-/**
- * Unmap this device.
- *
- * @param dev
- *   A pointer to a rte_cdx_device structure describing the device
- *   to use.
- */
-__rte_internal
-void rte_cdx_unmap_device(struct rte_cdx_device *dev);
-
 /**
  * Register a CDX driver.
  *
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index f9526e08cc..fc8d3b668b 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -74,6 +74,7 @@
 #include <rte_kvargs.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_bus_cdx.h>
 
 #include <eal_filesystem.h>
 
@@ -383,10 +384,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
 	CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
 		dr->driver.name);
 
-	ret = cdx_vfio_map_resource(dev);
-	if (ret != 0) {
-		CDX_BUS_ERR("CDX map device failed: %d", ret);
-		goto error_map_device;
+	if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
+		ret = cdx_vfio_map_resource(dev);
+		if (ret != 0) {
+			CDX_BUS_ERR("CDX map device failed: %d", ret);
+			goto error_map_device;
+		}
 	}
 
 	/* call the driver probe() function */
diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
new file mode 100644
index 0000000000..4ca12f90c4
--- /dev/null
+++ b/drivers/bus/cdx/rte_bus_cdx.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (C) 2023, Advanced Micro Devices, Inc.
+ */
+
+#ifndef RTE_BUS_CDX_H
+#define RTE_BUS_CDX_H
+
+/**
+ * @file
+ * CDX device & driver interface
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Forward declarations */
+struct rte_cdx_device;
+
+/**
+ * Map the CDX device resources in user space virtual memory address.
+ *
+ * Note that driver should not call this function when flag
+ * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
+ * you when it's on.
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use.
+ *
+ * @return
+ *   0 on success, negative on error and positive if no driver
+ *   is found for the device.
+ */
+__rte_experimental
+int rte_cdx_map_device(struct rte_cdx_device *dev);
+
+/**
+ * Unmap this device.
+ *
+ * @param dev
+ *   A pointer to a rte_cdx_device structure describing the device
+ *   to use.
+ */
+__rte_experimental
+void rte_cdx_unmap_device(struct rte_cdx_device *dev);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_BUS_CDX_H */
diff --git a/drivers/bus/cdx/version.map b/drivers/bus/cdx/version.map
index 0a15d39ae8..4ce444572b 100644
--- a/drivers/bus/cdx/version.map
+++ b/drivers/bus/cdx/version.map
@@ -1,9 +1,16 @@
-INTERNAL {
+EXPERIMENTAL {
 	global:
 
 	rte_cdx_map_device;
-	rte_cdx_register;
 	rte_cdx_unmap_device;
+
+	local: *;
+};
+
+INTERNAL {
+	global:
+
+	rte_cdx_register;
 	rte_cdx_unregister;
 	rte_cdx_vfio_intr_disable;
 	rte_cdx_vfio_intr_enable;
-- 
2.25.1

Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[Gupta, Nipun]

Please use 'in-reply-to' of the first submission when sending any 
subsequent patches.

On 7/11/2023 11:21 AM, Abhijit Gangurde wrote:
> Provide driver flag which gives an option to map the cdx
> device resource before probing the device driver.
> Also, make rte_cdx_map_device() API as public to map
> device resource separately.
> 
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>

Acked-by: Nipun Gupta <nipun.gupta@amd.com>

Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[David Marchand]

On Tue, Jul 11, 2023 at 7:52 AM Abhijit Gangurde
<abhijit.gangurde@amd.com> wrote:
> @@ -383,10 +384,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
>         CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
>                 dr->driver.name);
>
> -       ret = cdx_vfio_map_resource(dev);
> -       if (ret != 0) {
> -               CDX_BUS_ERR("CDX map device failed: %d", ret);
> -               goto error_map_device;
> +       if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
> +               ret = cdx_vfio_map_resource(dev);
> +               if (ret != 0) {
> +                       CDX_BUS_ERR("CDX map device failed: %d", ret);
> +                       goto error_map_device;
> +               }
>         }
>
>         /* call the driver probe() function */
> diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
> new file mode 100644
> index 0000000000..4ca12f90c4
> --- /dev/null
> +++ b/drivers/bus/cdx/rte_bus_cdx.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef RTE_BUS_CDX_H
> +#define RTE_BUS_CDX_H
> +
> +/**
> + * @file
> + * CDX device & driver interface
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* Forward declarations */
> +struct rte_cdx_device;
> +
> +/**
> + * Map the CDX device resources in user space virtual memory address.
> + *
> + * Note that driver should not call this function when flag
> + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> + * you when it's on.

Why should we export this function in the application ABI, if it is
only used by drivers?


> + *
> + * @param dev
> + *   A pointer to a rte_cdx_device structure describing the device
> + *   to use.
> + *
> + * @return
> + *   0 on success, negative on error and positive if no driver
> + *   is found for the device.
> + */
> +__rte_experimental
> +int rte_cdx_map_device(struct rte_cdx_device *dev);
>


-- 
David Marchand

RE: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[Gangurde, Abhijit]

[AMD Official Use Only - General]

> <abhijit.gangurde@amd.com> wrote:
> > @@ -383,10 +384,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
> >         CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
> >                 dr->driver.name);
> >
> > -       ret = cdx_vfio_map_resource(dev);
> > -       if (ret != 0) {
> > -               CDX_BUS_ERR("CDX map device failed: %d", ret);
> > -               goto error_map_device;
> > +       if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
> > +               ret = cdx_vfio_map_resource(dev);
> > +               if (ret != 0) {
> > +                       CDX_BUS_ERR("CDX map device failed: %d", ret);
> > +                       goto error_map_device;
> > +               }
> >         }
> >
> >         /* call the driver probe() function */
> > diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
> > new file mode 100644
> > index 0000000000..4ca12f90c4
> > --- /dev/null
> > +++ b/drivers/bus/cdx/rte_bus_cdx.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> > + */
> > +
> > +#ifndef RTE_BUS_CDX_H
> > +#define RTE_BUS_CDX_H
> > +
> > +/**
> > + * @file
> > + * CDX device & driver interface
> > + */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/* Forward declarations */
> > +struct rte_cdx_device;
> > +
> > +/**
> > + * Map the CDX device resources in user space virtual memory address.
> > + *
> > + * Note that driver should not call this function when flag
> > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> > + * you when it's on.
>
> Why should we export this function in the application ABI, if it is
> only used by drivers?

This can be called from an application as well if this flag is not set hence, we need to export this function.
If needed, I can update the description of the API.

Thanks,
Abhijit

Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[David Marchand]

On Wed, Oct 4, 2023 at 12:06 PM Gangurde, Abhijit
<abhijit.gangurde@amd.com> wrote:
> > > +/**
> > > + * Map the CDX device resources in user space virtual memory address.
> > > + *
> > > + * Note that driver should not call this function when flag
> > > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> > > + * you when it's on.
> >
> > Why should we export this function in the application ABI, if it is
> > only used by drivers?
>
> This can be called from an application as well if this flag is not set hence, we need to export this function.

What kind of applications / in which usecase, one would need to map
the device resources?
Except a driver?


-- 
David Marchand

RE: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[Gangurde, Abhijit]

> > > > +/**
> > > > + * Map the CDX device resources in user space virtual memory address.
> > > > + *
> > > > + * Note that driver should not call this function when flag
> > > > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> > > > + * you when it's on.
> > >
> > > Why should we export this function in the application ABI, if it is
> > > only used by drivers?
> >
> > This can be called from an application as well if this flag is not set hence, we
> need to export this function.
> 
> What kind of applications / in which usecase, one would need to map
> the device resources?
> Except a driver?

I understand that it is probably not the ideal use case, but some of the customers
are using a single application which also registers itself as driver. Probably such
applications need to use internal APIs instead of making these APIs external.
Will analyze it further and send another rev of this patch.

Thanks,
Abhijit

Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping

[David Marchand]

On Fri, Oct 13, 2023 at 1:52 PM Gangurde, Abhijit
<abhijit.gangurde@amd.com> wrote:
>
> > > > > +/**
> > > > > + * Map the CDX device resources in user space virtual memory address.
> > > > > + *
> > > > > + * Note that driver should not call this function when flag
> > > > > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> > > > > + * you when it's on.
> > > >
> > > > Why should we export this function in the application ABI, if it is
> > > > only used by drivers?
> > >
> > > This can be called from an application as well if this flag is not set hence, we
> > need to export this function.
> >
> > What kind of applications / in which usecase, one would need to map
> > the device resources?
> > Except a driver?
>
> I understand that it is probably not the ideal use case, but some of the customers
> are using a single application which also registers itself as driver. Probably such
> applications need to use internal APIs instead of making these APIs external.
> Will analyze it further and send another rev of this patch.

External drivers should be supported with current code.
DPDK must be built with enable_driver_sdk option, and the external
driver code can include bus_cdx_driver.h.
Possibly compiling this code with -DENABLE_INTERNAL_API cflag will be necessary.


-- 
David Marchand

[PATCH v4 1/1] bus/cdx: provide driver flag for optional resource mapping

[Abhijit Gangurde]

Provide driver flag which gives an option to map the cdx device
resource before probing the device driver. External driver can
use rte_cdx_map_device() and rte_cdx_unmap_device() APIs to map/
unmap device resource separately.

Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
---
 v4:
 - rte_cdx_map_device() and rte_cdx_unmap_device() APIs are kept
   as internal APIs.

 v3:
 - Changed APIs to __rte_experimental.

 v2:
 - Corrected _RTE_BUS_CDX_H_ to RTE_BUS_CDX_H
 - Improved code comments.

 drivers/bus/cdx/bus_cdx_driver.h |  3 +++
 drivers/bus/cdx/cdx.c            | 10 ++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
index fcacdb5896..1c9a64c87a 100644
--- a/drivers/bus/cdx/bus_cdx_driver.h
+++ b/drivers/bus/cdx/bus_cdx_driver.h
@@ -37,6 +37,9 @@ struct rte_cdx_bus;
 static const char DRV_EXP_TAG(name, cdx_tbl_export)[] __rte_used = \
 RTE_STR(table)
 
+/** Device needs resource mapping */
+#define RTE_CDX_DRV_NEED_MAPPING 0x0001
+
 /**
  * A structure describing an ID for a CDX driver. Each driver provides a
  * table of these IDs for each device that it supports.
diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
index f9526e08cc..541aae76c3 100644
--- a/drivers/bus/cdx/cdx.c
+++ b/drivers/bus/cdx/cdx.c
@@ -383,10 +383,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
 	CDX_BUS_DEBUG("  probe device %s using driver: %s", dev_name,
 		dr->driver.name);
 
-	ret = cdx_vfio_map_resource(dev);
-	if (ret != 0) {
-		CDX_BUS_ERR("CDX map device failed: %d", ret);
-		goto error_map_device;
+	if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
+		ret = cdx_vfio_map_resource(dev);
+		if (ret != 0) {
+			CDX_BUS_ERR("CDX map device failed: %d", ret);
+			goto error_map_device;
+		}
 	}
 
 	/* call the driver probe() function */
-- 
2.34.1

Re: [PATCH v4 1/1] bus/cdx: provide driver flag for optional resource mapping

[Thomas Monjalon]

16/10/2023 11:16, Abhijit Gangurde:
> Provide driver flag which gives an option to map the cdx device
> resource before probing the device driver. External driver can
> use rte_cdx_map_device() and rte_cdx_unmap_device() APIs to map/
> unmap device resource separately.
> 
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>

Applied, thanks.