Messages in this thread | | | Date | Fri, 28 Jul 2023 16:24:31 +0530 | Subject | Re: [PATCH 2/2 v4] vfio-cdx: add ioctl support for bus master enable | From | "Gupta, Nipun" <> |
| |
On 7/26/2023 10:33 PM, Alex Williamson wrote: > On Wed, 26 Jul 2023 16:32:20 +0530 > Nipun Gupta <nipun.gupta@amd.com> wrote: > >> add VFIO_DEVICE_CDX_CTRL IOCTL to expose control operations for CDX >> devices to VFIO users. Support Bus master enable and Bus master disable >> on CDX bus control. >> >> Signed-off-by: Shubham Rohila <shubham.rohila@amd.com> >> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> >> --- >> >> Changes in v4: >> - This patch is newly added which uses cdx_set_master() and >> cdx_clear_master() APIs. >> >> drivers/vfio/cdx/main.c | 26 ++++++++++++++++++++++++++ >> include/uapi/linux/vfio.h | 14 ++++++++++++++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c >> index c376a69d2db2..c39a965716f4 100644 >> --- a/drivers/vfio/cdx/main.c >> +++ b/drivers/vfio/cdx/main.c >> @@ -98,6 +98,30 @@ static int vfio_cdx_ioctl_get_region_info(struct vfio_cdx_device *vdev, >> return copy_to_user(arg, &info, minsz) ? -EFAULT : 0; >> } >> >> +static int vfio_cdx_ioctl_ctrl(struct vfio_cdx_device *vdev, >> + struct vfio_device_cdx_ctrl __user *arg) >> +{ >> + unsigned long minsz = offsetofend(struct vfio_device_cdx_ctrl, op); >> + struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev); >> + struct vfio_device_cdx_ctrl ops; >> + >> + if (copy_from_user(&ops, arg, minsz)) >> + return -EFAULT; >> + >> + if (ops.argsz < minsz) >> + return -EINVAL; >> + >> + switch (ops.op) { >> + case VFIO_CDX_CTRL_CLEAR_MASTER: >> + cdx_clear_master(cdx_dev); >> + return 0; >> + case VFIO_CDX_CTRL_SET_MASTER: >> + return cdx_set_master(cdx_dev); >> + default: >> + return -EINVAL; >> + } >> +} >> + >> static long vfio_cdx_ioctl(struct vfio_device *core_vdev, >> unsigned int cmd, unsigned long arg) >> { >> @@ -112,6 +136,8 @@ static long vfio_cdx_ioctl(struct vfio_device *core_vdev, >> return vfio_cdx_ioctl_get_region_info(vdev, uarg); >> case VFIO_DEVICE_RESET: >> return cdx_dev_reset(core_vdev->dev); >> + case VFIO_DEVICE_CDX_CTRL: >> + return vfio_cdx_ioctl_ctrl(vdev, uarg); >> default: >> return -ENOTTY; >> } >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index 20c804bdc09c..5f6a58f9f8e2 100644 >> --- a/include/uapi/linux/vfio.h >> +++ b/include/uapi/linux/vfio.h >> @@ -1649,6 +1649,20 @@ struct vfio_iommu_spapr_tce_remove { >> }; >> #define VFIO_IOMMU_SPAPR_TCE_REMOVE _IO(VFIO_TYPE, VFIO_BASE + 20) >> >> +/** >> + * VFIO_DEVICE_CDX_CTRL - _IO(VFIO_TYPE, VFIO_BASE + 21) >> + * >> + * Control CDX device. >> + * Variable op is set as per the operation required >> + */ >> +struct vfio_device_cdx_ctrl { >> + __u32 argsz; >> + __u32 op; >> +#define VFIO_CDX_CTRL_SET_MASTER 0 /* Set Bus Master */ >> +#define VFIO_CDX_CTRL_CLEAR_MASTER 1 /* Clear Bus Master */ >> +}; >> +#define VFIO_DEVICE_CDX_CTRL _IO(VFIO_TYPE, VFIO_BASE + 21) >> + >> /* ***************************************************************** */ >> >> #endif /* _UAPIVFIO_H */ > > This doesn't follow standard conventions for a vfio ioctl. Not knowing > how CDX works, is there really a need for this ioctl vs enabling bus > master when the device is opened and clearing it when closed? I think > this is effectively the behavior of vfio-platform. Otherwise, I assume > this is not a high frequency operation, would the VFIO_DEVICE_FEATURE > ioctl be an option? Thanks,
Thank you for the quick feedback. There are few use-cases where VFIO-cdx users need to explicitly trigger bus master enable/disable. As you suggest VFIO_DEVICE_FEATURE is better suited in this case, and we will move the BME handling in this ioctl.
Regards, Nipun
> > Alex >
| |