Re: [Qemu-devel] [PATCH] vfio: avoid adding same iommu mr for notify

[David Gibson]

On Mon, Nov 14, 2016 at 07:59:28PM -0500, Peter Xu wrote:
> When one IOMMU memory region is splitted into multiple memory sections,
> vfio will register multiple same notifiers to a vIOMMU for the same
> region. That's not sensible. What we need is to register one IOMMU
> notifier for each IOMMU region, not per section.
> 
> Solution is simple - we traverse the container->giommu_list, and skip
> the registration if memory region is already registered.
> 
> To make vfio's region_add() short, vfio_listener_region_add_iommu() is
> introduced.
> 
> Signed-off-by: Peter Xu <address@hidden>

This is wrong.  It will work on the register side, but when the first
section attached to the IOMMU is removed, the IOMMU will be removed
from the list (triggering an unmap of the whole vfio space), rather
than when the *last* section attached to the MR is removed.

You'll get away with it in the simple x86 case, because both sections
will be removed at basically the same time, but it's not correct in
general.

I really think a better approach is to add the section boundary
information to the IOMMUNotifier structure within VFIOGuestIOMMU, and
check that as well as the MR on remove.  That additionally means the
IOMMU notifier won't get called for portions of the MR outside the
mapped sections, which the notifier handler probably isn't going to
expect.

> ---
>  hw/vfio/common.c | 56 
> +++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 801578b..5279fd1 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -360,6 +360,40 @@ out:
>      rcu_read_unlock();
>  }
>  
> +static void vfio_listener_region_add_iommu(VFIOContainer *container,
> +                                           MemoryRegionSection *section,
> +                                           hwaddr iova,
> +                                           hwaddr end)
> +{
> +    VFIOGuestIOMMU *giommu;
> +
> +    QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> +        if (giommu->iommu == section->mr) {
> +            /* We have already registered with this MR, skip */
> +            return;
> +        }
> +    }
> +
> +    trace_vfio_listener_region_add_iommu(iova, end);
> +
> +    /*
> +     * FIXME: For VFIO iommu types which have KVM acceleration to
> +     * avoid bouncing all map/unmaps through qemu this way, this
> +     * would be the right place to wire that up (tell the KVM
> +     * device emulation the VFIO iommu handles to use).
> +     */
> +    giommu = g_malloc0(sizeof(*giommu));
> +    giommu->iommu = section->mr;
> +    giommu->iommu_offset = section->offset_within_address_space -
> +        section->offset_within_region;
> +    giommu->container = container;
> +    giommu->n.notify = vfio_iommu_map_notify;
> +    giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> +    QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> +    memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> +    memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
> +}
> +
>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> @@ -439,27 +473,7 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      memory_region_ref(section->mr);
>  
>      if (memory_region_is_iommu(section->mr)) {
> -        VFIOGuestIOMMU *giommu;
> -
> -        trace_vfio_listener_region_add_iommu(iova, end);
> -        /*
> -         * FIXME: For VFIO iommu types which have KVM acceleration to
> -         * avoid bouncing all map/unmaps through qemu this way, this
> -         * would be the right place to wire that up (tell the KVM
> -         * device emulation the VFIO iommu handles to use).
> -         */
> -        giommu = g_malloc0(sizeof(*giommu));
> -        giommu->iommu = section->mr;
> -        giommu->iommu_offset = section->offset_within_address_space -
> -                               section->offset_within_region;
> -        giommu->container = container;
> -        giommu->n.notify = vfio_iommu_map_notify;
> -        giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> -        QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> -
> -        memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> -        memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
> -
> +        vfio_listener_region_add_iommu(container, section, iova, end);
>          return;
>      }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature