Re: [PATCH v2 09/22] vfio/common: init HostIOMMUContext per-container

[Auger Eric]

Hi Yi,

On 3/30/20 6:24 AM, Liu Yi L wrote:
> In this patch, QEMU firstly gets iommu info from kernel to check the
> supported capabilities by a VFIO_IOMMU_TYPE1_NESTING iommu. And inits
> HostIOMMUContet instance.
> 
> Cc: Kevin Tian <address@hidden>
> Cc: Jacob Pan <address@hidden>
> Cc: Peter Xu <address@hidden>
> Cc: Eric Auger <address@hidden>
> Cc: Yi Sun <address@hidden>
> Cc: David Gibson <address@hidden>
> Cc: Alex Williamson <address@hidden>
> Signed-off-by: Liu Yi L <address@hidden>
> ---
>  hw/vfio/common.c | 99 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 5f3534d..44b142c 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1226,10 +1226,89 @@ static int 
> vfio_host_iommu_ctx_pasid_free(HostIOMMUContext *iommu_ctx,
>      return 0;
>  }
>  
> +/**
> + * Get iommu info from host. Caller of this funcion should free
> + * the memory pointed by the returned pointer stored in @info
> + * after a successful calling when finished its usage.
> + */
> +static int vfio_get_iommu_info(VFIOContainer *container,
> +                         struct vfio_iommu_type1_info **info)
> +{
> +
> +    size_t argsz = sizeof(struct vfio_iommu_type1_info);
> +
> +    *info = g_malloc0(argsz);
> +
> +retry:
> +    (*info)->argsz = argsz;
> +
> +    if (ioctl(container->fd, VFIO_IOMMU_GET_INFO, *info)) {
> +        g_free(*info);
> +        *info = NULL;
> +        return -errno;
> +    }
> +
> +    if (((*info)->argsz > argsz)) {
> +        argsz = (*info)->argsz;
> +        *info = g_realloc(*info, argsz);
> +        goto retry;
> +    }
> +
> +    return 0;
> +}
> +
> +static struct vfio_info_cap_header *
> +vfio_get_iommu_info_cap(struct vfio_iommu_type1_info *info, uint16_t id)
> +{
> +    struct vfio_info_cap_header *hdr;
> +    void *ptr = info;
> +
> +    if (!(info->flags & VFIO_IOMMU_INFO_CAPS)) {
> +        return NULL;
> +    }
> +
> +    for (hdr = ptr + info->cap_offset; hdr != ptr; hdr = ptr + hdr->next) {
> +        if (hdr->id == id) {
> +            return hdr;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static int vfio_get_nesting_iommu_cap(VFIOContainer *container,
> +                   struct vfio_iommu_type1_info_cap_nesting *cap_nesting)
> +{
> +    struct vfio_iommu_type1_info *info;
> +    struct vfio_info_cap_header *hdr;
> +    struct vfio_iommu_type1_info_cap_nesting *cap;
> +    int ret;
> +
> +    ret = vfio_get_iommu_info(container, &info);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    hdr = vfio_get_iommu_info_cap(info,
> +                        VFIO_IOMMU_TYPE1_INFO_CAP_NESTING);
> +    if (!hdr) {
> +        g_free(info);
> +        return -errno;
> +    }
> +
> +    cap = container_of(hdr,
> +                struct vfio_iommu_type1_info_cap_nesting, header);
> +    *cap_nesting = *cap;
> +
> +    g_free(info);
> +    return 0;
> +}
> +
>  static int vfio_init_container(VFIOContainer *container, int group_fd,
>                                 Error **errp)
>  {
>      int iommu_type, ret;
> +    uint64_t flags = 0;
>  
>      iommu_type = vfio_get_iommu_type(container, errp);
>      if (iommu_type < 0) {
> @@ -1257,6 +1336,26 @@ static int vfio_init_container(VFIOContainer 
> *container, int group_fd,
>          return -errno;
>      }
>  
> +    if (iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
> +        struct vfio_iommu_type1_info_cap_nesting nesting = {
> +                                         .nesting_capabilities = 0x0,
> +                                         .stage1_formats = 0, };
> +
> +        ret = vfio_get_nesting_iommu_cap(container, &nesting);
> +        if (ret) {
> +            error_setg_errno(errp, -ret,
> +                             "Failed to get nesting iommu cap");
> +            return ret;
> +        }
> +
> +        flags |= (nesting.nesting_capabilities & VFIO_IOMMU_PASID_REQS) ?
> +                 HOST_IOMMU_PASID_REQUEST : 0;
I still don't get why you can't transform your iommu_ctx into a  pointer
and do
container->iommu_ctx = g_new0(HostIOMMUContext, 1);
then
host_iommu_ctx_init(container->iommu_ctx, flags);

looks something similar to (hw/vfio/common.c). You may not even need to
use a derived VFIOHostIOMMUContext object (As only VFIO does use that
object)? Only the ops do change, no new field?
        region->mem = g_new0(MemoryRegion, 1);
        memory_region_init_io(region->mem, obj, &vfio_region_ops,
                              region, name, region->size);

Thanks

Eric

> +        host_iommu_ctx_init(&container->iommu_ctx,
> +                            sizeof(container->iommu_ctx),
> +                            TYPE_VFIO_HOST_IOMMU_CONTEXT,
> +                            flags);
> +    }
> +
>      container->iommu_type = iommu_type;
>      return 0;
>  }
>