[PATCH 0/1] Remove flatview_simplify()

[PATCH 0/1] Remove flatview_simplify()

[FelixCuioc]

When assign EHCI device to the virtual machine,
after initializing EHCI in seabios,it will continuously
send dma cycles.
Flatview_simplify() will merge a very large range.The IOVA
mappings of the EHCI device will be innocently unmapped
between the delete and add phases of the VFIO MemoryListener.
At this time,the IOMMU will block the DMA cycle.
We want to remove flatview_simplify() is to prevent EHCI
device IOVA mappings from being unmapped.

FelixCuioc (1):
  Remove flatview_simplify()

 softmmu/memory.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

-- 
2.17.1

[PATCH 1/1] Remove flatview_simplify()

[FelixCuioc]

Flatview_simplify() can merge many small memory ranges
into a large one and contains EHCI dma buffers.
For example,the merged range maybe0xc0000-0xbfffffff.
When seabios write PAM register to change the properties
of part of the merged range from RW to readonly,
this action cause the merged IOVA mapping will be
unmapped.But EHCI device still send DMA cycles
and then IOMMU blocks the DMA cycles of EHCI device.

Because the merged range is too large,there will be
problems even if EHCI device is configured to not
allocate buffers in low memory.
So this patch wants to remove flatview_simplify() completely.

Signed-off-by: FelixCuioc <FelixCui-oc@zhaoxin.com>
---
 softmmu/memory.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e4..aaace8c03f 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -314,42 +314,6 @@ void flatview_unref(FlatView *view)
     }
 }
 
-static bool can_merge(FlatRange *r1, FlatRange *r2)
-{
-    return int128_eq(addrrange_end(r1->addr), r2->addr.start)
-        && r1->mr == r2->mr
-        && int128_eq(int128_add(int128_make64(r1->offset_in_region),
-                                r1->addr.size),
-                     int128_make64(r2->offset_in_region))
-        && r1->dirty_log_mask == r2->dirty_log_mask
-        && r1->romd_mode == r2->romd_mode
-        && r1->readonly == r2->readonly
-        && r1->nonvolatile == r2->nonvolatile;
-}
-
-/* Attempt to simplify a view by merging adjacent ranges */
-static void flatview_simplify(FlatView *view)
-{
-    unsigned i, j, k;
-
-    i = 0;
-    while (i < view->nr) {
-        j = i + 1;
-        while (j < view->nr
-               && can_merge(&view->ranges[j-1], &view->ranges[j])) {
-            int128_addto(&view->ranges[i].addr.size, view->ranges[j].addr.size);
-            ++j;
-        }
-        ++i;
-        for (k = i; k < j; k++) {
-            memory_region_unref(view->ranges[k].mr);
-        }
-        memmove(&view->ranges[i], &view->ranges[j],
-                (view->nr - j) * sizeof(view->ranges[j]));
-        view->nr -= j - i;
-    }
-}
-
 static bool memory_region_big_endian(MemoryRegion *mr)
 {
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -735,7 +699,6 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
                              addrrange_make(int128_zero(), int128_2_64()),
                              false, false);
     }
-    flatview_simplify(view);
 
     view->dispatch = address_space_dispatch_new(view);
     for (i = 0; i < view->nr; i++) {
-- 
2.17.1

Re: [PATCH 1/1] Remove flatview_simplify()

[Richard Henderson]

On 3/29/21 6:14 AM, FelixCuioc wrote:
> Flatview_simplify() can merge many small memory ranges
> into a large one and contains EHCI dma buffers.
> For example,the merged range maybe0xc0000-0xbfffffff.
> When seabios write PAM register to change the properties
> of part of the merged range from RW to readonly,
> this action cause the merged IOVA mapping will be
> unmapped.But EHCI device still send DMA cycles
> and then IOMMU blocks the DMA cycles of EHCI device.

You've described the problem, and it is quite obviously *not* in memory.c.


r~

Re: [PATCH 1/1] Remove flatview_simplify()

[Paolo Bonzini]

On 30/03/21 18:33, Richard Henderson wrote:
> 
>> Flatview_simplify() can merge many small memory ranges
>> into a large one and contains EHCI dma buffers.
>> For example,the merged range maybe0xc0000-0xbfffffff.
>> When seabios write PAM register to change the properties
>> of part of the merged range from RW to readonly,
>> this action cause the merged IOVA mapping will be
>> unmapped.But EHCI device still send DMA cycles
>> and then IOMMU blocks the DMA cycles of EHCI device.
> 
> You've described the problem, and it is quite obviously *not* in memory.c.

Well, sort of.

The problem is that neither VFIO nor KVM support atomically switching 
the memory map.  For KVM that would be possible, for VFIO based on past 
discussion it would be much harder.  Removing flatview_simplify() seems 
to be the easiest way to bypass the issue.

That said, perhaps it's better to keep the simplification within a 
page-sized range, to avoid introducing subpages unnecessarily.

Paolo

[PATCH 0/1] Remove flatview_simplify()

[FelixCuioc]

When assign EHCI device to the virtual machine,
after initializing EHCI in seabios,it will continuously
send dma cycles.
Flatview_simplify() will merge a very large range.The IOVA
mappings of the EHCI device will be innocently unmapped
between the delete and add phases of the VFIO MemoryListener.
At this time,the IOMMU will block the DMA cycle.
We want to remove flatview_simplify() is to prevent EHCI
device IOVA mappings from being unmapped.

FelixCui-oc (1):
  Remove flatview_simplify()

 softmmu/memory.c | 1 -
 1 file changed, 1 deletion(-)

-- 
2.17.1

[PATCH 1/1] Remove flatview_simplify()

[FelixCuioc]

From: FelixCui-oc <FelixCui-oc@zhaoxin.com>

Flatview_simplify() will cause some innocent mappings
to be unmapped.

Signed-off-by: FelixCui-oc <FelixCui-oc@zhaoxin.com>
Signed-off-by: FelixCuioc <FelixCui-oc@zhaoxin.com>
---
 softmmu/memory.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 11ca94d037..66d36dcac4 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -733,7 +733,6 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
                              addrrange_make(int128_zero(), int128_2_64()),
                              false, false);
     }
-    flatview_simplify(view);
 
     view->dispatch = address_space_dispatch_new(view);
     for (i = 0; i < view->nr; i++) {
-- 
2.17.1

Re: [PATCH 1/1] Remove flatview_simplify()

[Paolo Bonzini]

On 06/12/20 09:21, FelixCuioc wrote:
> From: FelixCui-oc <FelixCui-oc@zhaoxin.com>
> 
> Flatview_simplify() will cause some innocent mappings
> to be unmapped.
> 
> Signed-off-by: FelixCui-oc <FelixCui-oc@zhaoxin.com>
> Signed-off-by: FelixCuioc <FelixCui-oc@zhaoxin.com>
> ---
>   softmmu/memory.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 11ca94d037..66d36dcac4 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -733,7 +733,6 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
>                                addrrange_make(int128_zero(), int128_2_64()),
>                                false, false);
>       }
> -    flatview_simplify(view);
>   
>       view->dispatch = address_space_dispatch_new(view);
>       for (i = 0; i < view->nr; i++) {
> 

This will give a warning about an unused static function.  You have to 
clean that up too.

Also please include more information from the previous discussions in 
the commut message, thanks.

Paolo