>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Subject: Re: [PATCH for-10.1 13/32] vfio: Move VFIOAddressSpace helpers into
>container-base.c
>
>+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.
Yes, before introducing vfio_device_list, we scan vfio_group_list.
>
>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 ?
Sounds good.
Thanks
Zhenzhong
