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

[Auger Eric]

Hi Yi,

On 4/6/20 9:12 AM, Liu, Yi L wrote:
> Hi Eric,
> 
>> From: Auger Eric <address@hidden>
>> Sent: Wednesday, April 1, 2020 3:51 PM
>> To: Liu, Yi L <address@hidden>; address@hidden;
>> Subject: Re: [PATCH v2 09/22] vfio/common: init HostIOMMUContext 
>> per-container
>>
>> 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);
> 
> In this way, the vfio hook can easily get the VFIOContainer from
> HostIOMMUContext when call in the hook provided by vfio. e.g. the
> one below.
OK I get it. However in memory_region_init_io(), you also pass the
owner, eg. region so I think you could do the same. no?

Thanks

Eric
> 
> +static int vfio_host_iommu_ctx_pasid_alloc(HostIOMMUContext *iommu_ctx,
> +                                           uint32_t min, uint32_t max,
> +                                           uint32_t *pasid)
> +{
> +    VFIOContainer *container = container_of(iommu_ctx,
> +                                            VFIOContainer, iommu_ctx);
>  
> Regards,
> Yi Liu
>