Re: [Qemu-devel] [PATCH qemu v14 15/18] vfio: Add host side IOMMU capabilities

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7009 bytes --]

On Mon, Mar 21, 2016 at 06:47:03PM +1100, Alexey Kardashevskiy wrote:
> There are going to be multiple IOMMUs per a container. This moves
> the single host IOMMU parameter set to a list of VFIOHostIOMMU.
> 
> This should cause no behavioral change and will be used later by
> the SPAPR TCE IOMMU v2 which will also add a vfio_host_iommu_del() helper.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

This looks ok except for the name.  Calling each window a separate
"host IOMMU" is misleading.  The different windows the container
supports might be implemented by different IOMMUs on the host side, or
it might be implemented by one IOMMU with multiple tables.

Better to call them host DMA windows, or maybe container DMA windows.

> ---
>  hw/vfio/common.c              | 65 +++++++++++++++++++++++++++++++++----------
>  include/hw/vfio/vfio-common.h |  9 ++++--
>  2 files changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index a8deb16..b257655 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -29,6 +29,7 @@
>  #include "exec/memory.h"
>  #include "hw/hw.h"
>  #include "qemu/error-report.h"
> +#include "qemu/range.h"
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  
> @@ -239,6 +240,45 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
>      return -errno;
>  }
>  
> +static VFIOHostIOMMU *vfio_host_iommu_lookup(VFIOContainer *container,
> +                                             hwaddr min_iova, hwaddr max_iova)
> +{
> +    VFIOHostIOMMU *hiommu;
> +
> +    QLIST_FOREACH(hiommu, &container->hiommu_list, hiommu_next) {
> +        if (hiommu->min_iova <= min_iova && max_iova <= hiommu->max_iova) {
> +            return hiommu;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static int vfio_host_iommu_add(VFIOContainer *container,
> +                               hwaddr min_iova, hwaddr max_iova,
> +                               uint64_t iova_pgsizes)
> +{
> +    VFIOHostIOMMU *hiommu;
> +
> +    QLIST_FOREACH(hiommu, &container->hiommu_list, hiommu_next) {
> +        if (ranges_overlap(min_iova, max_iova - min_iova + 1,
> +                           hiommu->min_iova,
> +                           hiommu->max_iova - hiommu->min_iova + 1)) {
> +            error_report("%s: Overlapped IOMMU are not enabled", __func__);
> +            return -1;
> +        }
> +    }
> +
> +    hiommu = g_malloc0(sizeof(*hiommu));
> +
> +    hiommu->min_iova = min_iova;
> +    hiommu->max_iova = max_iova;
> +    hiommu->iova_pgsizes = iova_pgsizes;
> +    QLIST_INSERT_HEAD(&container->hiommu_list, hiommu, hiommu_next);
> +
> +    return 0;
> +}
> +
>  static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>  {
>      return (!memory_region_is_ram(section->mr) &&
> @@ -352,7 +392,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      }
>      end = int128_get64(llend);
>  
> -    if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
> +    if (!vfio_host_iommu_lookup(container, iova, end - 1)) {
>          error_report("vfio: IOMMU container %p can't map guest IOVA region"
>                       " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
>                       container, iova, end - 1);
> @@ -367,10 +407,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>  
>          trace_vfio_listener_region_add_iommu(iova, end - 1);
>          /*
> -         * FIXME: We should do some checking to see if the
> -         * capabilities of the host VFIO IOMMU are adequate to model
> -         * the guest IOMMU
> -         *
>           * 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
> @@ -818,16 +854,14 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>           * existing Type1 IOMMUs generally support any IOVA we're
>           * going to actually try in practice.
>           */
> -        container->min_iova = 0;
> -        container->max_iova = (hwaddr)-1;
> -
> -        /* Assume just 4K IOVA page size */
> -        container->iova_pgsizes = 0x1000;
>          info.argsz = sizeof(info);
>          ret = ioctl(fd, VFIO_IOMMU_GET_INFO, &info);
>          /* Ignore errors */
>          if ((ret == 0) && (info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
> -            container->iova_pgsizes = info.iova_pgsizes;
> +            vfio_host_iommu_add(container, 0, (hwaddr)-1, info.iova_pgsizes);
> +        } else {
> +            /* Assume just 4K IOVA page size */
> +            vfio_host_iommu_add(container, 0, (hwaddr)-1, 0x1000);
>          }
>      } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) ||
>                 ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) {
> @@ -884,11 +918,12 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>              ret = -errno;
>              goto listener_release_exit;
>          }
> -        container->min_iova = info.dma32_window_start;
> -        container->max_iova = container->min_iova + info.dma32_window_size - 1;
>  
> -        /* Assume just 4K IOVA pages for now */
> -        container->iova_pgsizes = 0x1000;
> +        /* The default table uses 4K pages */
> +        vfio_host_iommu_add(container, info.dma32_window_start,
> +                            info.dma32_window_start +
> +                            info.dma32_window_size - 1,
> +                            0x1000);
>      } else {
>          error_report("vfio: No available IOMMU models");
>          ret = -EINVAL;
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b861eec..1b98e33 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -82,9 +82,8 @@ typedef struct VFIOContainer {
>       * contiguous IOVA window.  We may need to generalize that in
>       * future
>       */
> -    hwaddr min_iova, max_iova;
> -    uint64_t iova_pgsizes;
>      QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> +    QLIST_HEAD(, VFIOHostIOMMU) hiommu_list;
>      QLIST_HEAD(, VFIOGroup) group_list;
>      QLIST_ENTRY(VFIOContainer) next;
>  } VFIOContainer;
> @@ -97,6 +96,12 @@ typedef struct VFIOGuestIOMMU {
>      QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
>  } VFIOGuestIOMMU;
>  
> +typedef struct VFIOHostIOMMU {
> +    hwaddr min_iova, max_iova;
> +    uint64_t iova_pgsizes;
> +    QLIST_ENTRY(VFIOHostIOMMU) hiommu_next;
> +} VFIOHostIOMMU;
> +
>  typedef struct VFIODeviceOps VFIODeviceOps;
>  
>  typedef struct VFIODevice {

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]