Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 15/16] vfio: Move iova_pgsizes from container to guest IOMMU

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

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

On Tue, Mar 01, 2016 at 08:10:40PM +1100, Alexey Kardashevskiy wrote:
> The page size is an attribute of an IOMMU, not a container as a container
> may contain more just one IOMMU.
> 
> This moves iova_pgsizes from VFIOContainer to VFIOGuestIOMMU.
> The following patch will use this.
> 
> This removes iova_pgsizes from Type1 IOMMU as it is not used there anyway
> and when it will get guest visible IOMMU, it will use VFIOGuestIOMMU's
> iova_pgsizes.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Hmm.  This makes an important semantic change which.. I'm not sure is
wrong, but certainly isn't adequately addressed in your commit
message.

The current iova_pgsizes is populated with information about the
*host* IOMMU, whereas you're replacing it with information about the
*guest* IOMMU.

> ---
>  hw/vfio/common.c              | 16 ++++------------
>  include/hw/vfio/vfio-common.h |  2 +-
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index f2a03e0..42ef1eb 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -313,9 +313,9 @@ out:
>      rcu_read_unlock();
>  }
>  
> -static hwaddr vfio_container_granularity(VFIOContainer *container)
> +static hwaddr vfio_container_granularity(VFIOGuestIOMMU *giommu)
>  {
> -    return (hwaddr)1 << ctz64(container->iova_pgsizes);
> +    return (hwaddr)1 << ctz64(giommu->iova_pgsizes);
>  }
>  
>  static hwaddr vfio_iommu_page_mask(MemoryRegion *mr)
> @@ -392,12 +392,13 @@ static void vfio_listener_region_add(VFIOMemoryListener *vlistener,
>              section->offset_within_address_space;
>          giommu->container = container;
>          giommu->n.notify = vfio_iommu_map_notify;
> +        giommu->iova_pgsizes = section->mr->iommu_ops->get_page_sizes(section->mr);
>          QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
>  
>          memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
>          giommu->iommu->iommu_ops->vfio_notify(section->mr, true);
>          memory_region_iommu_replay(giommu->iommu, &giommu->n,
> -                                   vfio_container_granularity(container),
> +                                   vfio_container_granularity(giommu),
>                                     false);
>  
>          return;
> @@ -743,14 +744,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>          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;
> -        }
>      } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) ||
>                 ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) {
>          struct vfio_iommu_spapr_tce_info info;
> @@ -811,9 +806,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>          }
>          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;
>      } 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 bcbc5cb..48a1d7f 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -80,7 +80,6 @@ typedef struct VFIOContainer {
>       * future
>       */
>      hwaddr min_iova, max_iova;
> -    uint64_t iova_pgsizes;
>      QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
>      QLIST_HEAD(, VFIOGroup) group_list;
>      QLIST_ENTRY(VFIOContainer) next;
> @@ -90,6 +89,7 @@ typedef struct VFIOGuestIOMMU {
>      VFIOContainer *container;
>      MemoryRegion *iommu;
>      hwaddr offset_within_address_space;
> +    uint64_t iova_pgsizes;
>      Notifier n;
>      QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
>  } VFIOGuestIOMMU;

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