lkml.org 
[lkml]   [2023]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2 v4] vfio-cdx: add ioctl support for bus master enable
From


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
>

\
 
 \ /
  Last update: 2023-07-28 12:55    [W:0.057 / U:2.952 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site