[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230816114610.79c9eccc.alex.williamson@redhat.com>
Date: Wed, 16 Aug 2023 11:46:10 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Nipun Gupta <nipun.gupta@....com>
Cc: <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
<git@....com>, <pieter.jansen-van-vuuren@....com>,
<nikhil.agarwal@....com>, <michal.simek@....com>,
<abhijit.gangurde@....com>, Shubham Rohila <shubham.rohila@....com>
Subject: Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature
support
On Thu, 10 Aug 2023 14:14:09 +0530
Nipun Gupta <nipun.gupta@....com> wrote:
> Support Bus master enable and disable on VFIO-CDX devices using
> VFIO_DEVICE_FEATURE_BUS_MASTER flag over VFIO_DEVICE_FEATURE IOCTL.
>
> Co-developed-by: Shubham Rohila <shubham.rohila@....com>
> Signed-off-by: Shubham Rohila <shubham.rohila@....com>
> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
> ---
>
> Changes v5->v6:
> - Called CDX device reset at cdx_open_device()
>
> Changes v4->v5:
> - Use device feature IOCTL instead of adding a new VFIO IOCTL
> for bus master feature.
>
> Changes in v4:
> - This patch is newly added which uses cdx_set_master() and
> cdx_clear_master() APIs.
>
> drivers/vfio/cdx/main.c | 46 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> index c376a69d2db2..bf0e1f56e0f9 100644
> --- a/drivers/vfio/cdx/main.c
> +++ b/drivers/vfio/cdx/main.c
> @@ -14,7 +14,7 @@ static int vfio_cdx_open_device(struct vfio_device *core_vdev)
> container_of(core_vdev, struct vfio_cdx_device, vdev);
> struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> int count = cdx_dev->res_count;
> - int i;
> + int i, ret;
>
> vdev->regions = kcalloc(count, sizeof(struct vfio_cdx_region),
> GFP_KERNEL_ACCOUNT);
> @@ -39,8 +39,11 @@ static int vfio_cdx_open_device(struct vfio_device *core_vdev)
> if (!(cdx_dev->res[i].flags & IORESOURCE_READONLY))
> vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
> }
> + ret = cdx_dev_reset(core_vdev->dev);
> + if (ret)
> + kfree(vdev->regions);
AIUI, this reset clears bus master, but per the first patch in the
series the ability to set or clear bus master depends on whether the
underlying cdx_ops supports dev_configure. Apparently all currently
do, but will that always be true?
It seems like this could make a gratuitous call to cdx_clear_master()
to validate the return value and only conditionally support this device
feature based on that result (or fail the device open if it's meant to
be required).
It might also be a good idea to set vdev->regions = NULL in the error
path to avoid the possibility of a double-free. Thanks,
Alex
>
> - return 0;
> + return ret;
> }
>
> static void vfio_cdx_close_device(struct vfio_device *core_vdev)
> @@ -52,6 +55,44 @@ static void vfio_cdx_close_device(struct vfio_device *core_vdev)
> cdx_dev_reset(core_vdev->dev);
> }
>
> +static int vfio_cdx_bm_ctrl(struct vfio_device *core_vdev, u32 flags,
> + void __user *arg, size_t argsz)
> +{
> + size_t minsz =
> + offsetofend(struct vfio_device_feature_bus_master, op);
> + struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
> + struct vfio_device_feature_bus_master ops;
> + int ret;
> +
> + ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> + sizeof(ops));
> + if (ret != 1)
> + return ret;
> +
> + if (copy_from_user(&ops, arg, minsz))
> + return -EFAULT;
> +
> + switch (ops.op) {
> + case VFIO_DEVICE_FEATURE_CLEAR_MASTER:
> + return cdx_clear_master(cdx_dev);
> + case VFIO_DEVICE_FEATURE_SET_MASTER:
> + return cdx_set_master(cdx_dev);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int vfio_cdx_ioctl_feature(struct vfio_device *device, u32 flags,
> + void __user *arg, size_t argsz)
> +{
> + switch (flags & VFIO_DEVICE_FEATURE_MASK) {
> + case VFIO_DEVICE_FEATURE_BUS_MASTER:
> + return vfio_cdx_bm_ctrl(device, flags, arg, argsz);
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> static int vfio_cdx_ioctl_get_info(struct vfio_cdx_device *vdev,
> struct vfio_device_info __user *arg)
> {
> @@ -169,6 +210,7 @@ static const struct vfio_device_ops vfio_cdx_ops = {
> .open_device = vfio_cdx_open_device,
> .close_device = vfio_cdx_close_device,
> .ioctl = vfio_cdx_ioctl,
> + .device_feature = vfio_cdx_ioctl_feature,
> .mmap = vfio_cdx_mmap,
> .bind_iommufd = vfio_iommufd_physical_bind,
> .unbind_iommufd = vfio_iommufd_physical_unbind,
Powered by blists - more mailing lists