Re: [Intel-gfx] [PATCH v4 5/9] vfio: Mark cdev usage in vfio_device

[Yi Liu <yi.l.liu@intel.com>]

On 2023/4/28 02:43, Alex Williamson wrote:
> On Wed, 26 Apr 2023 07:54:15 -0700
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
>> Use it to differentiate whether to report group_id or dev_id in revised
>> VFIO_DEVICE_GET_PCI_HOT_RESET_INFO ioctl. Though it is not set at this
>> moment introducing it now allows us to get hot reset ready for cdev.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   include/linux/vfio.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index 4ee613924435..298f4ef16be7 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -63,6 +63,7 @@ struct vfio_device {
>>   	bool iommufd_attached;
>>   #endif
>>   	bool noiommu;
>> +	bool cdev_opened;
>>   };
>>   
>>   /**
>> @@ -140,6 +141,12 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id);
>>   	((int (*)(struct vfio_device *vdev, u32 *pt_id)) NULL)
>>   #endif
>>   
>> +static inline bool vfio_device_cdev_opened(struct vfio_device *device)
>> +{
>> +	lockdep_assert_held(&device->dev_set->lock);
>> +	return device->cdev_opened;
>> +}
> 
> The lockdep test doesn't make much sense here, the method of opening
> the device can't change from an ioctl called on the device, which is
> the only path using this.  

yes, it is only used by the ioctl paths so far. And this flag should be
static in other user-space trigger-able paths with device fd, like the
read/write/mmap. Do you think a comment would help to avoid abuse in other
paths or the name of it implies it should be used after a device is opened?

> If there needs to be a placeholder for
> future code, it should probably statically return false here and we can
> save adding the structure field and locking checks based on the
> semantics of how the field is actually used later.  Thanks,

Thanks for this suggestion. I was also wondering how to handle it when
Eric questioned better to add cdev_opened when it is really used.[1]

[1] 
https://lore.kernel.org/kvm/DS0PR11MB752914607E1DC9CFBFC2EF7AC3609@DS0PR11MB7529.namprd11.prod.outlook.com/

-- 
Regards,
Yi Liu