Re: [PATCH v1 05/22] vfio/common: Extract out vfio_kvm_device_[add/del]_fd

[Cédric Le Goater]

On 9/20/23 13:49, Eric Auger wrote:

Hi Zhenzhong,

On 8/30/23 12:37, Zhenzhong Duan wrote:

...which will be used by both legacy and iommufd backend.

I prefer genuine sentences in the commit msg. Also you explain what you
do but not why.

suggestion: Introduce two new helpers, vfio_kvm_device_[add/del]_fd
which take as input a file descriptor which can be either a group fd or
a cdev fd. This uses the new KVM_DEV_VFIO_FILE VFIO KVM device group,
which aliases to the legacy KVM_DEV_VFIO_GROUP.

Ah yes. I didn't understand why the 's/GROUP/FILE/' change in the
VFIO KVM device ioctls. Thanks for clarifying.

What about pre-6.6 kernels without KVM_DEV_VFIO_FILE support ?

C.

vfio_kvm_device_add/del_group then call those new helpers.

Signed-off-by: Yi Liu <yi.l....@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
  hw/vfio/common.c              | 44 +++++++++++++++++++++++------------
  include/hw/vfio/vfio-common.h |  3 +++
  2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 67150e4575..949ad6714a 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1759,17 +1759,17 @@ void vfio_reset_handler(void *opaque)
      }
  }

-static void vfio_kvm_device_add_group(VFIOGroup *group)

+int vfio_kvm_device_add_fd(int fd)
  {
  #ifdef CONFIG_KVM
      struct kvm_device_attr attr = {
-        .group = KVM_DEV_VFIO_GROUP,
-        .attr = KVM_DEV_VFIO_GROUP_ADD,
-        .addr = (uint64_t)(unsigned long)&group->fd,
+        .group = KVM_DEV_VFIO_FILE,
+        .attr = KVM_DEV_VFIO_FILE_ADD,
+        .addr = (uint64_t)(unsigned long)&fd,
      };

if (!kvm_enabled()) {

-        return;
+        return 0;
      }

if (vfio_kvm_device_fd < 0) {

@@ -1779,37 +1779,51 @@ static void vfio_kvm_device_add_group(VFIOGroup *group)

if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {

              error_report("Failed to create KVM VFIO device: %m");
-            return;
+            return -ENODEV;

can't you return -errno?

          }

vfio_kvm_device_fd = cd.fd;

      }

if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {

-        error_report("Failed to add group %d to KVM VFIO device: %m",
-                     group->groupid);
+        error_report("Failed to add fd %d to KVM VFIO device: %m",
+                     fd);
+        return -errno;
      }
  #endif
+    return 0;
  }

-static void vfio_kvm_device_del_group(VFIOGroup *group)

+static void vfio_kvm_device_add_group(VFIOGroup *group)
+{
+    vfio_kvm_device_add_fd(group->fd);

Since vfio_kvm_device_add_fd now returns an error value, it's a pity not
to use it and propagate it. Also you could fill an errp with the error
msg and use it in vfio_connect_container(). But this is a new error
handling there.

+}
+
+int vfio_kvm_device_del_fd(int fd)

not sure we want this to return an error. But if we do, I think it would
be nicer to propagate the error up.

  {
  #ifdef CONFIG_KVM
      struct kvm_device_attr attr = {
-        .group = KVM_DEV_VFIO_GROUP,
-        .attr = KVM_DEV_VFIO_GROUP_DEL,
-        .addr = (uint64_t)(unsigned long)&group->fd,
+        .group = KVM_DEV_VFIO_FILE,
+        .attr = KVM_DEV_VFIO_FILE_DEL,
+        .addr = (uint64_t)(unsigned long)&fd,
      };

if (vfio_kvm_device_fd < 0) {

-        return;
+        return -EINVAL;
      }

if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {

-        error_report("Failed to remove group %d from KVM VFIO device: %m",
-                     group->groupid);
+        error_report("Failed to remove fd %d from KVM VFIO device: %m",
+                     fd);
+        return -EBADF;

-errno?

      }
  #endif
+    return 0;
+}
+
+static void vfio_kvm_device_del_group(VFIOGroup *group)
+{
+    vfio_kvm_device_del_fd(group->fd);
  }

static VFIOAddressSpace *vfio_get_address_space(AddressSpace *as)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 5e376c436e..598c3ce079 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -220,6 +220,9 @@ struct vfio_device_info *vfio_get_device_info(int fd);
  int vfio_get_device(VFIOGroup *group, const char *name,
                      VFIODevice *vbasedev, Error **errp);

+int vfio_kvm_device_add_fd(int fd);

+int vfio_kvm_device_del_fd(int fd);
+
  extern const MemoryRegionOps vfio_region_ops;
  typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
  extern VFIOGroupList vfio_group_list;

Thanks

Eric