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

[Liu, Yi L]

> From: Peter Xu [mailto:address@hidden
> Sent: Friday, March 9, 2018 3:06 PM
> To: Liu, Yi L <address@hidden>
[...]
> > > > > > +
> > > > > >  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.
> > >
> > > I agree that the things are mostly managed by vIOMMU, but I still
> > > cannot understand why vfio must have its own register hook.
> > >
> > > Let me try to explain a bit more on my question.  Basically I was
> > > asking about whether we can removet the register/unregister hook in
> > > the SVAOps, instead we can have something like (I'll start to use pasid 
> > > as prefix):
> > >
> > > struct PCIPASIDOps {
> > >     void (*pasid_bind_table)(PCIBus *bus, int32_t devfn, ...);
> > >     void (*pasid_invalidate_extend_iotlb)(PCIBus *bus, int32_t
> > > devfn, ...) };
> > >
> > > Firstly we keep the bind_table operation, but instead of the
> > > reg/unreg we only provide a hook that device can override to listen to 
> > > extend
> iotlb invalidations.
> >
> > Yeah, I also considered do invalidation this manner. I turned to the one in 
> > this
> patch.
> > Reason as below:
> >     the invalidate operation is supposed to pass down thru vfio container 
> > IOCTL, for
> >     each pasid_invalidate_extend_iotlb() calling, it needs to figure out a 
> > vfio
> container,
> >     which may be time consuming. Pls refer to 
> > vfio_get_container_from_busdev() in
> this
> >     patch. If we expose register/unregister hook, searching container will 
> > happen
> only in
> >     the register/unregister phase. And future invalidation could only be 
> > notifier firing.
> > With the reason above, I chose the register/unregister hook solution.
> > If there is solution to save the container searching, it would be
> > better to do it in your proposal. Pls feel free to let me know if any idea 
> > from you.
> 
> If there is PCIBus* and devfn passed into
> pasid_invalidate_extend_iotlb() (let's assume it's called this way), then 
> IMHO we can
> get the PCIDevice*, which can be upcast to a VFIOPCIDevice, then would
> VFIOPCIDevice.vbasedev.group->container be the container for that device?

Good catch. Would apply. Let me try to use it in next version.

Thanks,
Yi Liu