On Tue, Dec 06, 2022 at 02:16:32PM +0100, Eric Auger wrote:
> Hi Peter,
> On 12/6/22 00:28, Peter Xu wrote:
> > 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
> accurate synchronizations sound too vague & subjective to me.
Suggestions?
> > + * 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
> s/requesting/request
> > + * needs to register only with UNMAP or DEVIOTLB_UNMAP notifies.
> would be nice to clarify the distinction between both then
> > + * 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
> I do not catch 'is not used for vIOMMU unit on this address space'
How about: "Note that in this working mode the vIOMMU will not maintain a
shadowed page table for the address space, and 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,
> >
> Thanks
>
> Eric
>
--
Peter Xu
