Re: [Qemu-devel] [PATCH qemu v16 01/19] vfio: Delay DMA address space listener release

[Alex Williamson]

On Wed,  4 May 2016 16:52:13 +1000
Alexey Kardashevskiy <address@hidden> wrote:

> This postpones VFIO container deinitialization to let region_del()
> callbacks (called via vfio_listener_release) do proper clean up
> while the group is still attached to the container.

Any mappings within the container should clean themselves up when the
container is deprivleged by removing the last group in the kernel.  Is
the issue that that doesn't happen, which would be a spapr vfio kernel
bug, or that our QEMU side structures get all out of whack if we let
that happen?

> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  hw/vfio/common.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fe5ec6a..0b40262 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -921,23 +921,31 @@ static void vfio_disconnect_container(VFIOGroup *group)
>  {
>      VFIOContainer *container = group->container;
>  
> -    if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
> -        error_report("vfio: error disconnecting group %d from container",
> -                     group->groupid);
> -    }
> -
>      QLIST_REMOVE(group, container_next);
> +
> +    if (QLIST_EMPTY(&container->group_list)) {
> +        VFIOGuestIOMMU *giommu;
> +
> +        vfio_listener_release(container);
> +
> +        QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> +            memory_region_unregister_iommu_notifier(&giommu->n);
> +        }
> +    }
> +
>      group->container = NULL;
> +    if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
> +        error_report("vfio: error disconnecting group %d from container",
> +                     group->groupid);
> +    }
>  
>      if (QLIST_EMPTY(&container->group_list)) {
>          VFIOAddressSpace *space = container->space;
>          VFIOGuestIOMMU *giommu, *tmp;
>  
> -        vfio_listener_release(container);
>          QLIST_REMOVE(container, next);
>  
>          QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, 
> tmp) {
> -            memory_region_unregister_iommu_notifier(&giommu->n);
>              QLIST_REMOVE(giommu, giommu_next);
>              g_free(giommu);
>          }

I'm not spotting why this is a 2-pass process vs simply moving the
existing QLIST_EMPTY cleanup above the ioctl.  Thanks,

Alex