Re: [PATCH] intel-iommu: Document iova_tree

[Peter Xu <peterx@redhat.com>]

On Tue, Dec 06, 2022 at 05:28:01PM +0100, Eric Auger wrote:
> 
> 
> On 12/6/22 17:05, Peter Xu wrote:
> > 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 <peterx@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 <jasowang@redhat.com>
> >>>>> Signed-off-by: Peter Xu <peterx@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?
> Well I would say:
> when the notified device maintains a shadow page table and must to be

s/to//

> notified on each guest MAP (page table entry creation) and UNMAP
> (invalidation) events (VFIO). Both notifications must be accurate so
> that the shadow page table is fully in sync with the guest view.

Thanks, I'll try to squash this into the new version.

> >
> >>> + *       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
> when the notified device maintains a cache of IOMMU translations (IOTLB)
> and is able to fill that cache by requesting translations from the
> vIOMMU through a protocol similar to ATS. In that case the notified
> device only needs to register an UNMAP notifier. In that case the unmap
> notifications are allower to be wider than the strict necessary.

Same here.

> 
> However the problem is since you need to satisfy the VFIO use case, how
> do you detect when you are allowed to invalidate more that the strict
> necessary?

We detect that by checking whether the vtd_as has map notifier registered.
Please feel free to have a look at all sites of vtd_as_has_map_notifier().
We maintain the iova tree only for MAP case currently.

-- 
Peter Xu