diff mbox series

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

Message ID 1585542301-84087-9-git-send-email-yi.l.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: expose Shared Virtual Addressing to VMs | expand

Commit Message

Yi Liu March 30, 2020, 4:24 a.m. UTC 
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(+)

Comments

Eric Auger March 31, 2020, 10:47 a.m. UTC | #1 
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
> +    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?
> +        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
> +    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
> +        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.
I initially thought VTDHostIOMMUContext was, sorry for the misunderstanding.

Do you expect other HostIOMMUContext backends? Given the name and ops,
it looks really related to VFIO?

Thanks

Eric


> +};
> +
> +static void vfio_register_types(void)
> +{
> +    type_register_static(&vfio_host_iommu_context_info);
> +}
> +
> +type_init(vfio_register_types)
> diff --git a/include/hw/iommu/host_iommu_context.h b/include/hw/iommu/host_iommu_context.h
> index 35c4861..227c433 100644
> --- a/include/hw/iommu/host_iommu_context.h
> +++ b/include/hw/iommu/host_iommu_context.h
> @@ -33,6 +33,9 @@
>  #define TYPE_HOST_IOMMU_CONTEXT "qemu:host-iommu-context"
>  #define HOST_IOMMU_CONTEXT(obj) \
>          OBJECT_CHECK(HostIOMMUContext, (obj), TYPE_HOST_IOMMU_CONTEXT)
> +#define HOST_IOMMU_CONTEXT_CLASS(klass) \
> +        OBJECT_CLASS_CHECK(HostIOMMUContextClass, (klass), \
> +                         TYPE_HOST_IOMMU_CONTEXT)
>  #define HOST_IOMMU_CONTEXT_GET_CLASS(obj) \
>          OBJECT_GET_CLASS(HostIOMMUContextClass, (obj), \
>                           TYPE_HOST_IOMMU_CONTEXT)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index fd56420..0b07303 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -26,12 +26,15 @@
>  #include "qemu/notify.h"
>  #include "ui/console.h"
>  #include "hw/display/ramfb.h"
> +#include "hw/iommu/host_iommu_context.h"
>  #ifdef CONFIG_LINUX
>  #include <linux/vfio.h>
>  #endif
>  
>  #define VFIO_MSG_PREFIX "vfio %s: "
>  
> +#define TYPE_VFIO_HOST_IOMMU_CONTEXT "qemu:vfio-host-iommu-context"
> +
>  enum {
>      VFIO_DEVICE_TYPE_PCI = 0,
>      VFIO_DEVICE_TYPE_PLATFORM = 1,
> @@ -71,6 +74,7 @@ typedef struct VFIOContainer {
>      MemoryListener listener;
>      MemoryListener prereg_listener;
>      unsigned iommu_type;
> +    HostIOMMUContext iommu_ctx;
>      Error *error;
>      bool initialized;
>      unsigned long pgsizes;
>
Yi Liu March 31, 2020, 10:59 a.m. UTC | #2 
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,
};

Regards,
Yi Liu
Eric Auger March 31, 2020, 11:15 a.m. UTC | #3 
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.

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

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.

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

Thanks

Eric
> 
> Regards,
> Yi Liu
>
Yi Liu March 31, 2020, 12:54 p.m. UTC | #4 
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
diff mbox series

Patch

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;
+    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);
+        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;
+    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);
+        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,
+};
+
+static void vfio_register_types(void)
+{
+    type_register_static(&vfio_host_iommu_context_info);
+}
+
+type_init(vfio_register_types)
diff --git a/include/hw/iommu/host_iommu_context.h b/include/hw/iommu/host_iommu_context.h
index 35c4861..227c433 100644
--- a/include/hw/iommu/host_iommu_context.h
+++ b/include/hw/iommu/host_iommu_context.h
@@ -33,6 +33,9 @@ 
 #define TYPE_HOST_IOMMU_CONTEXT "qemu:host-iommu-context"
 #define HOST_IOMMU_CONTEXT(obj) \
         OBJECT_CHECK(HostIOMMUContext, (obj), TYPE_HOST_IOMMU_CONTEXT)
+#define HOST_IOMMU_CONTEXT_CLASS(klass) \
+        OBJECT_CLASS_CHECK(HostIOMMUContextClass, (klass), \
+                         TYPE_HOST_IOMMU_CONTEXT)
 #define HOST_IOMMU_CONTEXT_GET_CLASS(obj) \
         OBJECT_GET_CLASS(HostIOMMUContextClass, (obj), \
                          TYPE_HOST_IOMMU_CONTEXT)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index fd56420..0b07303 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -26,12 +26,15 @@ 
 #include "qemu/notify.h"
 #include "ui/console.h"
 #include "hw/display/ramfb.h"
+#include "hw/iommu/host_iommu_context.h"
 #ifdef CONFIG_LINUX
 #include <linux/vfio.h>
 #endif
 
 #define VFIO_MSG_PREFIX "vfio %s: "
 
+#define TYPE_VFIO_HOST_IOMMU_CONTEXT "qemu:vfio-host-iommu-context"
+
 enum {
     VFIO_DEVICE_TYPE_PCI = 0,
     VFIO_DEVICE_TYPE_PLATFORM = 1,
@@ -71,6 +74,7 @@  typedef struct VFIOContainer {
     MemoryListener listener;
     MemoryListener prereg_listener;
     unsigned iommu_type;
+    HostIOMMUContext iommu_ctx;
     Error *error;
     bool initialized;
     unsigned long pgsizes;