Re: [PATCH v6 3/3] vfio-cdx: add bus mastering device feature support

["Gupta, Nipun" <nipun.gupta@amd.com>]

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