Re: [PATCH for-10.1 13/32] vfio: Move VFIOAddressSpace helpers into container-base.c

[Cédric Le Goater]

+John

On 3/20/25 10:36, Duan, Zhenzhong wrote:


-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Subject: [PATCH for-10.1 13/32] vfio: Move VFIOAddressSpace helpers into
container-base.c

VFIOAddressSpace is a common object used by VFIOContainerBase which is
declared in "hw/vfio/vfio-container-base.h". Move the VFIOAddressSpace
related services into "container-base.c".

While at it, rename :

  vfio_get_address_space -> vfio_address_space_get
  vfio_put_address_space -> vfio_address_space_put

to better reflect the namespace these routines belong to.

Signed-off-by: Cédric Le Goater <c...@redhat.com>
---
include/hw/vfio/vfio-common.h         |  6 ---
include/hw/vfio/vfio-container-base.h |  5 ++
hw/ppc/spapr_pci_vfio.c               |  5 +-
hw/vfio/common.c                      | 66 -------------------------
hw/vfio/container-base.c              | 69 +++++++++++++++++++++++++++
hw/vfio/container.c                   |  6 +--
hw/vfio/iommufd.c                     |  6 +--
hw/vfio/trace-events                  |  4 +-
8 files changed, 85 insertions(+), 82 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index
e23626856e6ff96939a4660f059833f166aa88e9..2ea7f9c6f6e7e752699954ac236
cac0bbe834b39 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -120,18 +120,12 @@ struct VFIODeviceOps {
#define TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO \
             TYPE_HOST_IOMMU_DEVICE_IOMMUFD "-vfio"

-VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
-void vfio_put_address_space(VFIOAddressSpace *space);
-void vfio_address_space_insert(VFIOAddressSpace *space,
-                               VFIOContainerBase *bcontainer);
-
void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
bool vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
                             int action, int fd, Error **errp);

-void vfio_reset_handler(void *opaque);
struct vfio_device_info *vfio_get_device_info(int fd);
bool vfio_device_is_mdev(VFIODevice *vbasedev);
bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
container-base.h
index
4cff9943ab4861a25d07b5ebd1200509ebfab12d..27668879f5ca77e558a2bda954
8c8e60afefe794 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -71,6 +71,11 @@ typedef struct VFIORamDiscardListener {
     QLIST_ENTRY(VFIORamDiscardListener) next;
} VFIORamDiscardListener;

+VFIOAddressSpace *vfio_address_space_get(AddressSpace *as);
+void vfio_address_space_put(VFIOAddressSpace *space);
+void vfio_address_space_insert(VFIOAddressSpace *space,
+                               VFIOContainerBase *bcontainer);
+
int vfio_container_dma_map(VFIOContainerBase *bcontainer,
                            hwaddr iova, ram_addr_t size,
                            void *vaddr, bool readonly);
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index
1722a5bfa3983d42baac558f22410e36eed375f5..e318d0d912f3e90d1289e4bc21
95bf68418e5206 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -24,7 +24,6 @@
#include "hw/pci-host/spapr.h"
#include "hw/pci/msix.h"
#include "hw/pci/pci_device.h"
-#include "hw/vfio/vfio-common.h"
#include "hw/vfio/vfio-container.h"
#include "qemu/error-report.h"
#include CONFIG_DEVICES /* CONFIG_VFIO_PCI */
@@ -86,7 +85,7 @@ static int vfio_eeh_container_op(VFIOContainer *container,
uint32_t op)

static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
{
-    VFIOAddressSpace *space = vfio_get_address_space(as);
+    VFIOAddressSpace *space = vfio_address_space_get(as);
     VFIOContainerBase *bcontainer = NULL;

     if (QLIST_EMPTY(&space->containers)) {
@@ -106,7 +105,7 @@ static VFIOContainer
*vfio_eeh_as_container(AddressSpace *as)
     }

out:
-    vfio_put_address_space(space);
+    vfio_address_space_put(space);
     return container_of(bcontainer, VFIOContainer, bcontainer);
}

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
0e3746eddd1c08e98bf57a59d542e158487d346e..08e2494d7c4a9858657724730
b2829290fb3f197 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -36,7 +36,6 @@
#include "qemu/main-loop.h"
#include "qemu/range.h"
#include "system/kvm.h"
-#include "system/reset.h"
#include "system/runstate.h"
#include "trace.h"
#include "qapi/error.h"
@@ -48,8 +47,6 @@

VFIODeviceList vfio_device_list =
     QLIST_HEAD_INITIALIZER(vfio_device_list);
-static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
-    QLIST_HEAD_INITIALIZER(vfio_address_spaces);

