Re: [PATCH v2 03/12] vfio: Collect container iova range info

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 13 Sep 2023 10:01:38 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Collect iova range information if VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE
> capability is supported.
> 
> This allows to propagate the information though the IOMMU MR
> set_iova_ranges() callback so that virtual IOMMUs
> get aware of those aperture constraints.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/hw/vfio/vfio-common.h |  2 ++
>  hw/vfio/common.c              | 45 +++++++++++++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index da43d27352..74b9b27270 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -98,6 +98,8 @@ typedef struct VFIOContainer {
>      QLIST_HEAD(, VFIOGroup) group_list;
>      QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
>      QLIST_ENTRY(VFIOContainer) next;
> +    unsigned nr_iovas;
> +    struct  vfio_iova_range *iova_ranges;
>  } VFIOContainer;
>  
>  typedef struct VFIOGuestIOMMU {
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9aac21abb7..26da38de05 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1157,6 +1157,14 @@ static void vfio_listener_region_add(MemoryListener *listener,
>              goto fail;
>          }
>  
> +        ret = memory_region_iommu_set_iova_ranges(giommu->iommu_mr,
> +                container->nr_iovas, (struct Range *)container->iova_ranges,
> +                &err);

The semantics of calling this with nr_iovas == 0 and the vIOMMU driver
ignoring that it's being told there are no usable iova ranges is rather
strange.  Should nr_iovas be initialized to -1 for that or should this
call be conditional on non-zero nr_iovas?

Also, vfio_get_info_iova_range() is only called in the type1 container
path and the IOVA range info capability has only existed since kernel
v5.4.  So we need to do something sane even if we don't have the kernel
telling us about the IOVA ranges.  I think this precludes the assert in
the final patch of the series or else new QEMU on an old kernel is
broken.

> +        if (ret) {
> +            g_free(giommu);
> +            goto fail;
> +        }
> +
>          ret = memory_region_register_iommu_notifier(section->mr, &giommu->n,
>                                                      &err);
>          if (ret) {
> @@ -1981,6 +1989,29 @@ bool vfio_get_info_dma_avail(struct vfio_iommu_type1_info *info,
>      return true;
>  }
>  
> +static void vfio_get_info_iova_range(struct vfio_iommu_type1_info *info,
> +                                     unsigned int *nr_iovas,
> +                                     struct  vfio_iova_range **iova_ranges)

Just pass the VFIOContainer pointer?  Thanks,

Alex

> +{
> +    struct vfio_info_cap_header *hdr;
> +    struct vfio_iommu_type1_info_cap_iova_range *cap;
> +
> +    hdr = vfio_get_iommu_type1_info_cap(info,
> +                                        VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE);
> +    if (hdr == NULL) {
> +        return;
> +    }
> +
> +    cap = (void *)hdr;
> +    *nr_iovas = cap->nr_iovas;
> +
> +    if (*nr_iovas == 0) {
> +        return;
> +    }
> +    *iova_ranges = g_memdup2(cap->iova_ranges,
> +                             *nr_iovas * sizeof(struct  vfio_iova_range));
> +}
> +
>  static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
>                                            struct vfio_region_info *info)
>  {
> @@ -2433,6 +2464,12 @@ static void vfio_get_iommu_info_migration(VFIOContainer *container,
>      }
>  }
>  
> +static void vfio_free_container(VFIOContainer *container)
> +{
> +    g_free(container->iova_ranges);
> +    g_free(container);
> +}
> +
>  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>                                    Error **errp)
>  {
> @@ -2550,6 +2587,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>          if (!vfio_get_info_dma_avail(info, &container->dma_max_mappings)) {
>              container->dma_max_mappings = 65535;
>          }
> +
> +        vfio_get_info_iova_range(info, &container->nr_iovas,
> +                                 &container->iova_ranges);
> +
>          vfio_get_iommu_info_migration(container, info);
>          g_free(info);
>  
> @@ -2663,7 +2704,7 @@ enable_discards_exit:
>      vfio_ram_block_discard_disable(container, false);
>  
>  free_container_exit:
> -    g_free(container);
> +    vfio_free_container(container);
>  
>  close_fd_exit:
>      close(fd);
> @@ -2717,7 +2758,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>  
>          trace_vfio_disconnect_container(container->fd);
>          close(container->fd);
> -        g_free(container);
> +        vfio_free_container(container);
>  
>          vfio_put_address_space(space);
>      }