RE: [PATCH v5 10/20] vfio/pci: Make vfio cdev pre-openable by passing a file handle

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, November 10, 2023 6:53 PM
>Subject: Re: [PATCH v5 10/20] vfio/pci: Make vfio cdev pre-openable by passing 
>a
>file handle
>
>On 11/9/23 12:45, 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 a helper function vfio_device_get_name() to check fd and get
>> device name, it 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>
>> ---
>>   include/hw/vfio/vfio-common.h |  1 +
>>   hw/vfio/helpers.c             | 34 +++++++++++++++++++++++++++++
>>   hw/vfio/iommufd.c             | 12 +++++++----
>>   hw/vfio/pci.c                 | 40 ++++++++++++++++++++++++-----------
>>   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..960a14e8d8 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -238,6 +238,7 @@ struct vfio_info_cap_header *
>>   vfio_get_device_info_cap(struct vfio_device_info *info, uint16_t id);
>>   struct vfio_info_cap_header *
>>   vfio_get_cap(void *ptr, uint32_t cap_offset, uint16_t id);
>> +int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
>>   #endif
>>
>>   bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
>> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
>> index 168847e7c5..d80aa58719 100644
>> --- a/hw/vfio/helpers.c
>> +++ b/hw/vfio/helpers.c
>> @@ -20,6 +20,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>   #include <sys/ioctl.h>
>>
>>   #include "hw/vfio/vfio-common.h"
>> @@ -609,3 +610,36 @@ 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);
>> +        }
>> +    }
>> +#ifdef CONFIG_IOMMUFD
>> +    else {
>> +        if (!vbasedev->iommufd) {
>
>
>Can we handle with this case without CONFIG_IOMMUFD, simply by
>testing vbasedev->iommufd ?

Sure, will do.

>
>> +            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);
>> +        }
>> +    }
>> +#endif
>> +    return 0;
>> +}
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 44dc6848bf..fd30477275 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -326,11 +326,15 @@ static int iommufd_attach_device(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_connect_and_bind(vbasedev, errp);
>>       if (ret) {
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index e9a426200b..f95725ed16 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -44,6 +44,7 @@
>>   #include "migration/blocker.h"
>>   #include "migration/qemu-file.h"
>>   #include "sysemu/iommufd.h"
>> +#include "monitor/monitor.h"
>>
>>   #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
>>
>> @@ -2934,18 +2935,23 @@ 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");
>> +#ifdef CONFIG_IOMMUFD
>> +            error_append_hint(errp, "Use -device 
>> vfio-pci,host=DDDD:BB:DD.F, "
>> +                              "-device vfio-pci,sysfsdev=PATH_TO_DEVICE "
>> +                              "or -device vfio-pci,fd=DEVICE_FD\n");
>> +#else
>>               error_append_hint(errp, "Use -device 
>> vfio-pci,host=DDDD:BB:DD.F "
>>                                 "or -device 
>> vfio-pci,sysfsdev=PATH_TO_DEVICE\n");
>> +#endif
>
>or simply :
>
>
>                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");

Good idea, will do.

>
>
>
>>               return;
>>           }
>>           vbasedev->sysfsdev =
>> @@ -2954,13 +2960,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)) {
>>           return;
>>       }
>> -
>> -    vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
>>       vbasedev->ops = &vfio_pci_ops;
>>       vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>>       vbasedev->dev = DEVICE(vdev);
>> @@ -3320,6 +3322,7 @@ static void vfio_instance_init(Object *obj)
>>       vdev->host.bus = ~0U;
>>       vdev->host.slot = ~0U;
>>       vdev->host.function = ~0U;
>> +    vdev->vbasedev.fd = -1;
>We should probably move the all VFIODevice initializations :
>
>     vbasedev->ops = &vfio_pci_ops;
>     vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>     vbasedev->dev = DEVICE(vdev);
>
>under vfio_instance_init (should be called vfio_pci_instance_init).
>
>This is true for all other VFIO devices. May be not for this series,
>it can come later.

Sure, Let me send a separate series addressing this.

>
>
>>
>>       vdev->nv_gpudirect_clique = 0xFF;
>>
>> @@ -3373,11 +3376,6 @@ static Property vfio_pci_dev_properties[] = {
>>                                      qdev_prop_nv_gpudirect_clique, uint8_t),
>>       DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice,
>msix_relo,
>>                                   OFF_AUTOPCIBAR_OFF),
>> -    /*
>> -     * TODO - support passed fds... is this necessary?
>> -     * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
>> -     * DEFINE_PROP_STRING("vfiogroupfd, VFIOPCIDevice, vfiogroupfd_name),
>> -     */
>>   #ifdef CONFIG_IOMMUFD
>>       DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
>>                        TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
>> @@ -3385,6 +3383,21 @@ static Property vfio_pci_dev_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>> +#ifdef CONFIG_IOMMUFD
>> +static void vfio_pci_set_fd(Object *obj, const char *str, Error **errp)
>> +{
>> +    VFIOPCIDevice *vdev = VFIO_PCI(obj);
>> +    int fd = -1;
>> +
>> +    fd = monitor_fd_param(monitor_cur(), str, errp);
>> +    if (fd == -1) {
>> +        error_prepend(errp, "Could not parse remote object fd %s:", str);
>> +        return;
>> +    }
>> +    vdev->vbasedev.fd = fd;
>
>We could introduce a common helper in hw/vfio/common.c to remove code
>duplication :
>
>#ifdef CONFIG_IOMMUFD
>static void vfio_pci_set_fd(Object *obj, const char *str, Error **errp)
>{
>     vfio_device_set_fd(&VFIO_PCI(obj)->vbasedev, str, errp);
>}
>#endif

Good suggestions! Will do.

Thanks
Zhenzhong