* Re: [Qemu-devel] [PATCH] memory: update coalesced_range on transaction_commit
2018-11-28 9:17 [Qemu-devel] [PATCH] memory: update coalesced_range on transaction_commit Atsushi Nemoto
@ 2018-11-28 10:03 ` Paolo Bonzini
2018-11-28 16:30 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2018-11-28 10:03 UTC (permalink / raw)
To: Atsushi Nemoto, qemu-devel
On 28/11/18 10:17, Atsushi Nemoto wrote:
> The e1000 driver calls memory_region_add_coalescing but
> kvm_coalesce_mmio_region never been called for those regions.
>
> Update coalesced_range on memory_region_transaction_commit to fix this.
>
> It seems very old bug since memory region API
> was merged on commit 093bc2cd885e ("Hierarchical memory region API").
>
> Signed-off-by: Atsushi Nemoto <atsushi.nemoto@sord.co.jp>
> ---
> memory.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index d14c6dec1d..f773b0be50 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1021,6 +1021,8 @@ void memory_region_transaction_begin(void)
> ++memory_region_transaction_depth;
> }
>
> +static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as);
> +
> void memory_region_transaction_commit(void)
> {
> AddressSpace *as;
> @@ -1038,6 +1040,7 @@ void memory_region_transaction_commit(void)
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> address_space_set_flatview(as);
> address_space_update_ioeventfds(as);
> + memory_region_update_coalesced_range_as(NULL, as);
> }
> memory_region_update_pending = false;
> ioeventfd_update_pending = false;
> @@ -2142,7 +2145,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
>
> view = address_space_get_flatview(as);
> FOR_EACH_FLAT_RANGE(fr, view) {
> - if (fr->mr == mr) {
> + if (fr->mr == mr || (!mr && !QTAILQ_EMPTY(&fr->mr->coalesced))) {
> section = (MemoryRegionSection) {
> .fv = view,
> .offset_within_address_space = int128_get64(fr->addr.start),
I'm worried that coalesced_io_del would be skipped when the region is
deleted. I think the right place to add the coalesced_io_{add,del}
calls is address_space_update_topology_pass.
Let me test an alternative fix and post it; thanks for the report in the
meanwhile!
Paolo
> @@ -2152,7 +2155,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
> MEMORY_LISTENER_CALL(as, coalesced_io_del, Reverse, §ion,
> int128_get64(fr->addr.start),
> int128_get64(fr->addr.size));
> - QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
> + QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
> tmp = addrrange_shift(cmr->addr,
> int128_sub(fr->addr.start,
> int128_make64(fr->offset_in_region)));
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] memory: update coalesced_range on transaction_commit
2018-11-28 9:17 [Qemu-devel] [PATCH] memory: update coalesced_range on transaction_commit Atsushi Nemoto
2018-11-28 10:03 ` Paolo Bonzini
@ 2018-11-28 16:30 ` Paolo Bonzini
2018-11-29 4:02 ` Atsushi Nemoto
1 sibling, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2018-11-28 16:30 UTC (permalink / raw)
To: Atsushi Nemoto, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
On 28/11/18 10:17, Atsushi Nemoto wrote:
> The e1000 driver calls memory_region_add_coalescing but
> kvm_coalesce_mmio_region never been called for those regions.
>
> Update coalesced_range on memory_region_transaction_commit to fix this.
>
> It seems very old bug since memory region API
> was merged on commit 093bc2cd885e ("Hierarchical memory region API").
>
> Signed-off-by: Atsushi Nemoto <atsushi.nemoto@sord.co.jp>
I attach the patch I came up with. What do you think?
Thanks,
Paolo
> memory.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index d14c6dec1d..f773b0be50 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1021,6 +1021,8 @@ void memory_region_transaction_begin(void)
> ++memory_region_transaction_depth;
> }
>
> +static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as);
> +
> void memory_region_transaction_commit(void)
> {
> AddressSpace *as;
> @@ -1038,6 +1040,7 @@ void memory_region_transaction_commit(void)
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> address_space_set_flatview(as);
> address_space_update_ioeventfds(as);
> + memory_region_update_coalesced_range_as(NULL, as);
> }
> memory_region_update_pending = false;
> ioeventfd_update_pending = false;
> @@ -2142,7 +2145,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
>
> view = address_space_get_flatview(as);
> FOR_EACH_FLAT_RANGE(fr, view) {
> - if (fr->mr == mr) {
> + if (fr->mr == mr || (!mr && !QTAILQ_EMPTY(&fr->mr->coalesced))) {
> section = (MemoryRegionSection) {
> .fv = view,
> .offset_within_address_space = int128_get64(fr->addr.start),
> @@ -2152,7 +2155,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
> MEMORY_LISTENER_CALL(as, coalesced_io_del, Reverse, §ion,
> int128_get64(fr->addr.start),
> int128_get64(fr->addr.size));
> - QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
> + QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
> tmp = addrrange_shift(cmr->addr,
> int128_sub(fr->addr.start,
> int128_make64(fr->offset_in_region)));
>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: coalesce.patch --]
[-- Type: text/x-patch; name="coalesce.patch", Size: 5537 bytes --]
diff --git a/memory.c b/memory.c
index d14c6dec1d..5759f74034 100644
--- a/memory.c
+++ b/memory.c
@@ -217,6 +217,7 @@ struct FlatRange {
bool romd_mode;
bool readonly;
bool nonvolatile;
+ int has_coalesced_range;
};
#define FOR_EACH_FLAT_RANGE(var, view) \
@@ -650,6 +651,7 @@ static void render_memory_region(FlatView *view,
fr.romd_mode = mr->romd_mode;
fr.readonly = readonly;
fr.nonvolatile = nonvolatile;
+ fr.has_coalesced_range = 0;
/* Render the region itself into any gaps left by the current view. */
for (i = 0; i < view->nr && int128_nz(remain); ++i) {
@@ -850,6 +852,49 @@ static void address_space_update_ioeventfds(AddressSpace *as)
flatview_unref(view);
}
+static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
+{
+ if (!fr->has_coalesced_range) {
+ return;
+ }
+
+ if (--fr->has_coalesced_range > 0) {
+ return;
+ }
+
+ MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
+ int128_get64(fr->addr.start),
+ int128_get64(fr->addr.size));
+}
+
+static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
+{
+ MemoryRegion *mr = fr->mr;
+ CoalescedMemoryRange *cmr;
+ AddrRange tmp;
+
+ if (QTAILQ_EMPTY(&mr->coalesced)) {
+ return;
+ }
+
+ if (fr->has_coalesced_range++) {
+ return;
+ }
+
+ QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
+ tmp = addrrange_shift(cmr->addr,
+ int128_sub(fr->addr.start,
+ int128_make64(fr->offset_in_region)));
+ if (!addrrange_intersects(tmp, fr->addr)) {
+ continue;
+ }
+ tmp = addrrange_intersection(tmp, fr->addr);
+ MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, coalesced_io_add,
+ int128_get64(tmp.start),
+ int128_get64(tmp.size));
+ }
+}
+
static void address_space_update_topology_pass(AddressSpace *as,
const FlatView *old_view,
const FlatView *new_view,
@@ -882,6 +927,7 @@ static void address_space_update_topology_pass(AddressSpace *as,
/* In old but not in new, or in both but attributes changed. */
if (!adding) {
+ flat_range_coalesced_io_del(frold, as);
MEMORY_LISTENER_UPDATE_REGION(frold, as, Reverse, region_del);
}
@@ -889,7 +935,9 @@ static void address_space_update_topology_pass(AddressSpace *as,
} else if (frold && frnew && flatrange_equal(frold, frnew)) {
/* In both and unchanged (except logging may have changed) */
- if (adding) {
+ if (!adding) {
+ flat_range_coalesced_io_del(frold, as);
+ } else {
MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, region_nop);
if (frnew->dirty_log_mask & ~frold->dirty_log_mask) {
MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, log_start,
@@ -901,6 +949,7 @@ static void address_space_update_topology_pass(AddressSpace *as,
frold->dirty_log_mask,
frnew->dirty_log_mask);
}
+ flat_range_coalesced_io_add(frnew, as);
}
++iold;
@@ -910,6 +959,7 @@ static void address_space_update_topology_pass(AddressSpace *as,
if (adding) {
MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, region_add);
+ flat_range_coalesced_io_add(frnew, as);
}
++inew;
@@ -2136,34 +2186,12 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
{
FlatView *view;
FlatRange *fr;
- CoalescedMemoryRange *cmr;
- AddrRange tmp;
- MemoryRegionSection section;
view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
if (fr->mr == mr) {
- section = (MemoryRegionSection) {
- .fv = view,
- .offset_within_address_space = int128_get64(fr->addr.start),
- .size = fr->addr.size,
- };
-
- MEMORY_LISTENER_CALL(as, coalesced_io_del, Reverse, §ion,
- int128_get64(fr->addr.start),
- int128_get64(fr->addr.size));
- QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
- tmp = addrrange_shift(cmr->addr,
- int128_sub(fr->addr.start,
- int128_make64(fr->offset_in_region)));
- if (!addrrange_intersects(tmp, fr->addr)) {
- continue;
- }
- tmp = addrrange_intersection(tmp, fr->addr);
- MEMORY_LISTENER_CALL(as, coalesced_io_add, Forward, §ion,
- int128_get64(tmp.start),
- int128_get64(tmp.size));
- }
+ flat_range_coalesced_io_del(fr, as);
+ flat_range_coalesced_io_add(fr, as);
}
}
flatview_unref(view);
^ permalink raw reply related [flat|nested] 4+ messages in thread