Re: [PATCH] Add bus master enable/disable APIs for CDX devices

["Gupta, Nipun" <nipun.gupta@amd.com>]

On 10/26/2023 8:50 AM, Shubham Rohila wrote:
> Define rte_cdx_vfio_bm_enable and rte_cdx_vfio_bm_disable to
> enable or disable bus master functionality for cdx devices.
> 
> Signed-off-by: Shubham Rohila <shubham.rohila@amd.com>
> ---
>   drivers/bus/cdx/bus_cdx_driver.h | 25 ++++++++
>   drivers/bus/cdx/cdx_vfio.c       | 97 ++++++++++++++++++++++++++++++++
>   drivers/bus/cdx/version.map      |  2 +
>   lib/eal/include/rte_vfio.h       | 27 +++++++++
>   4 files changed, 151 insertions(+)
> 
> diff --git a/drivers/bus/cdx/bus_cdx_driver.h b/drivers/bus/cdx/bus_cdx_driver.h
> index 1c9a64c87a..2f94a6c3af 100644
> --- a/drivers/bus/cdx/bus_cdx_driver.h
> +++ b/drivers/bus/cdx/bus_cdx_driver.h
> @@ -16,6 +16,7 @@ extern "C" {
>   
>   #include <stdlib.h>
>   #include <inttypes.h>
> +#include <linux/types.h>
>   
>   #include <bus_driver.h>
>   #include <dev_driver.h>
> @@ -178,6 +179,30 @@ int rte_cdx_vfio_intr_enable(const struct rte_intr_handle *intr_handle);
>   __rte_internal
>   int rte_cdx_vfio_intr_disable(const struct rte_intr_handle *intr_handle);
>   
> +/**
> + * Enable Bus Mastering for CDX bus devices.
> + *
> + * @param dev
> + *   Pointer to the cdx device.
> + *
> + *  @return
> + *  0 on success, -1 on error.

alignment should be similar to @param dev

> + */
> +__rte_internal
> +int rte_cdx_vfio_bm_enable(struct rte_cdx_device *dev);
> +
> +/**
> + * Disable Bus Mastering for CDX bus devices.
> + *
> + * @param dev
> + *   Pointer to the cdx device.
> + *
> + *  @return
> + *  0 on success, -1 on error.

here too

> + */
> +__rte_internal
> +int rte_cdx_vfio_bm_disable(struct rte_cdx_device *dev);
> +
>   /**
>    * Unregister a CDX driver.
>    *
> diff --git a/drivers/bus/cdx/cdx_vfio.c b/drivers/bus/cdx/cdx_vfio.c
> index 8a3ac0b995..a0abf71483 100644
> --- a/drivers/bus/cdx/cdx_vfio.c
> +++ b/drivers/bus/cdx/cdx_vfio.c
> @@ -256,6 +256,16 @@ cdx_vfio_setup_device(struct rte_cdx_device *dev, int vfio_dev_fd,
>   		return -1;
>   	}
>   
> +	/*
> +	 * Enable Bus mastering for the device. errno is set as ENOTTY if
> +	 * device does not support configuring bus master.
> +	 */
> +	if (rte_cdx_vfio_bm_enable(dev) && errno != -ENOTTY) {

errno != -ENOTTY can be in brackets for better readability.

As we are probing the functionality in the 
rte_cdx_vfio_bm_enable/disable API, so why do we need a check for errno?

> +		CDX_BUS_ERR("Bus master enable failure! Error: %d (%s)", errno,
> +			strerror(errno));
> +		return -1;
> +	}

Disable in cdx_vfio_unmap_resource_primary?

> +
>   	return 0;
>   }
>   
> @@ -596,3 +606,90 @@ rte_cdx_vfio_intr_disable(const struct rte_intr_handle *intr_handle)
>   
>   	return ret;
>   }
> +
> +/* enable Bus Mastering */
> +int
> +rte_cdx_vfio_bm_enable(struct rte_cdx_device *dev)
> +{
> +	struct vfio_device_info device_info = { .argsz = sizeof(device_info) };
> +	struct vfio_device_feature_bus_master *vfio_bm_feature;
> +	struct vfio_device_feature *feature;
> +	int vfio_dev_fd, ret = 0;
> +	size_t argsz = 0;

no need to initialize argsz and ret here.

> +
> +	vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
> +	if (vfio_dev_fd < 0)
> +		return -1;
> +
> +	argsz = sizeof(struct vfio_device_feature) + sizeof(struct vfio_device_feature_bus_master);
> +
> +	feature = (struct vfio_device_feature *)malloc(argsz);
> +	if (!feature)
> +		return -ENOMEM;
> +
> +	vfio_bm_feature = (struct vfio_device_feature_bus_master *) feature->data;
> +
> +	feature->argsz = argsz;
> +
> +	feature->flags = RTE_VFIO_DEVICE_FEATURE_BUS_MASTER | VFIO_DEVICE_FEATURE_PROBE;
> +	feature->flags |= VFIO_DEVICE_FEATURE_SET;
> +	ret = ioctl(vfio_dev_fd, RTE_VFIO_DEVICE_FEATURE, feature);
> +	if (ret) {
> +		CDX_BUS_ERR("Bus Master configuring not supported for device: %s, error: %d (%s)\n",
> +			dev->name, errno, strerror(errno));
> +		free(feature);
> +		return ret;
> +	}
> +
> +	feature->flags = RTE_VFIO_DEVICE_FEATURE_BUS_MASTER | VFIO_DEVICE_FEATURE_SET;
> +	vfio_bm_feature->op = VFIO_DEVICE_FEATURE_SET_MASTER;
> +	ret = ioctl(vfio_dev_fd, RTE_VFIO_DEVICE_FEATURE, feature);
> +	if (ret < 0)
> +		CDX_BUS_ERR("BM Enable Error for device: %s, Error: %d (%s)\n",
> +			dev->name, errno, strerror(errno));
> +
> +	free(feature);
> +	return ret;
> +}
> +
> +/* Disable Bus Mastering */
> +int
> +rte_cdx_vfio_bm_disable(struct rte_cdx_device *dev)
> +{
> +	struct vfio_device_feature_bus_master *vfio_bm_feature;
> +	struct vfio_device_feature *feature;
> +	int vfio_dev_fd, ret = 0;
> +	size_t argsz = 0;

no need to initialize argsz and ret here.

> +
> +	vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
> +	if (vfio_dev_fd < 0)
> +		return -1;
> +
> +	feature = (struct vfio_device_feature *)malloc(argsz);

need to assign argsz before?

> +	if (!feature)
> +		return -ENOMEM;
> +
> +	vfio_bm_feature = (struct vfio_device_feature_bus_master *) feature->data;
> +
> +	feature->argsz = argsz;
> +
> +	feature->flags = RTE_VFIO_DEVICE_FEATURE_BUS_MASTER | VFIO_DEVICE_FEATURE_PROBE;
> +	feature->flags |= VFIO_DEVICE_FEATURE_SET;
> +	ret = ioctl(vfio_dev_fd, RTE_VFIO_DEVICE_FEATURE, feature);
> +	if (ret) {
> +		CDX_BUS_ERR("Bus Master configuring not supported for device: %s, Error: %d (%s)\n",
> +			dev->name, errno, strerror(errno));
> +		free(feature);
> +		return ret;
> +	}
> +
> +	feature->flags = RTE_VFIO_DEVICE_FEATURE_BUS_MASTER | VFIO_DEVICE_FEATURE_SET;
> +	vfio_bm_feature->op = VFIO_DEVICE_FEATURE_CLEAR_MASTER;
> +	ret = ioctl(vfio_dev_fd, RTE_VFIO_DEVICE_FEATURE, feature);
> +	if (ret < 0)
> +		CDX_BUS_ERR("BM Disable Error for device: %s, Error: %d (%s)\n",
> +			dev->name, errno, strerror(errno));
> +
> +	free(feature);
> +	return ret;
> +}
> diff --git a/drivers/bus/cdx/version.map b/drivers/bus/cdx/version.map
> index 0a15d39ae8..5438f76029 100644
> --- a/drivers/bus/cdx/version.map
> +++ b/drivers/bus/cdx/version.map
> @@ -7,6 +7,8 @@ INTERNAL {
>   	rte_cdx_unregister;
>   	rte_cdx_vfio_intr_disable;
>   	rte_cdx_vfio_intr_enable;
> +	rte_cdx_vfio_bm_enable;
> +	rte_cdx_vfio_bm_disable;

Alphabetical order.

>   
>   	local: *;
>   };
> diff --git a/lib/eal/include/rte_vfio.h b/lib/eal/include/rte_vfio.h
> index 3487c4f2a2..1c791b3636 100644
> --- a/lib/eal/include/rte_vfio.h
> +++ b/lib/eal/include/rte_vfio.h
> @@ -73,6 +73,33 @@ struct vfio_info_cap_header {
>   #define RTE_VFIO_CAP_MSIX_MAPPABLE 3
>   #endif
>   
> +/* VFIO_DEVICE_FEATURE is defined for kernel version 5.7 and newer. */
> +#ifdef VFIO_DEVICE_FEATURE
> +#define RTE_VFIO_DEVICE_FEATURE VFIO_DEVICE_FEATURE
> +#else
> +#define RTE_VFIO_DEVICE_FEATURE             _IO(VFIO_TYPE, VFIO_BASE + 17)

use tabs.

> +struct vfio_device_feature {
> +	__u32   argsz;
> +	__u32   flags;
> +#define VFIO_DEVICE_FEATURE_MASK        (0xffff) /* 16-bit feature index */
> +#define VFIO_DEVICE_FEATURE_GET         (1 << 16) /* Get feature into data[] */
> +#define VFIO_DEVICE_FEATURE_SET         (1 << 17) /* Set feature from data[] */
> +#define VFIO_DEVICE_FEATURE_PROBE       (1 << 18) /* Probe feature support */
> +	__u8    data[];
> +};
> +#endif
> +
> +#ifdef VFIO_DEVICE_FEATURE_BUS_MASTER
> +#define RTE_VFIO_DEVICE_FEATURE_BUS_MASTER VFIO_DEVICE_FEATURE_BUS_MASTER
> +#else
> +#define RTE_VFIO_DEVICE_FEATURE_BUS_MASTER  10
> +struct vfio_device_feature_bus_master {
> +	__u32 op;
> +#define  VFIO_DEVICE_FEATURE_CLEAR_MASTER       0       /* Clear Bus Master */
> +#define  VFIO_DEVICE_FEATURE_SET_MASTER         1       /* Set Bus Master */

and tabs in all above here.

Regards,
Nipun