RE: [PATCH v4] memory: Optimize replay of guest mapping

[Duan, Zhenzhong]

>-----Original Message-----
>From: Peter Maydell <peter.maydell@linaro.org>
>Sent: Thursday, April 20, 2023 12:10 AM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; mst@redhat.com; peterx@redhat.com;
>jasowang@redhat.com; marcel.apfelbaum@gmail.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>david@redhat.com; philmd@linaro.org; Peng, Chao P
><chao.p.peng@intel.com>
>Subject: Re: [PATCH v4] memory: Optimize replay of guest mapping
>
>On Wed, 19 Apr 2023 at 11:41, Zhenzhong Duan <zhenzhong.duan@intel.com>
>wrote:
>>
>> On x86, there are two notifiers registered due to vtd-ir memory region
>> splitting the entire address space. During replay of the address space
>> for each notifier, the whole address space is scanned which is
>> unnecessary. We only need to scan the space belong to notifier
>> monitored space.
>>
>> While on x86 IOMMU memory region spans over entire address space, but
>> on some other platforms(e.g. arm mps3-an547), IOMMU memory region is
>> only a window in the entire address space. User could register a
>> notifier with arbitrary scope beyond IOMMU memory region. Though in
>> current implementation replay is only triggered by VFIO and dirty page
>> sync with notifiers derived from memory region section, but this isn't
>> guaranteed in the future.
>>
>> So, we replay the intersection part of IOMMU memory region and
>> notifier's scope in memory_region_iommu_replay(). Update doc comment
>> to match this change.
>
>Hi; I have a couple of minor wording tweaks, and one question about the docs:
>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h index
>> 15ade918baa4..61da32d8a428 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -425,12 +425,13 @@ struct IOMMUMemoryRegionClass {
>>       * Called to handle memory_region_iommu_replay().
>>       *
>>       * The default implementation of memory_region_iommu_replay() is to
>> -     * call the IOMMU translate method for every page in the address space
>> +     * call the IOMMU translate method for every page falling in the
>> +     * intersection part of IOMMU memory region and notifier's scope
>
>"falling in the intersection of the IOMMU MemoryRegion and the
>MemoryRegion which the notifier was registered for"
>
>>       * with flag == IOMMU_NONE and then call the notifier if translate
>>       * returns a valid mapping. If this method is implemented then it
>>       * overrides the default behaviour, and must provide the full semantics
>>       * of memory_region_iommu_replay(), by calling @notifier for every
>> -     * translation present in the IOMMU.
>> +     * translation present in the intersection part.
>
>"present in the IOMMU that is within the MemoryRegion the notifier was
>registered for."
>
>Question: is it OK for an implementation of this method to call the notifier 
>for
>translations that are in the IOMMU and which are not in the scope of the
>notifier (ie which are outside the intersection) ? Or must it specifically 
>restrict
>itself to only calling the notifier for translations which are inside the 
>notifier's
>range ?
In the calling path to notifier->notify(), memory_region_notify_iommu_one()
ensures passing an IOMMUTLBEntry in the notifier's range.
I think it's ok for an implementation of replay() to walk over the entire
IOMMU MemoryRegion because unrelated entries are filtered as above.
It's just less optimistic which this patch try to address for intel_iommu.

>
>If the latter, we need to check all 4 existing implementations of this method 
>to
>ensure that they are not sending notifications they should not; if the former,
>we should document that implementations have that leeway.
I think the latter and memory_region_notify_iommu_one() already ensure it.

Thanks
Zhenzhong