[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier
From: |
Liu, Yi L |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier |
Date: |
Fri, 9 Mar 2018 10:25:27 +0000 |
> 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
- [Qemu-devel] [PATCH v3 04/12] vfio/pci: add notify framework based on IOMMUSVAContext, (continued)
- [Qemu-devel] [PATCH v3 06/12] vfio/pci: provide vfio_pci_sva_ops instance, Liu, Yi L, 2018/03/01
- [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Liu, Yi L, 2018/03/01
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Peter Xu, 2018/03/06
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Liu, Yi L, 2018/03/06
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Peter Xu, 2018/03/06
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Liu, Yi L, 2018/03/08
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier, Peter Xu, 2018/03/09
- Re: [Qemu-devel] [PATCH v3 07/12] vfio/pci: register sva notifier,
Liu, Yi L <=
[Qemu-devel] [PATCH v3 05/12] hw/pci: introduce PCISVAOps to PCIDevice, Liu, Yi L, 2018/03/01
Re: [Qemu-devel] [PATCH v3 05/12] hw/pci: introduce PCISVAOps to PCIDevice, Liu, Yi L, 2018/03/06
[Qemu-devel] [PATCH v3 09/12] intel_iommu: record assigned devices in a list, Liu, Yi L, 2018/03/01
[Qemu-devel] [PATCH v3 08/12] hw/pci: introduce pci_device_notify_iommu(), Liu, Yi L, 2018/03/01