[v1] memory: Optimize replay of guest mapping

[PATCH] memory: Optimize replay of guest mapping

Posted by Zhenzhong Duan 1 year, 2 months ago

On x86, there are two notifiers registered due to vtd-ir memory region
splitting the whole address space. During replay of the address space
for each notifier, the whole address space is scanned which is
unnecessory.

We only need to scan the space belong to notifier montiored space.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
Tested only on x86 with a net card passed to guest, ping/ssh pass.

 hw/i386/intel_iommu.c | 2 +-
 softmmu/memory.c      | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 98a5c304a7d7..6b1de80e8573 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3831,7 +3831,7 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
                 .domain_id = vtd_get_domain_id(s, &ce, vtd_as->pasid),
             };
 
-            vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid);
+            vtd_page_walk(s, &ce, n->start, n->end, &info, vtd_as->pasid);
         }
     } else {
         trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca269b..f096716e6e78 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1923,7 +1923,6 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr)
 
 void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
 {
-    MemoryRegion *mr = MEMORY_REGION(iommu_mr);
     IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
     hwaddr addr, granularity;
     IOMMUTLBEntry iotlb;
@@ -1936,7 +1935,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
 
     granularity = memory_region_iommu_get_min_page_size(iommu_mr);
 
-    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
+    for (addr = n->start; addr < n->end; addr += granularity) {
         iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
         if (iotlb.perm != IOMMU_NONE) {
             n->notify(n, &iotlb);
-- 
2.25.1

Re: [PATCH] memory: Optimize replay of guest mapping

Posted by Jason Wang 1 year, 2 months ago

On Tue, Feb 14, 2023 at 11:43 AM Zhenzhong Duan
<zhenzhong.duan@intel.com> wrote:
>
> On x86, there are two notifiers registered due to vtd-ir memory region
> splitting the whole address space. During replay of the address space
> for each notifier, the whole address space is scanned which is
> unnecessory.
>
> We only need to scan the space belong to notifier montiored space.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> Tested only on x86 with a net card passed to guest, ping/ssh pass.
>
>  hw/i386/intel_iommu.c | 2 +-
>  softmmu/memory.c      | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 98a5c304a7d7..6b1de80e8573 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3831,7 +3831,7 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>                  .domain_id = vtd_get_domain_id(s, &ce, vtd_as->pasid),
>              };
>
> -            vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid);
> +            vtd_page_walk(s, &ce, n->start, n->end, &info, vtd_as->pasid);
>          }
>      } else {
>          trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 9d64efca269b..f096716e6e78 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1923,7 +1923,6 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr)
>
>  void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>  {
> -    MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>      IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
>      hwaddr addr, granularity;
>      IOMMUTLBEntry iotlb;
> @@ -1936,7 +1935,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>
>      granularity = memory_region_iommu_get_min_page_size(iommu_mr);
>
> -    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> +    for (addr = n->start; addr < n->end; addr += granularity) {

Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)?

Thanks

>          iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
>          if (iotlb.perm != IOMMU_NONE) {
>              n->notify(n, &iotlb);
> --
> 2.25.1
>
>

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

Posted by Duan, Zhenzhong 1 year, 2 months ago


>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Sent: Tuesday, February 14, 2023 2:25 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; mst@redhat.com; peterx@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com; david@redhat.com; philmd@linaro.org
>Subject: Re: [PATCH] memory: Optimize replay of guest mapping
>
>On Tue, Feb 14, 2023 at 11:43 AM Zhenzhong Duan
><zhenzhong.duan@intel.com> wrote:
>>
>> On x86, there are two notifiers registered due to vtd-ir memory region
>> splitting the whole address space. During replay of the address space
>> for each notifier, the whole address space is scanned which is
>> unnecessory.
>>
>> We only need to scan the space belong to notifier montiored space.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> Tested only on x86 with a net card passed to guest, ping/ssh pass.
>>
>>  hw/i386/intel_iommu.c | 2 +-
>>  softmmu/memory.c      | 3 +--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
>> 98a5c304a7d7..6b1de80e8573 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3831,7 +3831,7 @@ static void
>vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>>                  .domain_id = vtd_get_domain_id(s, &ce, vtd_as->pasid),
>>              };
>>
>> -            vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid);
>> +            vtd_page_walk(s, &ce, n->start, n->end, &info,
>> + vtd_as->pasid);
>>          }
>>      } else {
>>          trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn),
>> diff --git a/softmmu/memory.c b/softmmu/memory.c index
>> 9d64efca269b..f096716e6e78 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1923,7 +1923,6 @@ uint64_t
>> memory_region_iommu_get_min_page_size(IOMMUMemoryRegion
>*iommu_mr)
>>
>>  void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr,
>> IOMMUNotifier *n)  {
>> -    MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>>      IOMMUMemoryRegionClass *imrc =
>IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
>>      hwaddr addr, granularity;
>>      IOMMUTLBEntry iotlb;
>> @@ -1936,7 +1935,7 @@ void
>> memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr,
>IOMMUNotifier
>> *n)
>>
>>      granularity = memory_region_iommu_get_min_page_size(iommu_mr);
>>
>> -    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
>> +    for (addr = n->start; addr < n->end; addr += granularity) {
>
>Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)?

