Re: [RFC v2 09/22] vfio/pci: add iommu_context notifier for pasid alloc/free

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On Wed, Nov 06, 2019 at 12:14:50PM +0000, Liu, Yi L wrote:
> > From: David Gibson [mailto:david@gibson.dropbear.id.au]
> > Sent: Tuesday, October 29, 2019 8:16 PM
> > To: Liu, Yi L <yi.l.liu@intel.com>
> > Subject: Re: [RFC v2 09/22] vfio/pci: add iommu_context notifier for pasid alloc/free
> > 
> > On Thu, Oct 24, 2019 at 08:34:30AM -0400, Liu Yi L wrote:
> > > This patch adds pasid alloc/free notifiers for vfio-pci. It is
> > > supposed to be fired by vIOMMU. VFIO then sends PASID allocation
> > > or free request to host.
> > >
> > > 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>
> > > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > > ---
> > >  hw/vfio/common.c         |  9 ++++++
> > >  hw/vfio/pci.c            | 81
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/iommu/iommu.h | 15 +++++++++
> > >  3 files changed, 105 insertions(+)
> > >
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index d418527..e6ad21c 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -1436,6 +1436,7 @@ static void vfio_disconnect_container(VFIOGroup
> > *group)
> > >      if (QLIST_EMPTY(&container->group_list)) {
> > >          VFIOAddressSpace *space = container->space;
> > >          VFIOGuestIOMMU *giommu, *tmp;
> > > +        VFIOIOMMUContext *giommu_ctx, *ctx;
> > >
> > >          QLIST_REMOVE(container, next);
> > >
> > > @@ -1446,6 +1447,14 @@ static void vfio_disconnect_container(VFIOGroup
> > *group)
> > >              g_free(giommu);
> > >          }
> > >
> > > +        QLIST_FOREACH_SAFE(giommu_ctx, &container->iommu_ctx_list,
> > > +                                                   iommu_ctx_next, ctx) {
> > > +            iommu_ctx_notifier_unregister(giommu_ctx->iommu_ctx,
> > > +                                                      &giommu_ctx->n);
> > > +            QLIST_REMOVE(giommu_ctx, iommu_ctx_next);
> > > +            g_free(giommu_ctx);
> > > +        }
> > > +
> > >          trace_vfio_disconnect_container(container->fd);
> > >          close(container->fd);
> > >          g_free(container);
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 12fac39..8721ff6 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2699,11 +2699,80 @@ static void
> > vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> > >      vdev->req_enabled = false;
> > >  }
> > >
> > > +static void vfio_register_iommu_ctx_notifier(VFIOPCIDevice *vdev,
> > > +                                             IOMMUContext *iommu_ctx,
> > > +                                             IOMMUCTXNotifyFn fn,
> > > +                                             IOMMUCTXEvent event)
> > > +{
> > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > +    VFIOIOMMUContext *giommu_ctx;
> > > +
> > > +    giommu_ctx = g_malloc0(sizeof(*giommu_ctx));
> > > +    giommu_ctx->container = container;
> > > +    giommu_ctx->iommu_ctx = iommu_ctx;
> > > +    QLIST_INSERT_HEAD(&container->iommu_ctx_list,
> > > +                      giommu_ctx,
> > > +                      iommu_ctx_next);
> > > +    iommu_ctx_notifier_register(iommu_ctx,
> > > +                                &giommu_ctx->n,
> > > +                                fn,
> > > +                                event);
> > > +}
> > > +
> > > +static void vfio_iommu_pasid_alloc_notify(IOMMUCTXNotifier *n,
> > > +                                          IOMMUCTXEventData *event_data)
> > > +{
> > > +    VFIOIOMMUContext *giommu_ctx = container_of(n, VFIOIOMMUContext, n);
> > > +    VFIOContainer *container = giommu_ctx->container;
> > > +    IOMMUCTXPASIDReqDesc *pasid_req =
> > > +                              (IOMMUCTXPASIDReqDesc *) event_data->data;
> > > +    struct vfio_iommu_type1_pasid_request req;
> > > +    unsigned long argsz;
> > > +    int pasid;
> > > +
> > > +    argsz = sizeof(req);
> > > +    req.argsz = argsz;
> > > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > > +    req.min_pasid = pasid_req->min_pasid;
> > > +    req.max_pasid = pasid_req->max_pasid;
> > > +
> > > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > +    if (pasid < 0) {
> > > +        error_report("%s: %d, alloc failed", __func__, -errno);
> > > +    }
> > > +    pasid_req->alloc_result = pasid;
> > 
> > Altering the event data from the notifier doesn't make sense.  By
> > definition there can be multiple notifiers on the chain, so in that
> > case which one is responsible for updating the writable field?
> 
> I guess you mean multiple pasid_alloc nofitiers. right?
> 
> It works for VT-d now, as Intel vIOMMU maintains the IOMMUContext
> per-bdf. And there will be only 1 pasid_alloc notifier in the chain. But, I
> agree it is not good if other module just share an IOMMUConext across
> devices. Definitely, it would have multiple pasid_alloc notifiers.

Right.

> How about enforcing IOMMUContext layer to only invoke one successful
> pasid_alloc/free notifier if PASID_ALLOC/FREE event comes? pasid
> alloc/free are really special as it requires feedback. And a potential
> benefit is that the pasid_alloc/free will not be affected by hot plug
> scenario. There will be always a notifier to work for pasid_alloc/free
> work unless all passthru devices are hot plugged. How do you think? Or
> if any other idea?

Hrm, that still doesn't seem right to me.  I don't think a notifier is
really the right mechanism for something that needs to return values.
This seems like something where you need to find a _single_
responsible object and call a method / callback on that specifically.

But it seems to me there's a more fundamental problem here.  AIUI the
idea is that a single IOMMUContext could hold multiple devices.  But
if the devices are responsible for assigning their own pasid values
(by passing that decisionon to the host through vfio) then that really
can't work.

I'm assuming it's impossible from the hardware side to virtualize the
pasids (so that we could assign them from qemu without host
intervention).

If so, then the pasid allocation really has to be a Context level, not
device level operation.  We'd have to wire the VFIO backend up to the
context itself, not a device... I'm not immediately sure how to do
that, though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]