Hi Eric,
> From: Auger Eric <eric.auger@xxxxxxxxxx>
> Sent: Monday, April 6, 2020 6:20 PM
> Subject: Re: [PATCH v2 09/22] vfio/common: init HostIOMMUContext per-container
>
> Hi Yi,
>
> On 4/6/20 9:12 AM, Liu, Yi L wrote:
> > Hi Eric,
> >
> >> From: Auger Eric <eric.auger@xxxxxxxxxx>
> >> Sent: Wednesday, April 1, 2020 3:51 PM
> >> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>; qemu-devel@xxxxxxxxxx;
> >> 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 <kevin.tian@xxxxxxxxx>
> >>> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >>> Cc: Peter Xu <peterx@xxxxxxxxxx>
> >>> Cc: Eric Auger <eric.auger@xxxxxxxxxx>
> >>> Cc: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> >>> Cc: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> >>> ---
> >>> 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?
Hmm, I can add it. But I've no idea about the proper owner for it so far.
any suggestion?
Regards,
Yi Liu
