Re: [PATCH] intel-iommu: Document iova_tree

[Eric Auger]

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.
> + *       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'
> + *       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