#ifdef CONFIG_KVM
/*
@@ -1304,24 +1301,6 @@ const MemoryListener vfio_memory_listener = {
     .log_sync = vfio_listener_log_sync,
};

-void vfio_reset_handler(void *opaque)
-{
-    VFIODevice *vbasedev;
-
-    trace_vfio_reset_handler();
-    QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) {
-        if (vbasedev->dev->realized) {
-            vbasedev->ops->vfio_compute_needs_reset(vbasedev);
-        }
-    }
-
-    QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) {
-        if (vbasedev->dev->realized && vbasedev->needs_reset) {
-            vbasedev->ops->vfio_hot_reset_multi(vbasedev);
-        }
-    }
-}
-
int vfio_kvm_device_add_fd(int fd, Error **errp)
{
#ifdef CONFIG_KVM
@@ -1380,51 +1359,6 @@ int vfio_kvm_device_del_fd(int fd, Error **errp)
     return 0;
}

-VFIOAddressSpace *vfio_get_address_space(AddressSpace *as)
-{
-    VFIOAddressSpace *space;
-
-    QLIST_FOREACH(space, &vfio_address_spaces, list) {
-        if (space->as == as) {
-            return space;
-        }
-    }
-
-    /* No suitable VFIOAddressSpace, create a new one */
-    space = g_malloc0(sizeof(*space));
-    space->as = as;
-    QLIST_INIT(&space->containers);
-
-    if (QLIST_EMPTY(&vfio_address_spaces)) {
-        qemu_register_reset(vfio_reset_handler, NULL);
-    }
-
-    QLIST_INSERT_HEAD(&vfio_address_spaces, space, list);
-
-    return space;
-}
-
-void vfio_put_address_space(VFIOAddressSpace *space)
-{
-    if (!QLIST_EMPTY(&space->containers)) {
-        return;
-    }
-
-    QLIST_REMOVE(space, list);
-    g_free(space);
-
-    if (QLIST_EMPTY(&vfio_address_spaces)) {
-        qemu_unregister_reset(vfio_reset_handler, NULL);
-    }
-}
-
-void vfio_address_space_insert(VFIOAddressSpace *space,
-                               VFIOContainerBase *bcontainer)
-{
-    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
-    bcontainer->space = space;
-}
-
struct vfio_device_info *vfio_get_device_info(int fd)
{
     struct vfio_device_info *info;
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index
749a3fd29dd6fc9143f14edf7e4ac6238315fcce..83e83ab9e67de8b004dfaf0067e
4c466a6c88451 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -13,7 +13,76 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
+#include "system/reset.h"
#include "hw/vfio/vfio-container-base.h"
+#include "hw/vfio/vfio-common.h" /* for vfio_device_list */
+#include "trace.h"
+
+static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
+    QLIST_HEAD_INITIALIZER(vfio_address_spaces);
+
+static void vfio_reset_handler(void *opaque)
+{
+    VFIODevice *vbasedev;
+
+    trace_vfio_reset_handler();
+    QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) {
+        if (vbasedev->dev->realized) {
+            vbasedev->ops->vfio_compute_needs_reset(vbasedev);
+        }
+    }
+
+    QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) {
+        if (vbasedev->dev->realized && vbasedev->needs_reset) {
+            vbasedev->ops->vfio_hot_reset_multi(vbasedev);
+        }
+    }
+}

This is not an address space scoped function, 

yep.

AIUI, pass-through devices of a VM are not necessarily in the same
group and we need to scan all groups/address_spaces when the machine
is reset.

There use to be a long comment explaining the context but we lost it
along the way. I will add it back :

  /*
   * We want to differentiate hot reset of mulitple in-use devices vs hot reset
   * of a single in-use device.  VFIO_DEVICE_RESET will already handle the case
   * of doing hot resets when there is only a single device per bus.  The in-use
   * here refers to how many VFIODevices are affected.  A hot reset that affects
   * multiple devices, but only a single in-use device, means that we can call
   * it from our bus ->reset() callback since the extent is effectively a single
   * device.  This allows us to make use of it in the hotplug path.  When there
   * are multiple in-use devices, we can only trigger the hot reset during a
   * system reset and thus from our reset handler.  We separate _one vs _multi
   * here so that we don't overlap and do a double reset on the system reset
   * path where both our reset handler and ->reset() callback are used.  Calling
   * _one() will only do a hot reset for the one in-use devices case, calling
   * _multi() will do nothing if a _one() would have been sufficient.
   */

See commit f16f39c3fc97 ("Implement PCI hot reset").

no sure if better to move to helper.c or common.c

This is a machine scope "helper" calling VFIODevice handlers, may be in
device.c  ?


Thanks,

C.


 
Thanks
Zhenzhong

