Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier

[Liu, Yi L]

> From: Peter Xu [mailto:address@hidden
> Sent: Tuesday, March 6, 2018 2:45 PM
> Subject: Re: [PATCH v3 07/12] vfio/pci: register sva notifier
> 
> On Thu, Mar 01, 2018 at 06:33:30PM +0800, Liu, Yi L wrote:
> > This patch shows how sva notifier is registered. And provided an
> > example by registering notify func for tlb flush propagation.
> >
> > Signed-off-by: Liu, Yi L <address@hidden>
> > ---
> >  hw/vfio/pci.c | 55
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index a60a4d7..b7297cc
> > 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2775,6 +2775,26 @@ static void 
> > vfio_unregister_req_notifier(VFIOPCIDevice
> *vdev)
> >      vdev->req_enabled = false;
> >  }
> >
> > +static VFIOContainer *vfio_get_container_from_busdev(PCIBus *bus,
> > +                                                     int32_t devfn) {
> > +    VFIOGroup *group;
> > +    VFIOPCIDevice *vdev_iter;
> > +    VFIODevice *vbasedev_iter;
> > +    PCIDevice *pdev_iter;
> > +
> > +    QLIST_FOREACH(group, &vfio_group_list, next) {
> > +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> > +            vdev_iter = container_of(vbasedev_iter, VFIOPCIDevice, 
> > vbasedev);
> > +            pdev_iter = &vdev_iter->pdev;
> > +            if (pci_get_bus(pdev_iter) == bus && pdev_iter->devfn == 
> > devfn) {
> > +                return group->container;
> > +            }
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> >  static void vfio_pci_device_sva_bind_pasid_table(PCIBus *bus,
> >                   int32_t devfn, uint64_t pasidt_addr, uint32_t size)
> > { @@ -2783,11 +2803,42 @@ static void
> > vfio_pci_device_sva_bind_pasid_table(PCIBus *bus,
> >      So far, Intel VT-d and AMD IOMMU requires it. */  }
> >
> > +static void vfio_iommu_sva_tlb_invalidate_notify(IOMMUSVANotifier *n,
> > +                                                 IOMMUSVAEventData
> > +*event_data) {
> > +/*  Sample code, would be detailed in coming virt-SVA patchset.
> > +    VFIOGuestIOMMUSVAContext *gsva_ctx;
> > +    IOMMUSVAContext *sva_ctx;
> > +    VFIOContainer *container;
> > +
> > +    gsva_ctx = container_of(n, VFIOGuestIOMMUSVAContext, n);
> > +    container = gsva_ctx->container;
> > +
> > +    TODO: forward to host through VFIO IOCTL
> 
> IMHO if the series is not ready for merging, we can still mark it as RFC and 
> declare
> that so people won't need to go into details of the patches.

Thanks for the suggestion. Actually, I was hesitating it. As you may know, this 
is actually
3rd version of this effort. But yes, I would follow your suggestion in coming 
versions.

> > +*/
> > +}
> > +
> >  static void vfio_pci_device_sva_register_notifier(PCIBus *bus,
> >                            int32_t devfn, IOMMUSVAContext *sva_ctx)  {
> > -    /* Register notifier for TLB invalidation propagation
> > -       */
> > +    VFIOContainer *container = vfio_get_container_from_busdev(bus,
> > + devfn);
> > +
> > +    if (container != NULL) {
> > +        VFIOGuestIOMMUSVAContext *gsva_ctx;
> > +        gsva_ctx = g_malloc0(sizeof(*gsva_ctx));
> > +        gsva_ctx->sva_ctx = sva_ctx;
> > +        gsva_ctx->container = container;
> > +        QLIST_INSERT_HEAD(&container->gsva_ctx_list,
> > +                          gsva_ctx,
> > +                          gsva_ctx_next);
> > +       /* Register vfio_iommu_sva_tlb_invalidate_notify with event flag
> > +           IOMMU_SVA_EVENT_TLB_INV */
> > +        iommu_sva_notifier_register(sva_ctx,
> > +                                    &gsva_ctx->n,
> > +                                    vfio_iommu_sva_tlb_invalidate_notify,
> > +                                    IOMMU_SVA_EVENT_TLB_INV);
> 
> I would squash this patch into previous one since basically this is only part 
> of the
> implementation to provide vfio-speicific register hook.

sure.

> But a more important question is... why this?
> 
> IMHO the notifier registration can be general for PCI.  Why vfio needs to 
> provide it's
> own register callback?  Would it be enough if it only provides its own notify 
> callback?

The notifiers are in VFIO. However, the registration is controlled by vIOMMU 
emulator.
In this series, PASID tagged Address Space is introduced. And the new notifiers 
are for
such Address Space. Such Address Space is created and deleted in vIOMMU 
emulator.
So the notifier registration needs to happen accordingly.

e.g. guest SVM application bind a device to a process, it programs the guest 
iommu
translation structure, vIOMMU emulator captures the change, and create a PASID
tagged Address Space for it and register notifiers.

That's why I do it in such a manner.

Thanks,
Yi Liu