In current implementation it is.
[n->start, n->end] of notifier is derived from iommu memory region's section
which is a subset of iommu memory region itself.

Thanks
Zhenzhong

Re: [PATCH] memory: Optimize replay of guest mapping

Posted by Peter Xu 1 year, 2 months ago

On Tue, Feb 14, 2023 at 07:04:56AM +0000, Duan, Zhenzhong wrote:
> >> @@ -1936,7 +1935,7 @@ void
> >> memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr,
> >IOMMUNotifier
> >> *n)
> >>
> >>      granularity = memory_region_iommu_get_min_page_size(iommu_mr);
> >>
> >> -    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> >> +    for (addr = n->start; addr < n->end; addr += granularity) {
> >
> >Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)?
> 
> In current implementation it is.
> [n->start, n->end] of notifier is derived from iommu memory region's section
> which is a subset of iommu memory region itself.

Yes, currently it seems to be guaranteed by the callers assuming they're
always doing the right thing.

Maybe it'll worth it to have memory_region_register_iommu_notifier() assert
properly to make sure it always hold true?

The patch itself looks good here, thanks.

-- 
Peter Xu

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

Posted by Duan, Zhenzhong 1 year, 2 months ago


>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Wednesday, February 15, 2023 1:29 AM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: Jason Wang <jasowang@redhat.com>; qemu-devel@nongnu.org;
>mst@redhat.com; pbonzini@redhat.com; richard.henderson@linaro.org;
>eduardo@habkost.net; marcel.apfelbaum@gmail.com; david@redhat.com;
>philmd@linaro.org
>Subject: Re: [PATCH] memory: Optimize replay of guest mapping
>
>On Tue, Feb 14, 2023 at 07:04:56AM +0000, Duan, Zhenzhong wrote:
>> >> @@ -1936,7 +1935,7 @@ void
>> >> memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr,
>> >IOMMUNotifier
>> >> *n)
>> >>
>> >>      granularity = memory_region_iommu_get_min_page_size(iommu_mr);
>> >>
>> >> -    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
>> >> +    for (addr = n->start; addr < n->end; addr += granularity) {
>> >
>> >Is [n->start, n->end] guaranteed to be the subset of
>memory_region_size(mr)?
>>
>> In current implementation it is.
>> [n->start, n->end] of notifier is derived from iommu memory region's
>> section which is a subset of iommu memory region itself.
>
>Yes, currently it seems to be guaranteed by the callers assuming they're
>always doing the right thing.
>
>Maybe it'll worth it to have memory_region_register_iommu_notifier() assert
>properly to make sure it always hold true?

Yea, good suggestion, will do in v2.

Thanks
Zhenzhong