[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2d94cffa-7ebf-a8ab-4f43-fc9ab1be41bb@amd.com>
Date: Fri, 18 Aug 2023 14:02:32 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Alex Williamson <alex.williamson@...hat.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 8/16/2023 11:16 PM, Alex Williamson wrote:
> 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).
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
Powered by blists - more mailing lists