+
+VFIOAddressSpace *vfio_address_space_get(AddressSpace *as)
+{
+    VFIOAddressSpace *space;
+
+    QLIST_FOREACH(space, &vfio_address_spaces, list) {
+        if (space->as == as) {
+            return space;
+        }
+    }
+
+    /* No suitable VFIOAddressSpace, create a new one */
+    space = g_malloc0(sizeof(*space));
+    space->as = as;
+    QLIST_INIT(&space->containers);
+
+    if (QLIST_EMPTY(&vfio_address_spaces)) {
+        qemu_register_reset(vfio_reset_handler, NULL);
+    }
+
+    QLIST_INSERT_HEAD(&vfio_address_spaces, space, list);
+
+    return space;
+}
+
+void vfio_address_space_put(VFIOAddressSpace *space)
+{
+    if (!QLIST_EMPTY(&space->containers)) {
+        return;
+    }
+
+    QLIST_REMOVE(space, list);
+    g_free(space);
+
+    if (QLIST_EMPTY(&vfio_address_spaces)) {
+        qemu_unregister_reset(vfio_reset_handler, NULL);
+    }
+}
+
+void vfio_address_space_insert(VFIOAddressSpace *space,
+                               VFIOContainerBase *bcontainer)
+{
+    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
+    bcontainer->space = space;
+}

int vfio_container_dma_map(VFIOContainerBase *bcontainer,
                            hwaddr iova, ram_addr_t size,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index
8badeb98ec052ad1fa7b5d45bb1733b1184bc6fb..9b86e24a4072e579bcdc2c060c
e42608ee44ee2e 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -546,7 +546,7 @@ static bool vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
     VFIOAddressSpace *space;
     VFIOIOMMUClass *vioc;

-    space = vfio_get_address_space(as);
+    space = vfio_address_space_get(as);

     /*
      * VFIO is currently incompatible with discarding of RAM insofar as the
@@ -675,7 +675,7 @@ close_fd_exit:
     close(fd);

put_space_exit:
-    vfio_put_address_space(space);
+    vfio_address_space_put(space);

     return false;
}
@@ -714,7 +714,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
         close(container->fd);
         object_unref(container);

-        vfio_put_address_space(space);
+        vfio_address_space_put(space);
     }
}

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index
a219b6453037e2d4e0d12800ea25678885af98f8..a170f5c71218db8c9b2f00b1a4
5ee900b6b21346 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -487,7 +487,7 @@ static bool iommufd_cdev_attach(const char *name,
VFIODevice *vbasedev,
         goto err_connect_bind;
     }

-    space = vfio_get_address_space(as);
+    space = vfio_address_space_get(as);

     /*
      * The HostIOMMUDevice data from legacy backend is static and doesn't need
@@ -607,7 +607,7 @@ err_discard_disable:
err_attach_container:
     iommufd_cdev_container_destroy(container);
err_alloc_ioas:
-    vfio_put_address_space(space);
+    vfio_address_space_put(space);
     iommufd_cdev_unbind_and_disconnect(vbasedev);
err_connect_bind:
     close(vbasedev->fd);
@@ -632,7 +632,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
     vfio_cpr_unregister_container(bcontainer);
     iommufd_cdev_detach_container(vbasedev, container);
     iommufd_cdev_container_destroy(container);
-    vfio_put_address_space(space);
+    vfio_address_space_put(space);

     iommufd_cdev_unbind_and_disconnect(vbasedev);
     close(vbasedev->fd);
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index
81f4130100c48012c15b5b4858446149a7eaf5b6..c3691c1a172c31c5b10bfd6967
c32fd32b65d0f7 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -109,7 +109,6 @@ vfio_get_dev_region(const char *name, int index,
uint32_t type, uint32_t subtype
vfio_legacy_dma_unmap_overflow_workaround(void) ""
vfio_get_dirty_bitmap(uint64_t iova, uint64_t size, uint64_t bitmap_size,
uint64_t start, uint64_t dirty_pages) "iova=0x%"PRIx64" size= 0x%"PRIx64"
bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty_pages=%"PRIu64
vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu
dirty @ 0x%"PRIx64" - 0x%"PRIx64
-vfio_reset_handler(void) ""

# region.c
vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data,
unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
@@ -194,3 +193,6 @@ iommufd_cdev_fail_attach_existing_container(const
char *msg) " %s"
iommufd_cdev_alloc_ioas(int iommufd, int ioas_id) " [iommufd=%d] new
IOMMUFD container with ioasid=%d"
iommufd_cdev_device_info(char *name, int devfd, int num_irqs, int
num_regions, int flags) " %s (%d) num_irqs=%d num_regions=%d flags=%d"
iommufd_cdev_pci_hot_reset_dep_devices(int domain, int bus, int slot, int
function, int dev_id) "\t%04x:%02x:%02x.%x devid %d"
+
+# container-base.c
+vfio_reset_handler(void) ""
--
2.48.1