lkml.org 
[lkml]   [2023]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support
From


On 8/16/2023 11:16 PM, Alex Williamson wrote:
> On Thu, 10 Aug 2023 14:14:09 +0530
> Nipun Gupta <nipun.gupta@amd.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@amd.com>
>> Signed-off-by: Shubham Rohila <shubham.rohila@amd.com>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.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).

CDX bus driver does not explicitly call cdx_clear_master during reset.
It is triggered by device implicitly and hence device_reset would never
fail due to lack of bus mastering capability.

Do you mean if cdx_dev_reset() fails we should not set the
VFIO_DEVICE_FLAGS_RESET in vfio_device_info? something like:

ret = cdx_dev_reset(core_vdev->dev);
if (ret == -EOPNOTSUPP)
/* make sure VFIO_DEVICE_FLAGS_RESET is not returned in
* flags for device get info */
else if (ret)
kfree(vdev->regions);

From new device feature added for BUS mastering if cdx_clear_master()
fails due to no support, the bus driver will return -EOPNOTSUPP, so same
would be communicated to the user-space, so it seems fine from this end.

>
> 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,

Sure. Will update.

Thanks,
Nipun

>
> Alex

\
 
 \ /
  Last update: 2023-08-18 10:34    [W:0.593 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site