Re: [PATCH V8 30/39] vfio-pci: recover from unmap-all-vaddr failure

[Alex Williamson]

On Wed, 15 Jun 2022 07:52:17 -0700
Steve Sistare <steven.sistare@oracle.com> wrote:

> If vfio_cpr_save fails to unmap all vaddr's, then recover by walking all
> flat sections to restore the vaddr for each.  Do so by invoking the
> vfio listener callback, and passing a new "replay" flag that tells it
> to replay a mapping without re-allocating new userland data structures.

Is this comment accurate?  I thought we had unwind in the kernel for
vaddr invalidation, and the notifier here is hooked up to any fault, so
it's at least misleading regarding vaddr.  The replay option really
needs some documentation in comments.

> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/vfio/common.c              | 66 
> ++++++++++++++++++++++++++++++++-----------
>  hw/vfio/cpr.c                 | 29 +++++++++++++++++++
>  include/hw/vfio/vfio-common.h |  2 +-
>  3 files changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index c7d73b6..5f2bd50 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -895,15 +895,35 @@ static bool 
> vfio_known_safe_misalignment(MemoryRegionSection *section)
>      return true;
>  }
>  
> +static VFIORamDiscardListener *vfio_find_ram_discard_listener(
> +    VFIOContainer *container, MemoryRegionSection *section)
> +{
> +    VFIORamDiscardListener *vrdl = NULL;

This initialization was copied from current code, but...

#define QLIST_FOREACH(var, head, field)                                 \
        for ((var) = ((head)->lh_first);                                \
               ...

it doesn't look necessary.  Thanks,

Alex

> +
> +    QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
> +        if (vrdl->mr == section->mr &&
> +            vrdl->offset_within_address_space ==
> +            section->offset_within_address_space) {
> +            break;
> +        }
> +    }
> +
> +    if (!vrdl) {
> +        hw_error("vfio: Trying to sync missing RAM discard listener");
> +        /* does not return */
> +    }
> +    return vrdl;
> +}
> +
>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
>      VFIOContainer *container = container_of(listener, VFIOContainer, 
> listener);
> -    vfio_container_region_add(container, section);
> +    vfio_container_region_add(container, section, false);
>  }
>  
>  void vfio_container_region_add(VFIOContainer *container,
> -                               MemoryRegionSection *section)
> +                               MemoryRegionSection *section, bool replay)
>  {
>      hwaddr iova, end;
>      Int128 llend, llsize;
> @@ -1033,6 +1053,23 @@ void vfio_container_region_add(VFIOContainer 
> *container,
>          int iommu_idx;
>  
>          trace_vfio_listener_region_add_iommu(iova, end);
> +
> +        if (replay) {
> +            hwaddr as_offset = section->offset_within_address_space;
> +            hwaddr iommu_offset = as_offset - section->offset_within_region;
> +
> +            QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> +                if (giommu->iommu_mr == iommu_mr &&
> +                    giommu->iommu_offset == iommu_offset) {
> +                    memory_region_iommu_replay(giommu->iommu_mr, &giommu->n);
> +                    return;
> +                }
> +            }
> +            error_report("Container cannot find iommu region %s offset %lx",
> +                memory_region_name(section->mr), iommu_offset);
> +            goto fail;
> +        }
> +
>          /*
>           * FIXME: For VFIO iommu types which have KVM acceleration to
>           * avoid bouncing all map/unmaps through qemu this way, this
> @@ -1083,7 +1120,15 @@ void vfio_container_region_add(VFIOContainer 
> *container,
>       * about changes.
>       */
>      if (memory_region_has_ram_discard_manager(section->mr)) {
> -        vfio_register_ram_discard_listener(container, section);
> +        if (replay)  {
> +            VFIORamDiscardListener *vrdl =
> +                vfio_find_ram_discard_listener(container, section);
> +            if (vfio_ram_discard_notify_populate(&vrdl->listener, section)) {
> +                error_report("ram_discard_manager_replay_populated failed");
> +            }
> +        } else {
> +            vfio_register_ram_discard_listener(container, section);
> +        }
>          return;
>      }
>  
> @@ -1417,19 +1462,8 @@ static int 
> vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainer *container,
>                                                     MemoryRegionSection 
> *section)
>  {
>      RamDiscardManager *rdm = 
> memory_region_get_ram_discard_manager(section->mr);
> -    VFIORamDiscardListener *vrdl = NULL;
> -
> -    QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
> -        if (vrdl->mr == section->mr &&
> -            vrdl->offset_within_address_space ==
> -            section->offset_within_address_space) {
> -            break;
> -        }
> -    }
> -
> -    if (!vrdl) {
> -        hw_error("vfio: Trying to sync missing RAM discard listener");
> -    }
> +    VFIORamDiscardListener *vrdl =
> +        vfio_find_ram_discard_listener(container, section);
>  
>      /*
>       * We only want/can synchronize the bitmap for actually mapped parts -
> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
> index a227d5e..2b5e77c 100644
> --- a/hw/vfio/cpr.c
> +++ b/hw/vfio/cpr.c
> @@ -32,6 +32,15 @@ vfio_dma_unmap_vaddr_all(VFIOContainer *container, Error 
> **errp)
>      return 0;
>  }
>  
> +static int
> +vfio_region_remap(MemoryRegionSection *section, void *handle, Error **errp)
> +{
> +    VFIOContainer *container = handle;
> +    vfio_container_region_add(container, section, true);
> +    container->vaddr_unmapped = false;
> +    return 0;
> +}
> +
>  static bool vfio_is_cpr_capable(VFIOContainer *container, Error **errp)
>  {
>      if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR) ||
> @@ -98,6 +107,22 @@ static const VMStateDescription vfio_container_vmstate = {
>      }
>  };
>  
> +static void vfio_cpr_save_failed_notifier(Notifier *notifier, void *data)
> +{
> +    Error *err;
> +    VFIOContainer *container =
> +        container_of(notifier, VFIOContainer, cpr_notifier);
> +
> +    /* Set reused so vfio_dma_map restores vaddr */
> +    container->reused = true;
> +    if (address_space_flat_for_each_section(container->space->as,
> +                                            vfio_region_remap,
> +                                            container, &err)) {
> +        error_report_err(err);
> +    }
> +    container->reused = false;
> +}
> +
>  int vfio_cpr_register_container(VFIOContainer *container, Error **errp)
>  {
>      container->cpr_blocker = NULL;
> @@ -108,6 +133,8 @@ int vfio_cpr_register_container(VFIOContainer *container, 
> Error **errp)
>  
>      vmstate_register(NULL, -1, &vfio_container_vmstate, container);
>  
> +    cpr_add_notifier(&container->cpr_notifier, vfio_cpr_save_failed_notifier,
> +                     CPR_NOTIFY_SAVE_FAILED);
>      return 0;
>  }
>  
> @@ -116,4 +143,6 @@ void vfio_cpr_unregister_container(VFIOContainer 
> *container)
>      cpr_del_blocker(&container->cpr_blocker);
>  
>      vmstate_unregister(NULL, &vfio_container_vmstate, container);
> +
> +    cpr_remove_notifier(&container->cpr_notifier);
>  }
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 17ad9ba..dd6bbcf 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -244,7 +244,7 @@ vfio_get_device_info_cap(struct vfio_device_info *info, 
> uint16_t id);
>  extern const MemoryListener vfio_prereg_listener;
>  void vfio_listener_register(VFIOContainer *container);
>  void vfio_container_region_add(VFIOContainer *container,
> -                               MemoryRegionSection *section);
> +                               MemoryRegionSection *section, bool replay);
>  
>  int vfio_spapr_create_window(VFIOContainer *container,
>                               MemoryRegionSection *section,