Re: [PATCH] intel-iommu: Document iova_tree

[Eric Auger]

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

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?

Eric
 
>> 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
>>