RE: [PATCH v6 11/21] vfio/pci: Make vfio cdev pre-openable by passing a file handle

[Duan, Zhenzhong]

Hi Philippe,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Wednesday, November 15, 2023 9:05 PM
>Subject: Re: [PATCH v6 11/21] vfio/pci: Make vfio cdev pre-openable by passing 
>a
>file handle
>
>On 11/15/23 13:09, Philippe Mathieu-Daudé wrote:
>> Hi Zhenzhong,
>>
>> On 14/11/23 11:09, Zhenzhong Duan wrote:
>>> This gives management tools like libvirt a chance to open the vfio
>>> cdev with privilege and pass FD to qemu. This way qemu never needs
>>> to have privilege to open a VFIO or iommu cdev node.
>>>
>>> Together with the earlier support of pre-opening /dev/iommu device,
>>> now we have full support of passing a vfio device to unprivileged
>>> qemu by management tool. This mode is no more considered for the
>>> legacy backend. So let's remove the "TODO" comment.
>>>
>>> Add helper functions vfio_device_set_fd() and vfio_device_get_name()
>>> to set fd and get device name, they will also be used by other vfio
>>> devices.
>>>
>>> There is no easy way to check if a device is mdev with FD passing,
>>> so fail the x-balloon-allowed check unconditionally in this case.
>>>
>>> There is also no easy way to get BDF as name with FD passing, so
>>> we fake a name by VFIO_FD[fd].
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>> v6: simplify CONFIG_IOMMUFD checking code
>>>      introduce a helper vfio_device_set_fd
>>>
>>>   include/hw/vfio/vfio-common.h |  3 +++
>>>   hw/vfio/helpers.c             | 44 +++++++++++++++++++++++++++++++++++
>>>   hw/vfio/iommufd.c             | 12 ++++++----
>>>   hw/vfio/pci.c                 | 28 ++++++++++++----------
>>>   4 files changed, 71 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index 3dac5c167e..567e5f7bea 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -251,4 +251,7 @@ int
>vfio_devices_query_dirty_bitmap(VFIOContainerBase *bcontainer,
>>>                                       hwaddr size);
>>>   int vfio_get_dirty_bitmap(VFIOContainerBase *bcontainer, uint64_t iova,
>>>                                    uint64_t size, ram_addr_t ram_addr);
>>> +
>>
>> Please add bare documentation:
>>
>>    /* Returns 0 on success, or a negative errno. */
>>
>>> +int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);

Will do, I'd like to wait a few days to collect more suggested changes and RB,
Then send all these updates to Cédric in once before he pushes this series to 
vfio-next.

>>
>> Functions taking an Error** param should return a boolean, so:
>>
>>    /* Return: true on success, else false setting @errp with error. */
>>
>>> +void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error 
>>> **errp);
>>>   #endif /* HW_VFIO_VFIO_COMMON_H */
>>
>>
>>> @@ -609,3 +611,45 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int
>region, uint16_t cap_type)
>>>       return ret;
>>>   }
>>> +
>>> +int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
>>> +{
>>> +    struct stat st;
>>> +
>>> +    if (vbasedev->fd < 0) {
>>> +        if (stat(vbasedev->sysfsdev, &st) < 0) {
>>> +            error_setg_errno(errp, errno, "no such host device");
>>> +            error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->sysfsdev);
>>> +            return -errno;
>>> +        }
>>> +        /* User may specify a name, e.g: VFIO platform device */
>>> +        if (!vbasedev->name) {
>>> +            vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
>>> +        }
>>> +    } else {
>>> +        if (!vbasedev->iommufd) {
>>> +            error_setg(errp, "Use FD passing only with iommufd backend");
>>> +            return -EINVAL;
>>> +        }
>>> +        /*
>>> +         * Give a name with fd so any function printing out vbasedev->name
>>> +         * will not break.
>>> +         */
>>> +        if (!vbasedev->name) {
>>> +            vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error 
>>> **errp)
>>
>>     bool vfio_device_set_fd(..., Error **errp)
>>
>>> +{
>>> +    int fd = monitor_fd_param(monitor_cur(), str, errp);
>>> +
>>> +    if (fd < 0) {
>>> +        error_prepend(errp, "Could not parse remote object fd %s:", str);
>>> +        return;
>>
>>             return false;
>>
>>> +    }
>>> +    vbasedev->fd = fd;
>>
>>         return true;
>
>If we had a QOM base device object, vfio_device_set_fd() would be passed
>directly to object_class_property_add_str() which expects a :
>
>   void (*set)(Object *, const char *, Error **)
>
>I think it is fine to keep as it is. We might have a QOM base device object
>one day ! Minor anyway.
>
>Thanks,
>
>C.
>
>
>>
>>> +}
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index 3eec428162..e08a217057 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -326,11 +326,15 @@ static int iommufd_cdev_attach(const char *name,
>VFIODevice *vbasedev,
>>>       uint32_t ioas_id;
>>>       Error *err = NULL;
>>> -    devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
>>> -    if (devfd < 0) {
>>> -        return devfd;
>>> +    if (vbasedev->fd < 0) {
>>> +        devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
>>> +        if (devfd < 0) {
>>> +            return devfd;
>>> +        }
>>> +        vbasedev->fd = devfd;
>>> +    } else {
>>> +        devfd = vbasedev->fd;
>>>       }
>>> -    vbasedev->fd = devfd;
>>>       ret = iommufd_cdev_connect_and_bind(vbasedev, errp);
>>>       if (ret) {
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index c5984b0598..b23b492cce 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2944,17 +2944,19 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>>       VFIODevice *vbasedev = &vdev->vbasedev;
>>>       char *tmp, *subsys;
>>>       Error *err = NULL;
>>> -    struct stat st;
>>>       int i, ret;
>>>       bool is_mdev;
>>>       char uuid[UUID_STR_LEN];
>>>       char *name;
>>> -    if (!vbasedev->sysfsdev) {
>>> +    if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
>>>           if (!(~vdev->host.domain || ~vdev->host.bus ||
>>>                 ~vdev->host.slot || ~vdev->host.function)) {
>>>               error_setg(errp, "No provided host device");
>>>               error_append_hint(errp, "Use -device 
>>> vfio-pci,host=DDDD:BB:DD.F "
>>> +#ifdef CONFIG_IOMMUFD
>>> +                              "or -device vfio-pci,fd=DEVICE_FD "
>>> +#endif
>>>                                 "or -device 
>>> vfio-pci,sysfsdev=PATH_TO_DEVICE\n");
>>>               return;
>>>           }
>>> @@ -2964,13 +2966,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>>                               vdev->host.slot, vdev->host.function);
>>>       }
>>> -    if (stat(vbasedev->sysfsdev, &st) < 0) {
>>> -        error_setg_errno(errp, errno, "no such host device");
>>> -        error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->sysfsdev);
>>> +    if (vfio_device_get_name(vbasedev, errp)) {
>>
>> Clearer as:
>>
>>         if (vfio_device_get_name(vbasedev, errp) < 0) {
>>
>>>           return;
>>>       }

Will do.

Thanks
Zhenzhong