> From: Peter Xu [mailto:pet...@redhat.com] > Sent: Friday, March 9, 2018 3:06 PM > To: Liu, Yi L <yi.l....@intel.com> [...] > > > > > > + > > > > > > 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
