From 001a013ea3f125d2ec0e709b5765754149d8d968 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:16 +0100 Subject: [PATCH 01/17] vfio/spapr: Extend VFIOIOMMUOps with a release handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows to abstract a bit more the sPAPR IOMMU support in the legacy IOMMU backend. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 10 +++----- hw/vfio/spapr.c | 35 +++++++++++++++------------ include/hw/vfio/vfio-container-base.h | 1 + 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index b22feb8ded..1e77a2929e 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -632,9 +632,8 @@ listener_release_exit: QLIST_REMOVE(bcontainer, next); vfio_kvm_device_del_group(group); memory_listener_unregister(&bcontainer->listener); - if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU || - container->iommu_type == VFIO_SPAPR_TCE_IOMMU) { - vfio_spapr_container_deinit(container); + if (bcontainer->ops->release) { + bcontainer->ops->release(bcontainer); } enable_discards_exit: @@ -667,9 +666,8 @@ static void vfio_disconnect_container(VFIOGroup *group) */ if (QLIST_EMPTY(&container->group_list)) { memory_listener_unregister(&bcontainer->listener); - if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU || - container->iommu_type == VFIO_SPAPR_TCE_IOMMU) { - vfio_spapr_container_deinit(container); + if (bcontainer->ops->release) { + bcontainer->ops->release(bcontainer); } } diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 5c6426e697..44617dfc6b 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -440,6 +440,24 @@ vfio_spapr_container_del_section_window(VFIOContainerBase *bcontainer, } } +static void vfio_spapr_container_release(VFIOContainerBase *bcontainer) +{ + VFIOContainer *container = container_of(bcontainer, VFIOContainer, + bcontainer); + VFIOSpaprContainer *scontainer = container_of(container, VFIOSpaprContainer, + container); + VFIOHostDMAWindow *hostwin, *next; + + if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { + memory_listener_unregister(&scontainer->prereg_listener); + } + QLIST_FOREACH_SAFE(hostwin, &scontainer->hostwin_list, hostwin_next, + next) { + QLIST_REMOVE(hostwin, hostwin_next); + g_free(hostwin); + } +} + static VFIOIOMMUOps vfio_iommu_spapr_ops; static void setup_spapr_ops(VFIOContainerBase *bcontainer) @@ -447,6 +465,7 @@ static void setup_spapr_ops(VFIOContainerBase *bcontainer) vfio_iommu_spapr_ops = *bcontainer->ops; vfio_iommu_spapr_ops.add_window = vfio_spapr_container_add_section_window; vfio_iommu_spapr_ops.del_window = vfio_spapr_container_del_section_window; + vfio_iommu_spapr_ops.release = vfio_spapr_container_release; bcontainer->ops = &vfio_iommu_spapr_ops; } @@ -527,19 +546,3 @@ listener_unregister_exit: } return ret; } - -void vfio_spapr_container_deinit(VFIOContainer *container) -{ - VFIOSpaprContainer *scontainer = container_of(container, VFIOSpaprContainer, - container); - VFIOHostDMAWindow *hostwin, *next; - - if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { - memory_listener_unregister(&scontainer->prereg_listener); - } - QLIST_FOREACH_SAFE(hostwin, &scontainer->hostwin_list, hostwin_next, - next) { - QLIST_REMOVE(hostwin, hostwin_next); - g_free(hostwin); - } -} diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 2ae297ccda..5c9594b6c7 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -117,5 +117,6 @@ struct VFIOIOMMUOps { Error **errp); void (*del_window)(VFIOContainerBase *bcontainer, MemoryRegionSection *section); + void (*release)(VFIOContainerBase *bcontainer); }; #endif /* HW_VFIO_VFIO_CONTAINER_BASE_H */ From d3764db87531cd53849ccee9b2f72aede90ccf5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:17 +0100 Subject: [PATCH 02/17] vfio/container: Introduce vfio_legacy_setup() for further cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will help subsequent patches to unify the initialization of type1 and sPAPR IOMMU backends. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 63 +++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 1e77a2929e..afcfe80488 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -474,6 +474,35 @@ static void vfio_get_iommu_info_migration(VFIOContainer *container, } } +static int vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp) +{ + VFIOContainer *container = container_of(bcontainer, VFIOContainer, + bcontainer); + g_autofree struct vfio_iommu_type1_info *info = NULL; + int ret; + + ret = vfio_get_iommu_info(container, &info); + if (ret) { + error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); + return ret; + } + + if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { + bcontainer->pgsizes = info->iova_pgsizes; + } else { + bcontainer->pgsizes = qemu_real_host_page_size(); + } + + if (!vfio_get_info_dma_avail(info, &bcontainer->dma_max_mappings)) { + bcontainer->dma_max_mappings = 65535; + } + + vfio_get_info_iova_range(info, bcontainer); + + vfio_get_iommu_info_migration(container, info); + return 0; +} + static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, Error **errp) { @@ -570,40 +599,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, switch (container->iommu_type) { case VFIO_TYPE1v2_IOMMU: case VFIO_TYPE1_IOMMU: - { - struct vfio_iommu_type1_info *info; - - ret = vfio_get_iommu_info(container, &info); - if (ret) { - error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); - goto enable_discards_exit; - } - - if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { - bcontainer->pgsizes = info->iova_pgsizes; - } else { - bcontainer->pgsizes = qemu_real_host_page_size(); - } - - if (!vfio_get_info_dma_avail(info, &bcontainer->dma_max_mappings)) { - bcontainer->dma_max_mappings = 65535; - } - - vfio_get_info_iova_range(info, bcontainer); - - vfio_get_iommu_info_migration(container, info); - g_free(info); + ret = vfio_legacy_setup(bcontainer, errp); break; - } case VFIO_SPAPR_TCE_v2_IOMMU: case VFIO_SPAPR_TCE_IOMMU: - { ret = vfio_spapr_container_init(container, errp); - if (ret) { - goto enable_discards_exit; - } break; + default: + g_assert_not_reached(); } + + if (ret) { + goto enable_discards_exit; } vfio_kvm_device_add_group(group); From bffe92af0e7571868d47a1d1cd2205e13054d492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:18 +0100 Subject: [PATCH 03/17] vfio/container: Initialize VFIOIOMMUOps under vfio_init_container() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vfio_init_container() already defines the IOMMU type of the container. Do the same for the VFIOIOMMUOps struct. This prepares ground for the following patches that will deduce the associated VFIOIOMMUOps struct from the IOMMU type. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index afcfe80488..f4a0434a52 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -370,7 +370,7 @@ static int vfio_get_iommu_type(VFIOContainer *container, } static int vfio_init_container(VFIOContainer *container, int group_fd, - Error **errp) + VFIOAddressSpace *space, Error **errp) { int iommu_type, ret; @@ -401,6 +401,7 @@ static int vfio_init_container(VFIOContainer *container, int group_fd, } container->iommu_type = iommu_type; + vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops); return 0; } @@ -583,9 +584,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, container = g_malloc0(sizeof(*container)); container->fd = fd; bcontainer = &container->bcontainer; - vfio_container_init(bcontainer, space, &vfio_legacy_ops); - ret = vfio_init_container(container, group->fd, errp); + ret = vfio_init_container(container, group->fd, space, errp); if (ret) { goto free_container_exit; } From fdaa774e67435a328c0e28006c4d749f2198294a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:19 +0100 Subject: [PATCH 04/17] vfio/container: Introduce a VFIOIOMMU QOM interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VFIOContainerBase was not introduced as an abstract QOM object because it felt unnecessary to expose all the IOMMU backends to the QEMU machine and human interface. However, we can still abstract the IOMMU backend handlers using a QOM interface class. This provides more flexibility when referencing the various implementations. Simply transform the VFIOIOMMUOps struct in an InterfaceClass and do some initial name replacements. Next changes will start converting VFIOIOMMUOps. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 2 +- hw/vfio/container-base.c | 12 +++++++++++- hw/vfio/pci.c | 2 +- include/hw/vfio/vfio-container-base.h | 23 +++++++++++++++++++---- 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 08a3e57672..49dab41566 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1503,7 +1503,7 @@ retry: int vfio_attach_device(char *name, VFIODevice *vbasedev, AddressSpace *as, Error **errp) { - const VFIOIOMMUOps *ops = &vfio_legacy_ops; + const VFIOIOMMUClass *ops = &vfio_legacy_ops; #ifdef CONFIG_IOMMUFD if (vbasedev->iommufd) { diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 1ffd25bbfa..913ae49077 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -72,7 +72,7 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, } void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, - const VFIOIOMMUOps *ops) + const VFIOIOMMUClass *ops) { bcontainer->ops = ops; bcontainer->space = space; @@ -99,3 +99,13 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer) g_list_free_full(bcontainer->iova_ranges, g_free); } + +static const TypeInfo types[] = { + { + .name = TYPE_VFIO_IOMMU, + .parent = TYPE_INTERFACE, + .class_size = sizeof(VFIOIOMMUClass), + }, +}; + +DEFINE_TYPES(types) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 9f838978be..d7fe06715c 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2488,7 +2488,7 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev, static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) { VFIODevice *vbasedev = &vdev->vbasedev; - const VFIOIOMMUOps *ops = vbasedev->bcontainer->ops; + const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops; return ops->pci_hot_reset(vbasedev, single); } diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 5c9594b6c7..d6147b4aee 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -16,7 +16,8 @@ #include "exec/memory.h" typedef struct VFIODevice VFIODevice; -typedef struct VFIOIOMMUOps VFIOIOMMUOps; +typedef struct VFIOIOMMUClass VFIOIOMMUClass; +#define VFIOIOMMUOps VFIOIOMMUClass /* To remove */ typedef struct { unsigned long *bitmap; @@ -34,7 +35,7 @@ typedef struct VFIOAddressSpace { * This is the base object for vfio container backends */ typedef struct VFIOContainerBase { - const VFIOIOMMUOps *ops; + const VFIOIOMMUClass *ops; VFIOAddressSpace *space; MemoryListener listener; Error *error; @@ -88,10 +89,24 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, - const VFIOIOMMUOps *ops); + const VFIOIOMMUClass *ops); void vfio_container_destroy(VFIOContainerBase *bcontainer); -struct VFIOIOMMUOps { + +#define TYPE_VFIO_IOMMU "vfio-iommu" + +/* + * VFIOContainerBase is not an abstract QOM object because it felt + * unnecessary to expose all the IOMMU backends to the QEMU machine + * and human interface. However, we can still abstract the IOMMU + * backend handlers using a QOM interface class. This provides more + * flexibility when referencing the various implementations. + */ +DECLARE_CLASS_CHECKERS(VFIOIOMMUClass, VFIO_IOMMU, TYPE_VFIO_IOMMU) + +struct VFIOIOMMUClass { + InterfaceClass parent_class; + /* basic feature */ int (*dma_map)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, From 9812feefab3a4ff95a6cfd73aecb120b406bc98c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:20 +0100 Subject: [PATCH 05/17] vfio/container: Introduce a VFIOIOMMU legacy QOM interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert the legacy VFIOIOMMUOps struct to the new VFIOIOMMU QOM interface. The set of of operations for this backend can be referenced with a literal typename instead of a C struct. This will simplify support of multiple backends. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 6 ++- hw/vfio/container.c | 58 ++++++++++++++++++++++----- include/hw/vfio/vfio-common.h | 1 - include/hw/vfio/vfio-container-base.h | 1 + 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 49dab41566..2329d0efc8 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1503,13 +1503,17 @@ retry: int vfio_attach_device(char *name, VFIODevice *vbasedev, AddressSpace *as, Error **errp) { - const VFIOIOMMUClass *ops = &vfio_legacy_ops; + const VFIOIOMMUClass *ops = + VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); #ifdef CONFIG_IOMMUFD if (vbasedev->iommufd) { ops = &vfio_iommufd_ops; } #endif + + assert(ops); + return ops->attach_device(name, vbasedev, as, errp); } diff --git a/hw/vfio/container.c b/hw/vfio/container.c index f4a0434a52..220e838a91 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -369,10 +369,30 @@ static int vfio_get_iommu_type(VFIOContainer *container, return -EINVAL; } +/* + * vfio_get_iommu_ops - get a VFIOIOMMUClass associated with a type + */ +static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp) +{ + ObjectClass *klass = NULL; + + switch (iommu_type) { + case VFIO_TYPE1v2_IOMMU: + case VFIO_TYPE1_IOMMU: + klass = object_class_by_name(TYPE_VFIO_IOMMU_LEGACY); + break; + default: + g_assert_not_reached(); + }; + + return VFIO_IOMMU_CLASS(klass); +} + static int vfio_init_container(VFIOContainer *container, int group_fd, VFIOAddressSpace *space, Error **errp) { int iommu_type, ret; + const VFIOIOMMUClass *vioc; iommu_type = vfio_get_iommu_type(container, errp); if (iommu_type < 0) { @@ -401,7 +421,14 @@ static int vfio_init_container(VFIOContainer *container, int group_fd, } container->iommu_type = iommu_type; - vfio_container_init(&container->bcontainer, space, &vfio_legacy_ops); + + vioc = vfio_get_iommu_class(iommu_type, errp); + if (!vioc) { + error_setg(errp, "No available IOMMU models"); + return -EINVAL; + } + + vfio_container_init(&container->bcontainer, space, vioc); return 0; } @@ -1098,12 +1125,25 @@ out_single: return ret; } -const VFIOIOMMUOps vfio_legacy_ops = { - .dma_map = vfio_legacy_dma_map, - .dma_unmap = vfio_legacy_dma_unmap, - .attach_device = vfio_legacy_attach_device, - .detach_device = vfio_legacy_detach_device, - .set_dirty_page_tracking = vfio_legacy_set_dirty_page_tracking, - .query_dirty_bitmap = vfio_legacy_query_dirty_bitmap, - .pci_hot_reset = vfio_legacy_pci_hot_reset, +static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) +{ + VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass); + + vioc->dma_map = vfio_legacy_dma_map; + vioc->dma_unmap = vfio_legacy_dma_unmap; + vioc->attach_device = vfio_legacy_attach_device; + vioc->detach_device = vfio_legacy_detach_device; + vioc->set_dirty_page_tracking = vfio_legacy_set_dirty_page_tracking; + vioc->query_dirty_bitmap = vfio_legacy_query_dirty_bitmap; + vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; }; + +static const TypeInfo types[] = { + { + .name = TYPE_VFIO_IOMMU_LEGACY, + .parent = TYPE_VFIO_IOMMU, + .class_init = vfio_iommu_legacy_class_init, + }, +}; + +DEFINE_TYPES(types) diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b8aa8a5495..14c497b6b0 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -210,7 +210,6 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; extern VFIOGroupList vfio_group_list; extern VFIODeviceList vfio_device_list; -extern const VFIOIOMMUOps vfio_legacy_ops; extern const VFIOIOMMUOps vfio_iommufd_ops; extern const MemoryListener vfio_memory_listener; extern int vfio_kvm_device_fd; diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index d6147b4aee..c60370fc5e 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -94,6 +94,7 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer); #define TYPE_VFIO_IOMMU "vfio-iommu" +#define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy" /* * VFIOContainerBase is not an abstract QOM object because it felt From 61d893f2cdb34a2b0255f9b5fbba6b49b94ff730 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:21 +0100 Subject: [PATCH 06/17] vfio/container: Intoduce a new VFIOIOMMUClass::setup handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will help in converting the sPAPR IOMMU backend to a QOM interface. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 1 + include/hw/vfio/vfio-container-base.h | 1 + 2 files changed, 2 insertions(+) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 220e838a91..c22bdd3216 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -1129,6 +1129,7 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) { VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass); + vioc->setup = vfio_legacy_setup; vioc->dma_map = vfio_legacy_dma_map; vioc->dma_unmap = vfio_legacy_dma_unmap; vioc->attach_device = vfio_legacy_attach_device; diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index c60370fc5e..ce8b1fba88 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -109,6 +109,7 @@ struct VFIOIOMMUClass { InterfaceClass parent_class; /* basic feature */ + int (*setup)(VFIOContainerBase *bcontainer, Error **errp); int (*dma_map)(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, void *vaddr, bool readonly); From f221f641a2fe69c2ca3857759551470664b0bec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:22 +0100 Subject: [PATCH 07/17] vfio/spapr: Introduce a sPAPR VFIOIOMMU QOM interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move vfio_spapr_container_setup() to a VFIOIOMMUClass::setup handler and convert the sPAPR VFIOIOMMUOps struct to a QOM interface. The sPAPR QOM interface inherits from the legacy QOM interface because because both have the same basic needs. The sPAPR interface is then extended with the handlers specific to the sPAPR IOMMU. This allows reuse and provides better abstraction of the backends. It will be useful to avoid compiling the sPAPR IOMMU backend on targets not supporting it. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 18 +++++-------- hw/vfio/spapr.c | 39 ++++++++++++++++----------- include/hw/vfio/vfio-container-base.h | 1 + 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index c22bdd3216..688cf23bab 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -381,6 +381,10 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp) case VFIO_TYPE1_IOMMU: klass = object_class_by_name(TYPE_VFIO_IOMMU_LEGACY); break; + case VFIO_SPAPR_TCE_v2_IOMMU: + case VFIO_SPAPR_TCE_IOMMU: + klass = object_class_by_name(TYPE_VFIO_IOMMU_SPAPR); + break; default: g_assert_not_reached(); }; @@ -623,19 +627,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, goto free_container_exit; } - switch (container->iommu_type) { - case VFIO_TYPE1v2_IOMMU: - case VFIO_TYPE1_IOMMU: - ret = vfio_legacy_setup(bcontainer, errp); - break; - case VFIO_SPAPR_TCE_v2_IOMMU: - case VFIO_SPAPR_TCE_IOMMU: - ret = vfio_spapr_container_init(container, errp); - break; - default: - g_assert_not_reached(); - } + assert(bcontainer->ops->setup); + ret = bcontainer->ops->setup(bcontainer, errp); if (ret) { goto enable_discards_exit; } diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 44617dfc6b..0d949bb728 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -458,20 +458,11 @@ static void vfio_spapr_container_release(VFIOContainerBase *bcontainer) } } -static VFIOIOMMUOps vfio_iommu_spapr_ops; - -static void setup_spapr_ops(VFIOContainerBase *bcontainer) +static int vfio_spapr_container_setup(VFIOContainerBase *bcontainer, + Error **errp) { - vfio_iommu_spapr_ops = *bcontainer->ops; - vfio_iommu_spapr_ops.add_window = vfio_spapr_container_add_section_window; - vfio_iommu_spapr_ops.del_window = vfio_spapr_container_del_section_window; - vfio_iommu_spapr_ops.release = vfio_spapr_container_release; - bcontainer->ops = &vfio_iommu_spapr_ops; -} - -int vfio_spapr_container_init(VFIOContainer *container, Error **errp) -{ - VFIOContainerBase *bcontainer = &container->bcontainer; + VFIOContainer *container = container_of(bcontainer, VFIOContainer, + bcontainer); VFIOSpaprContainer *scontainer = container_of(container, VFIOSpaprContainer, container); struct vfio_iommu_spapr_tce_info info; @@ -536,8 +527,6 @@ int vfio_spapr_container_init(VFIOContainer *container, Error **errp) 0x1000); } - setup_spapr_ops(bcontainer); - return 0; listener_unregister_exit: @@ -546,3 +535,23 @@ listener_unregister_exit: } return ret; } + +static void vfio_iommu_spapr_class_init(ObjectClass *klass, void *data) +{ + VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass); + + vioc->add_window = vfio_spapr_container_add_section_window; + vioc->del_window = vfio_spapr_container_del_section_window; + vioc->release = vfio_spapr_container_release; + vioc->setup = vfio_spapr_container_setup; +}; + +static const TypeInfo types[] = { + { + .name = TYPE_VFIO_IOMMU_SPAPR, + .parent = TYPE_VFIO_IOMMU_LEGACY, + .class_init = vfio_iommu_spapr_class_init, + }, +}; + +DEFINE_TYPES(types) diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index ce8b1fba88..9e21d7811f 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -95,6 +95,7 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer); #define TYPE_VFIO_IOMMU "vfio-iommu" #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy" +#define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr" /* * VFIOContainerBase is not an abstract QOM object because it felt From ce5f6d49f5845c3b9955cc377a5223c3f8d7ba1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:23 +0100 Subject: [PATCH 08/17] vfio/iommufd: Introduce a VFIOIOMMU iommufd QOM interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As previously done for the sPAPR and legacy IOMMU backends, convert the VFIOIOMMUOps struct to a QOM interface. The set of of operations for this backend can be referenced with a literal typename instead of a C struct. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 2 +- hw/vfio/iommufd.c | 35 ++++++++++++++++++++------- include/hw/vfio/vfio-common.h | 1 - include/hw/vfio/vfio-container-base.h | 2 +- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 2329d0efc8..89ff1c7aed 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1508,7 +1508,7 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, #ifdef CONFIG_IOMMUFD if (vbasedev->iommufd) { - ops = &vfio_iommufd_ops; + ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); } #endif diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 87a561c545..d4c586e842 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -319,6 +319,8 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, int ret, devfd; uint32_t ioas_id; Error *err = NULL; + const VFIOIOMMUClass *iommufd_vioc = + VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); if (vbasedev->fd < 0) { devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp); @@ -340,7 +342,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, /* try to attach to an existing container in this space */ QLIST_FOREACH(bcontainer, &space->containers, next) { container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer); - if (bcontainer->ops != &vfio_iommufd_ops || + if (bcontainer->ops != iommufd_vioc || vbasedev->iommufd != container->be) { continue; } @@ -374,7 +376,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev, container->ioas_id = ioas_id; bcontainer = &container->bcontainer; - vfio_container_init(bcontainer, space, &vfio_iommufd_ops); + vfio_container_init(bcontainer, space, iommufd_vioc); QLIST_INSERT_HEAD(&space->containers, bcontainer, next); ret = iommufd_cdev_attach_container(vbasedev, container, errp); @@ -476,9 +478,11 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev) static VFIODevice *iommufd_cdev_pci_find_by_devid(__u32 devid) { VFIODevice *vbasedev_iter; + const VFIOIOMMUClass *iommufd_vioc = + VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); QLIST_FOREACH(vbasedev_iter, &vfio_device_list, global_next) { - if (vbasedev_iter->bcontainer->ops != &vfio_iommufd_ops) { + if (vbasedev_iter->bcontainer->ops != iommufd_vioc) { continue; } if (devid == vbasedev_iter->devid) { @@ -621,10 +625,23 @@ out_single: return ret; } -const VFIOIOMMUOps vfio_iommufd_ops = { - .dma_map = iommufd_cdev_map, - .dma_unmap = iommufd_cdev_unmap, - .attach_device = iommufd_cdev_attach, - .detach_device = iommufd_cdev_detach, - .pci_hot_reset = iommufd_cdev_pci_hot_reset, +static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) +{ + VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass); + + vioc->dma_map = iommufd_cdev_map; + vioc->dma_unmap = iommufd_cdev_unmap; + vioc->attach_device = iommufd_cdev_attach; + vioc->detach_device = iommufd_cdev_detach; + vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; }; + +static const TypeInfo types[] = { + { + .name = TYPE_VFIO_IOMMU_IOMMUFD, + .parent = TYPE_VFIO_IOMMU, + .class_init = vfio_iommu_iommufd_class_init, + }, +}; + +DEFINE_TYPES(types) diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 14c497b6b0..9b7ef7d02b 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -210,7 +210,6 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList; typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList; extern VFIOGroupList vfio_group_list; extern VFIODeviceList vfio_device_list; -extern const VFIOIOMMUOps vfio_iommufd_ops; extern const MemoryListener vfio_memory_listener; extern int vfio_kvm_device_fd; diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 9e21d7811f..b2813b0c11 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -17,7 +17,6 @@ typedef struct VFIODevice VFIODevice; typedef struct VFIOIOMMUClass VFIOIOMMUClass; -#define VFIOIOMMUOps VFIOIOMMUClass /* To remove */ typedef struct { unsigned long *bitmap; @@ -96,6 +95,7 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer); #define TYPE_VFIO_IOMMU "vfio-iommu" #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy" #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr" +#define TYPE_VFIO_IOMMU_IOMMUFD TYPE_VFIO_IOMMU "-iommufd" /* * VFIOContainerBase is not an abstract QOM object because it felt From 10164df6ed3d41cbf67105dcd954a663ef4cc3e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:24 +0100 Subject: [PATCH 09/17] vfio/spapr: Only compile sPAPR IOMMU support when needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sPAPR IOMMU support is only needed for pseries machines. Compile out support when CONFIG_PSERIES is not set. This saves ~7K of text. Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build index e5d98b6adc..bb98493b53 100644 --- a/hw/vfio/meson.build +++ b/hw/vfio/meson.build @@ -4,9 +4,9 @@ vfio_ss.add(files( 'common.c', 'container-base.c', 'container.c', - 'spapr.c', 'migration.c', )) +vfio_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr.c')) vfio_ss.add(when: 'CONFIG_IOMMUFD', if_true: files( 'iommufd.c', )) From c1139fa4feba8c320e4bd0a4e34af55caa5ffbb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 19 Dec 2023 07:58:25 +0100 Subject: [PATCH 10/17] vfio/iommufd: Remove CONFIG_IOMMUFD usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Availability of the IOMMUFD backend can now be fully determined at runtime and the ifdef check was a build time protection (for PPC not supporting it mostly). Reviewed-by: Zhenzhong Duan Tested-by: Eric Farman Signed-off-by: Cédric Le Goater --- hw/vfio/common.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 89ff1c7aed..0d4d8b8416 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -19,7 +19,6 @@ */ #include "qemu/osdep.h" -#include CONFIG_DEVICES /* CONFIG_IOMMUFD */ #include #ifdef CONFIG_KVM #include @@ -1506,11 +1505,9 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev, const VFIOIOMMUClass *ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY)); -#ifdef CONFIG_IOMMUFD if (vbasedev->iommufd) { ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)); } -#endif assert(ops); From 213ae3ffda463c0503e39e0cf827511b5298c314 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Wed, 20 Dec 2023 14:53:02 +0100 Subject: [PATCH 11/17] vfio/container: Replace basename with g_path_get_basename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit g_path_get_basename() is a portable utility function that has the advantage of not modifing the string argument. It also fixes a compile breakage with the Musl C library reported in [1]. [1] https://lore.kernel.org/all/20231212010228.2701544-1-raj.khem@gmail.com/ Reported-by: Khem Raj Reviewed-by: Eric Auger Reviewed-by: Zhao Liu Reviewed-by: Zhenzhong Duan Signed-off-by: Cédric Le Goater --- hw/vfio/container.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 688cf23bab..8d334f52f2 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -869,7 +869,8 @@ static void vfio_put_base_device(VFIODevice *vbasedev) static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) { - char *tmp, group_path[PATH_MAX], *group_name; + char *tmp, group_path[PATH_MAX]; + g_autofree char *group_name = NULL; int ret, groupid; ssize_t len; @@ -885,7 +886,7 @@ static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) group_path[len] = 0; - group_name = basename(group_path); + group_name = g_path_get_basename(group_path); if (sscanf(group_name, "%d", &groupid) != 1) { error_setg_errno(errp, errno, "failed to read %s", group_path); return -errno; From 9353b6da430f90e47f352dbf6dc31120c8914da6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volker=20R=C3=BCmelin?= Date: Fri, 29 Dec 2023 21:38:54 +0100 Subject: [PATCH 12/17] hw/vfio: fix iteration over global VFIODevice list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 3d779abafe ("vfio/common: Introduce a global VFIODevice list") introduced a global VFIODevice list, but forgot to update the list element field name when iterating over the new list. Change the code to use the correct list element field. Fixes: 3d779abafe ("vfio/common: Introduce a global VFIODevice list") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2061 Signed-off-by: Volker Rümelin Reviewed-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater Reviewed-by: Eric Auger --- hw/vfio/common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 0d4d8b8416..0b3352f2a9 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -73,7 +73,7 @@ bool vfio_mig_active(void) return false; } - QLIST_FOREACH(vbasedev, &vfio_device_list, next) { + QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) { if (vbasedev->migration_blocker) { return false; } @@ -94,7 +94,7 @@ static bool vfio_multiple_devices_migration_is_supported(void) unsigned int device_num = 0; bool all_support_p2p = true; - QLIST_FOREACH(vbasedev, &vfio_device_list, next) { + QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) { if (vbasedev->migration) { device_num++; @@ -1366,13 +1366,13 @@ void vfio_reset_handler(void *opaque) { VFIODevice *vbasedev; - QLIST_FOREACH(vbasedev, &vfio_device_list, next) { + 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, next) { + QLIST_FOREACH(vbasedev, &vfio_device_list, global_next) { if (vbasedev->dev->realized && vbasedev->needs_reset) { vbasedev->ops->vfio_hot_reset_multi(vbasedev); } From 6ba254801f6bc7f3ef68a6414f1b107237c7eb26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 21 Dec 2023 09:09:57 +0100 Subject: [PATCH 13/17] vfio/iommufd: Remove the use of stat() to check file existence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using stat() before opening a file or a directory can lead to a time-of-check to time-of-use (TOCTOU) filesystem race, which is reported by coverity as a Security best practices violations. The sequence could be replaced by open and fdopendir but it doesn't add much in this case. Simply use opendir to avoid the race. Fixes: CID 1531551 Signed-off-by: Cédric Le Goater Reviewed-by: Zhenzhong Duan --- hw/vfio/iommufd.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index d4c586e842..9bfddc1360 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -121,17 +121,11 @@ static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp) DIR *dir = NULL; struct dirent *dent; gchar *contents; - struct stat st; gsize length; int major, minor; dev_t vfio_devt; path = g_strdup_printf("%s/vfio-dev", sysfs_path); - if (stat(path, &st) < 0) { - error_setg_errno(errp, errno, "no such host device"); - goto out_free_path; - } - dir = opendir(path); if (!dir) { error_setg_errno(errp, errno, "couldn't open directory %s", path); From 9f734a117cbf63b03577b46c8cad8ad88ec6dced Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan Date: Thu, 21 Dec 2023 10:45:17 +0800 Subject: [PATCH 14/17] vfio/container: Rename vfio_init_container to vfio_set_iommu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vfio_container_init() and vfio_init_container() names are confusing especially when we see vfio_init_container() calls vfio_container_init(). vfio_container_init() operates on base container which is consistent with all routines handling 'VFIOContainerBase *' ops. vfio_init_container() operates on legacy container and setup IOMMU context with ioctl(VFIO_SET_IOMMU). So choose to rename vfio_init_container to vfio_set_iommu to avoid the confusion. No functional change intended. Suggested-by: Cédric Le Goater Signed-off-by: Zhenzhong Duan Reviewed-by: Cédric Le Goater --- hw/vfio/container.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 8d334f52f2..bd25b9fbad 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -392,8 +392,8 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp) return VFIO_IOMMU_CLASS(klass); } -static int vfio_init_container(VFIOContainer *container, int group_fd, - VFIOAddressSpace *space, Error **errp) +static int vfio_set_iommu(VFIOContainer *container, int group_fd, + VFIOAddressSpace *space, Error **errp) { int iommu_type, ret; const VFIOIOMMUClass *vioc; @@ -616,7 +616,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, container->fd = fd; bcontainer = &container->bcontainer; - ret = vfio_init_container(container, group->fd, space, errp); + ret = vfio_set_iommu(container, group->fd, space, errp); if (ret) { goto free_container_exit; } From c817e5a377a334241eed149e35760aca58bdeb34 Mon Sep 17 00:00:00 2001 From: Avihai Horon Date: Sun, 31 Dec 2023 12:48:18 +0200 Subject: [PATCH 15/17] vfio/migration: Add helper function to set state or reset device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are several places where failure in setting the device state leads to a device reset, which is done by setting ERROR as the recover state. Add a helper function that sets the device state and resets the device in case of failure. This will make the code cleaner and remove duplicate comments. Signed-off-by: Avihai Horon Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé --- hw/vfio/migration.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 28d422b39f..70e6b1a709 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -163,6 +163,19 @@ reset_device: return ret; } +/* + * Some device state transitions require resetting the device if they fail. + * This function sets the device in new_state and resets the device if that + * fails. Reset is done by using ERROR as the recover state. + */ +static int +vfio_migration_set_state_or_reset(VFIODevice *vbasedev, + enum vfio_device_mig_state new_state) +{ + return vfio_migration_set_state(vbasedev, new_state, + VFIO_DEVICE_STATE_ERROR); +} + static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t data_size) { @@ -422,12 +435,7 @@ static void vfio_save_cleanup(void *opaque) * after migration has completed, so it won't increase downtime. */ if (migration->device_state == VFIO_DEVICE_STATE_STOP_COPY) { - /* - * If setting the device in STOP state fails, the device should be - * reset. To do so, use ERROR state as a recover state. - */ - vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, - VFIO_DEVICE_STATE_ERROR); + vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP); } g_free(migration->data_buffer); @@ -699,12 +707,7 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running, VFIO_DEVICE_STATE_PRE_COPY_P2P : VFIO_DEVICE_STATE_RUNNING_P2P; - /* - * If setting the device in new_state fails, the device should be reset. - * To do so, use ERROR state as a recover state. - */ - ret = vfio_migration_set_state(vbasedev, new_state, - VFIO_DEVICE_STATE_ERROR); + ret = vfio_migration_set_state_or_reset(vbasedev, new_state); if (ret) { /* * Migration should be aborted in this case, but vm_state_notify() @@ -736,12 +739,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) VFIO_DEVICE_STATE_STOP; } - /* - * If setting the device in new_state fails, the device should be reset. - * To do so, use ERROR state as a recover state. - */ - ret = vfio_migration_set_state(vbasedev, new_state, - VFIO_DEVICE_STATE_ERROR); + ret = vfio_migration_set_state_or_reset(vbasedev, new_state); if (ret) { /* * Migration should be aborted in this case, but vm_state_notify() @@ -770,12 +768,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data) case MIGRATION_STATUS_CANCELLING: case MIGRATION_STATUS_CANCELLED: case MIGRATION_STATUS_FAILED: - /* - * If setting the device in RUNNING state fails, the device should - * be reset. To do so, use ERROR state as a recover state. - */ - vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING, - VFIO_DEVICE_STATE_ERROR); + vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING); } } From c2ab3a6f7411c895e538e8350fee8948ac07c1a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Fri, 22 Dec 2023 08:55:23 +0100 Subject: [PATCH 16/17] backends/iommufd: Remove check on number of backend users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QOM already has a ref count on objects and it will assert much earlier, when INT_MAX is reached. Reviewed-by: Eric Auger Reviewed-by: Zhenzhong Duan Signed-off-by: Cédric Le Goater --- backends/iommufd.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/backends/iommufd.c b/backends/iommufd.c index ba58a0eb0d..393c0d9a37 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -80,11 +80,6 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp) int fd, ret = 0; qemu_mutex_lock(&be->lock); - if (be->users == UINT32_MAX) { - error_setg(errp, "too many connections"); - ret = -E2BIG; - goto out; - } if (be->owned && !be->users) { fd = qemu_open_old("/dev/iommu", O_RDWR); if (fd < 0) { From 19368b1905b4b917e915526fcbd5bfa3f7439451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Thu, 21 Dec 2023 16:58:41 +0100 Subject: [PATCH 17/17] backends/iommufd: Remove mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity reports a concurrent data access violation because be->users is being accessed in iommufd_backend_can_be_deleted() without holding the mutex. However, these routines are called from the QEMU main thread when a device is created. In this case, the code paths should be protected by the BQL lock and it should be safe to drop the IOMMUFD backend mutex. Simply remove it. Fixes: CID 1531550 Fixes: CID 1531549 Reviewed-by: Eric Auger Reviewed-by: Zhenzhong Duan Signed-off-by: Cédric Le Goater --- backends/iommufd.c | 7 ------- include/sysemu/iommufd.h | 2 -- 2 files changed, 9 deletions(-) diff --git a/backends/iommufd.c b/backends/iommufd.c index 393c0d9a37..1ef683c7b0 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -29,7 +29,6 @@ static void iommufd_backend_init(Object *obj) be->fd = -1; be->users = 0; be->owned = true; - qemu_mutex_init(&be->lock); } static void iommufd_backend_finalize(Object *obj) @@ -52,10 +51,8 @@ static void iommufd_backend_set_fd(Object *obj, const char *str, Error **errp) error_prepend(errp, "Could not parse remote object fd %s:", str); return; } - qemu_mutex_lock(&be->lock); be->fd = fd; be->owned = false; - qemu_mutex_unlock(&be->lock); trace_iommu_backend_set_fd(be->fd); } @@ -79,7 +76,6 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp) { int fd, ret = 0; - qemu_mutex_lock(&be->lock); if (be->owned && !be->users) { fd = qemu_open_old("/dev/iommu", O_RDWR); if (fd < 0) { @@ -93,13 +89,11 @@ int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp) out: trace_iommufd_backend_connect(be->fd, be->owned, be->users, ret); - qemu_mutex_unlock(&be->lock); return ret; } void iommufd_backend_disconnect(IOMMUFDBackend *be) { - qemu_mutex_lock(&be->lock); if (!be->users) { goto out; } @@ -110,7 +104,6 @@ void iommufd_backend_disconnect(IOMMUFDBackend *be) } out: trace_iommufd_backend_disconnect(be->fd, be->users); - qemu_mutex_unlock(&be->lock); } int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id, diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h index 9c5524b0ed..9af27ebd6c 100644 --- a/include/sysemu/iommufd.h +++ b/include/sysemu/iommufd.h @@ -2,7 +2,6 @@ #define SYSEMU_IOMMUFD_H #include "qom/object.h" -#include "qemu/thread.h" #include "exec/hwaddr.h" #include "exec/cpu-common.h" @@ -19,7 +18,6 @@ struct IOMMUFDBackend { /*< protected >*/ int fd; /* /dev/iommu file descriptor */ bool owned; /* is the /dev/iommu opened internally */ - QemuMutex lock; uint32_t users; /*< public >*/