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

[Liu, Yi L]

Hi Eric,

> From: Auger Eric
> Sent: Tuesday, March 31, 2020 6:48 PM
> To: Liu, Yi L <address@hidden>; address@hidden;
> address@hidden; address@hidden
> Cc: address@hidden; address@hidden; address@hidden; Tian,
> Kevin <address@hidden>; Tian, Jun J <address@hidden>; Sun, Yi Y
> <address@hidden>; address@hidden; Wu, Hao <address@hidden>; jean-
> address@hidden; Jacob Pan <address@hidden>; Yi Sun
> <address@hidden>
> 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 <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                      | 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,
};

Regards,
Yi Liu