RE: [PATCH v2 08/22] vfio/common: provide PASID alloc/free hooks

["Liu, Yi L" <yi.l.liu@intel.com>]

Hi Eric,

> From: Auger Eric <eric.auger@redhat.com>
> Sent: Tuesday, March 31, 2020 7:16 PM
> To: Liu, Yi L <yi.l.liu@intel.com>; qemu-devel@nongnu.org;
> Subject: Re: [PATCH v2 08/22] vfio/common: provide PASID alloc/free hooks
> 
> Hi Yi,
> On 3/31/20 12:59 PM, Liu, Yi L wrote:
> > Hi Eric,
> >
> >> From: Auger Eric
> >> Sent: Tuesday, March 31, 2020 6:48 PM
> >> To: Liu, Yi L <yi.l.liu@intel.com>; qemu-devel@nongnu.org;
> >> alex.williamson@redhat.com; peterx@redhat.com
> >> Cc: pbonzini@redhat.com; mst@redhat.com; david@gibson.dropbear.id.au; Tian,
> >> Kevin <kevin.tian@intel.com>; Tian, Jun J <jun.j.tian@intel.com>; Sun, Yi Y
> >> <yi.y.sun@intel.com>; kvm@vger.kernel.org; Wu, Hao <hao.wu@intel.com>;
> jean-
> >> philippe@linaro.org; Jacob Pan <jacob.jun.pan@linux.intel.com>; Yi Sun
> >> <yi.y.sun@linux.intel.com>
> >> Subject: Re: [PATCH v2 08/22] vfio/common: provide PASID alloc/free hooks
> >>
> >> Yi,
> >>
> >> On 3/30/20 6:24 AM, Liu Yi L wrote:
> >>> This patch defines vfio_host_iommu_context_info, implements the PASID
> >>> alloc/free hooks defined in HostIOMMUContextClass.
> >>>
> >>> Cc: Kevin Tian <kevin.tian@intel.com>
> >>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> Cc: Peter Xu <peterx@redhat.com>
> >>> Cc: Eric Auger <eric.auger@redhat.com>
> >>> Cc: Yi Sun <yi.y.sun@linux.intel.com>
> >>> Cc: David Gibson <david@gibson.dropbear.id.au>
> >>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> >>> ---
> >>>  hw/vfio/common.c                      | 69
> +++++++++++++++++++++++++++++++++++
> >>>  include/hw/iommu/host_iommu_context.h |  3 ++
> >>>  include/hw/vfio/vfio-common.h         |  4 ++
> >>>  3 files changed, 76 insertions(+)
> >>>
> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
> >>> c276732..5f3534d 100644
> >>> --- a/hw/vfio/common.c
> >>> +++ b/hw/vfio/common.c
> >>> @@ -1179,6 +1179,53 @@ static int vfio_get_iommu_type(VFIOContainer
> >> *container,
> >>>      return -EINVAL;
> >>>  }
> >>>
> >>> +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);
> >>> +    struct vfio_iommu_type1_pasid_request req;
> >>> +    unsigned long argsz;
> >> you can easily avoid using argsz variable
> >
> > oh, right. :-)
> >
> >>> +    int ret;
> >>> +
> >>> +    argsz = sizeof(req);
> >>> +    req.argsz = argsz;
> >>> +    req.flags = VFIO_IOMMU_PASID_ALLOC;
> >>> +    req.alloc_pasid.min = min;
> >>> +    req.alloc_pasid.max = max;
> >>> +
> >>> +    if (ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req)) {
> >>> +        ret = -errno;
> >>> +        error_report("%s: %d, alloc failed", __func__, ret);
> >> better use %m directly or strerror(errno) also include vbasedev->name?
> >
> > or yes, vbasedev->name is also nice to have.
> >
> >>> +        return ret;
> >>> +    }
> >>> +    *pasid = req.alloc_pasid.result;
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int vfio_host_iommu_ctx_pasid_free(HostIOMMUContext *iommu_ctx,
> >>> +                                          uint32_t pasid) {
> >>> +    VFIOContainer *container = container_of(iommu_ctx,
> >>> +                                            VFIOContainer, iommu_ctx);
> >>> +    struct vfio_iommu_type1_pasid_request req;
> >>> +    unsigned long argsz;
> >> same
> >
> > got it.
> >
> >>> +    int ret;
> >>> +
> >>> +    argsz = sizeof(req);
> >>> +    req.argsz = argsz;
> >>> +    req.flags = VFIO_IOMMU_PASID_FREE;
> >>> +    req.free_pasid = pasid;
> >>> +
> >>> +    if (ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req)) {
> >>> +        ret = -errno;
> >>> +        error_report("%s: %d, free failed", __func__, ret);
> >> same
> >
> > yep.
> >>> +        return ret;
> >>> +    }
> >>> +    return 0;
> >>> +}
> >>> +
> >>>  static int vfio_init_container(VFIOContainer *container, int group_fd,
> >>>                                 Error **errp)  { @@ -1791,3 +1838,25
> >>> @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
> >>>      }
> >>>      return vfio_eeh_container_op(container, op);  }
> >>> +
> >>> +static void vfio_host_iommu_context_class_init(ObjectClass *klass,
> >>> +                                                       void *data) {
> >>> +    HostIOMMUContextClass *hicxc = HOST_IOMMU_CONTEXT_CLASS(klass);
> >>> +
> >>> +    hicxc->pasid_alloc = vfio_host_iommu_ctx_pasid_alloc;
> >>> +    hicxc->pasid_free = vfio_host_iommu_ctx_pasid_free; }
> >>> +
> >>> +static const TypeInfo vfio_host_iommu_context_info = {
> >>> +    .parent = TYPE_HOST_IOMMU_CONTEXT,
> >>> +    .name = TYPE_VFIO_HOST_IOMMU_CONTEXT,
> >>> +    .class_init = vfio_host_iommu_context_class_init,
> >> Ah OK
> >>
> >> This is the object inheriting from the abstract TYPE_HOST_IOMMU_CONTEXT.
> >
> > yes. it is. :-)
> >
> >> I initially thought VTDHostIOMMUContext was, sorry for the misunderstanding.
> >
> > Ah, my fault, should have got it earlier. so we may have just aligned
> > in last Oct.
> >
> >> Do you expect other HostIOMMUContext backends? Given the name and ops, it
> >> looks really related to VFIO?
> >
> > For other backends, I guess you mean other passthru modules? If yes, I
> > think they should have their own type name. Just like vIOMMUs, the below
> > vIOMMUs defines their own type name and inherits the same parent.
> >
> > static const TypeInfo vtd_iommu_memory_region_info = {
> >     .parent = TYPE_IOMMU_MEMORY_REGION,
> >     .name = TYPE_INTEL_IOMMU_MEMORY_REGION,
> >     .class_init = vtd_iommu_memory_region_class_init,
> > };
> >
> > static const TypeInfo smmuv3_iommu_memory_region_info = {
> >     .parent = TYPE_IOMMU_MEMORY_REGION,
> >     .name = TYPE_SMMUV3_IOMMU_MEMORY_REGION,
> >     .class_init = smmuv3_iommu_memory_region_class_init,
> > };
> >
> > static const TypeInfo amdvi_iommu_memory_region_info = {
> >     .parent = TYPE_IOMMU_MEMORY_REGION,
> >     .name = TYPE_AMD_IOMMU_MEMORY_REGION,
> >     .class_init = amdvi_iommu_memory_region_class_init,
> > };
> Sorry I am confused now.

The three above definition are just as an example. Just want to explain
what model I'm referencing. :-)

> You don't have such kind of inheritance at the moment in your series.

yes, only vfio inherits HostIOMMUContext, no other module inherits.
But I want to show a case in which there are multiple module inherits
one single parent. Just lack a vfio equivalent module to show it. So
I used the iommu_memory_region example. sorry to confuse you.

> 
> You have an abstract object (TYPE_HOST_IOMMU_CONTEXT, HostIOMMUContext)
> which is derived into TYPE_VFIO_HOST_IOMMU_CONTEXT. Only the class ops
> are specialized for VFIO. But I do not foresee any other user than VFIO
> (ie. other implementers of the class ops), hence my question. For
> instance would virtio/vhost ever implement its TYPE_HOST_IOMMU_CONTEXT.

I don't know either. But I think it's possible. They can do it per their
need in future.

> On the other hand you have VTDHostIOMMUContext which is not a QOM
> derived object.

Ok, I guess I made you believe both vfio and vIOMMU will inherit the
HostIOMMUContext now. is it?

Actually, it's not. Only vfio inherits HostIOMMUContext in QOM manner.
For the VTDHostIOMMUContext, it's just referencing the HostIOMMUContext
which is initialized by vfio.

Regards,
Yi Liu