Re: [PATCH V3 12/42] vfio/container: recover from unmap-all-vaddr failure

["Cédric Le Goater" <clg@redhat.com>]

On 5/12/25 17:32, Steve Sistare wrote:
> If there are multiple containers and unmap-all fails for some container, we
> need to remap vaddr for the other containers for which unmap-all succeeded.
> Recover by walking all address ranges of all containers to restore the vaddr
> for each.  Do so by invoking the vfio listener callback, and passing a new
> "remap" flag that tells it to restore a mapping without re-allocating new
> userland data structures.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/vfio/cpr-legacy.c                  | 91 +++++++++++++++++++++++++++++++++++
>   hw/vfio/listener.c                    | 19 +++++++-
>   include/hw/vfio/vfio-container-base.h |  3 ++
>   include/hw/vfio/vfio-cpr.h            | 10 ++++
>   4 files changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
> index bbcf71e..f8ddf78 100644
> --- a/hw/vfio/cpr-legacy.c
> +++ b/hw/vfio/cpr-legacy.c
> @@ -31,6 +31,7 @@ static bool vfio_dma_unmap_vaddr_all(VFIOContainer *container, Error **errp)
>           error_setg_errno(errp, errno, "vfio_dma_unmap_vaddr_all");
>           return false;
>       }
> +    container->cpr.vaddr_unmapped = true;
>       return true;
>   }
>   
> @@ -63,6 +64,14 @@ static int vfio_legacy_cpr_dma_map(const VFIOContainerBase *bcontainer,
>       return 0;
>   }
>   
> +static void vfio_region_remap(MemoryListener *listener,
> +                              MemoryRegionSection *section)
> +{
> +    VFIOContainer *container = container_of(listener, VFIOContainer,
> +                                            cpr.remap_listener);
> +    vfio_container_region_add(&container->bcontainer, section, true);
> +}
> +
>   static bool vfio_cpr_supported(VFIOContainer *container, Error **errp)
>   {
>       if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR)) {
> @@ -131,6 +140,40 @@ static const VMStateDescription vfio_container_vmstate = {
>       }
>   };
>   
> +static int vfio_cpr_fail_notifier(NotifierWithReturn *notifier,
> +                                  MigrationEvent *e, Error **errp)
> +{
> +    VFIOContainer *container =
> +        container_of(notifier, VFIOContainer, cpr.transfer_notifier);
> +    VFIOContainerBase *bcontainer = &container->bcontainer;
> +
> +    if (e->type != MIG_EVENT_PRECOPY_FAILED) {
> +        return 0;
> +    }
> +
> +    if (container->cpr.vaddr_unmapped) {
> +        /*
> +         * Force a call to vfio_region_remap for each mapped section by
> +         * temporarily registering a listener, and temporarily diverting
> +         * dma_map to vfio_legacy_cpr_dma_map.  The latter restores vaddr.
> +         */
> +
> +        VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +        vioc->dma_map = vfio_legacy_cpr_dma_map;
> +
> +        container->cpr.remap_listener = (MemoryListener) {
> +            .name = "vfio cpr recover",
> +            .region_add = vfio_region_remap
> +        };
> +        memory_listener_register(&container->cpr.remap_listener,
> +                                 bcontainer->space->as);
> +        memory_listener_unregister(&container->cpr.remap_listener);
> +        container->cpr.vaddr_unmapped = false;
> +        vioc->dma_map = vfio_legacy_dma_map;
> +    }
> +    return 0;
> +}
> +
>   bool vfio_legacy_cpr_register_container(VFIOContainer *container, Error **errp)
>   {
>       VFIOContainerBase *bcontainer = &container->bcontainer;
> @@ -152,6 +195,10 @@ bool vfio_legacy_cpr_register_container(VFIOContainer *container, Error **errp)
>           VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>           vioc->dma_map = vfio_legacy_cpr_dma_map;
>       }
> +
> +    migration_add_notifier_mode(&container->cpr.transfer_notifier,
> +                                vfio_cpr_fail_notifier,
> +                                MIG_MODE_CPR_TRANSFER);
>       return true;
>   }
>   
> @@ -162,6 +209,50 @@ void vfio_legacy_cpr_unregister_container(VFIOContainer *container)
>       migration_remove_notifier(&bcontainer->cpr_reboot_notifier);
>       migrate_del_blocker(&container->cpr.blocker);
>       vmstate_unregister(NULL, &vfio_container_vmstate, container);
> +    migration_remove_notifier(&container->cpr.transfer_notifier);
> +}
> +
> +/*
> + * In old QEMU, VFIO_DMA_UNMAP_FLAG_VADDR may fail on some mapping after
> + * succeeding for others, so the latter have lost their vaddr.  Call this
> + * to restore vaddr for a section with a giommu.
> + *
> + * The giommu already exists.  Find it and replay it, which calls
> + * vfio_legacy_cpr_dma_map further down the stack.
> + */
> +void vfio_cpr_giommu_remap(VFIOContainerBase *bcontainer,
> +                           MemoryRegionSection *section)
> +{
> +    VFIOGuestIOMMU *giommu = NULL;
> +    hwaddr as_offset = section->offset_within_address_space;
> +    hwaddr iommu_offset = as_offset - section->offset_within_region;
> +
> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> +        if (giommu->iommu_mr == IOMMU_MEMORY_REGION(section->mr) &&
> +            giommu->iommu_offset == iommu_offset) {
> +            break;
> +        }
> +    }
> +    g_assert(giommu);
> +    memory_region_iommu_replay(giommu->iommu_mr, &giommu->n);
> +}
> +
> +/*
> + * In old QEMU, VFIO_DMA_UNMAP_FLAG_VADDR may fail on some mapping after
> + * succeeding for others, so the latter have lost their vaddr.  Call this
> + * to restore vaddr for a section with a RamDiscardManager.
> + *
> + * The ram discard listener already exists.  Call its populate function
> + * directly, which calls vfio_legacy_cpr_dma_map.
> + */
> +bool vfio_cpr_ram_discard_register_listener(VFIOContainerBase *bcontainer,
> +                                            MemoryRegionSection *section)
> +{
> +    VFIORamDiscardListener *vrdl =
> +        vfio_find_ram_discard_listener(bcontainer, section);
> +
> +    g_assert(vrdl);
> +    return vrdl->listener.notify_populate(&vrdl->listener, section) == 0;
>   }
>   
>   static bool same_device(int fd1, int fd2)
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index 5642d04..e86ffcf 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
> @@ -474,6 +474,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
>   {
>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                    listener);
> +    vfio_container_region_add(bcontainer, section, false);
> +}
> +
> +void vfio_container_region_add(VFIOContainerBase *bcontainer,
> +                               MemoryRegionSection *section,
> +                               bool cpr_remap)
> +{
>       hwaddr iova, end;
>       Int128 llend, llsize;
>       void *vaddr;
> @@ -509,6 +516,11 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           int iommu_idx;
>   
>           trace_vfio_listener_region_add_iommu(section->mr->name, iova, end);
> +
> +        if (cpr_remap) {
> +            vfio_cpr_giommu_remap(bcontainer, section);
> +        }
> +
>           /*
>            * FIXME: For VFIO iommu types which have KVM acceleration to
>            * avoid bouncing all map/unmaps through qemu this way, this
> @@ -551,7 +563,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
>        * about changes.
>        */
>       if (memory_region_has_ram_discard_manager(section->mr)) {
> -        vfio_ram_discard_register_listener(bcontainer, section);
> +        if (!cpr_remap) {
> +            vfio_ram_discard_register_listener(bcontainer, section);
> +        } else if (!vfio_cpr_ram_discard_register_listener(bcontainer,
> +                                                           section)) {
> +            goto fail;
> +        }
>           return;
>       }
>   
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index a2f6c3a..5776fd7 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -189,4 +189,7 @@ VFIORamDiscardListener *vfio_find_ram_discard_listener(
>   int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>                           ram_addr_t size, void *vaddr, bool readonly);
>   
> +void vfio_container_region_add(VFIOContainerBase *bcontainer,
> +                               MemoryRegionSection *section, bool cpr_remap);
> +
>   #endif /* HW_VFIO_VFIO_CONTAINER_BASE_H */
> diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
> index 0fc7ab2..d6d22f2 100644
> --- a/include/hw/vfio/vfio-cpr.h
> +++ b/include/hw/vfio/vfio-cpr.h
> @@ -10,10 +10,14 @@
>   #define HW_VFIO_VFIO_CPR_H
>   
>   #include "migration/misc.h"
> +#include "system/memory.h"
>   
>   typedef struct VFIOContainerCPR {
>       Error *blocker;
>       bool reused;
> +    bool vaddr_unmapped;
> +    NotifierWithReturn transfer_notifier;
> +    MemoryListener remap_listener;
>   } VFIOContainerCPR;
>   
>   typedef struct VFIODeviceCPR {
> @@ -39,4 +43,10 @@ void vfio_cpr_unregister_container(struct VFIOContainerBase *bcontainer);
>   bool vfio_cpr_container_match(struct VFIOContainer *container,
>                                 struct VFIOGroup *group, int *fd);
>   
> +void vfio_cpr_giommu_remap(struct VFIOContainerBase *bcontainer,
> +                           MemoryRegionSection *section);
> +
> +bool vfio_cpr_ram_discard_register_listener(
> +    struct VFIOContainerBase *bcontainer, MemoryRegionSection *section);
> +
>   #endif /* HW_VFIO_VFIO_CPR_H */

Please add to your .gitconfig :

[diff]
     orderFile = /path/to/qemu/scripts/git.orderfile




Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.