On Mon, Dec 05, 2022 at 12:23:20PM +0800, Jason Wang wrote: > On Fri, Dec 2, 2022 at 12:25 AM Peter Xu <pet...@redhat.com> wrote: > > > > It seems not super clear on when iova_tree is used, and why. Add a rich > > comment above iova_tree to track why we needed the iova_tree, and when we > > need it. > > > > Suggested-by: Jason Wang <jasow...@redhat.com> > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > include/hw/i386/intel_iommu.h | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > index 46d973e629..8d130ab2e3 100644 > > --- a/include/hw/i386/intel_iommu.h > > +++ b/include/hw/i386/intel_iommu.h > > @@ -109,7 +109,35 @@ struct VTDAddressSpace { > > QLIST_ENTRY(VTDAddressSpace) next; > > /* Superset of notifier flags that this address space has */ > > IOMMUNotifierFlag notifier_flags; > > - IOVATree *iova_tree; /* Traces mapped IOVA ranges */ > > + /* > > + * @iova_tree traces mapped IOVA ranges. > > + * > > + * The tree is not needed if no MAP notifiers is registered with > > + * current VTD address space, because all UNMAP (including iotlb or > > + * dev-iotlb) events can be transparently delivered to !MAP iommu > > + * notifiers. > > So this means the UNMAP notifier doesn't need to be as accurate as > MAP. (Should we document it in the notifier headers)?
Yes. > > For MAP[a, b] MAP[b, c] we can do a UNMAP[a. c]. IIUC a better way to say this is, for MAP[a, b] we can do an UNMAP[a-X, b+Y] as long as the range covers [a, b]? > > > + * > > + * The tree OTOH is required for MAP typed iommu notifiers for a few > > + * reasons. > > + * > > + * Firstly, there's no way to identify whether an PSI event is MAP or > > + * UNMAP within the PSI message itself. Without having prior knowledge > > + * of existing state vIOMMU doesn't know whether it should notify MAP > > + * or UNMAP for a PSI message it received. > > + * > > + * Secondly, PSI received from guest driver (or even a large PSI can > > + * grow into a DSI at least with Linux intel-iommu driver) can be > > + * larger in range than the newly mapped ranges for either MAP or UNMAP > > + * events. > > Yes, so I think we need a document that the UNMAP handler should be > prepared for this. How about I squash below into this same patch? diff --git a/include/exec/memory.h b/include/exec/memory.h index 91f8a2395a..c83bd11a68 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -129,6 +129,24 @@ struct IOMMUTLBEntry { /* * Bitmap for different IOMMUNotifier capabilities. Each notifier can * register with one or multiple IOMMU Notifier capability bit(s). + * + * Normally there're two use cases for the notifiers: + * + * (1) When the device needs accurate synchronizations of the vIOMMU page + * tables, it needs to register with both MAP|UNMAP notifies (which + * is defined as IOMMU_NOTIFIER_IOTLB_EVENTS below). As long as MAP + * events are registered, the notifications will be accurate but + * there's overhead on synchronizing the guest vIOMMU page tables. + * + * (2) When the device doesn't need accurate synchronizations of the + * vIOMMU page tables (when the device can both cache translations + * and requesting to translate dynamically during DMA process), it + * needs to register only with UNMAP or DEVIOTLB_UNMAP notifies. + * Note that in such working mode shadow page table is not used for + * vIOMMU unit on this address space, so the UNMAP messages can be + * actually larger than the real invalidations (just like how the + * Linux IOMMU driver normally works, where an invalidation can be + * enlarged as long as it still covers the target range). */ typedef enum { IOMMU_NOTIFIER_NONE = 0, Thanks, -- Peter Xu