[PATCH v2 0/7] kvm: Get coalesced MMIO flushing out of the hot-path

[PATCH v2 0/7] kvm: Get coalesced MMIO flushing out of the hot-path

[Jan Kiszka]

Changes in v2:
 - added memory_region_clear_flush_coalesced
 - call memory_region_clear_flush_coalesced from
   memory_region_clear_coalescing
 - wrap all region manipulations via memory_region_transaction_begin/
   commit internally
 - flush coalesced MMIO only on memory_region_transaction_begin

Original description:

We currently flush the coalesced MMIO buffer on every vmexit to
userspace. KVM only provides a single buffer per VM, so a central lock
is required to read from it. This is a contention point given a large
enough VCPU set. Moreover, we need to hold the BQL while replaying the
queued requests, probably for a long time until there is more fine
grained locking available. Good reasons to overcome the unconditional
flush.

The series achieves this by flushing only on selected memory region
accesses, either generically via the memory access dispatcher or
directly on certain VGA PIO accesses that are not yet fully converted.
Another reason to flush are remappings or other relevant region state
changes.

Jan Kiszka (7):
  i82378: Remove bogus MMIO coalescing
  memory: Flush coalesced MMIO on selected region access
  memory: Use transaction_begin/commit also for single-step operations
  memory: Fold memory_region_update_topology into
    memory_region_transaction_commit
  memory: Flush coalesced MMIO on mapping and state changes
  VGA: Flush coalesced MMIO on related MMIO/PIO accesses
  kvm: Stop flushing coalesced MMIO on vmexit

 hw/cirrus_vga.c |    7 ++++
 hw/i82378.c     |    1 -
 hw/qxl.c        |    1 +
 hw/vga-isa-mm.c |    1 +
 hw/vga.c        |    5 +++
 hw/vmware_vga.c |    1 +
 kvm-all.c       |    2 -
 memory.c        |  101 +++++++++++++++++++++++++++++++-----------------------
 memory.h        |   24 +++++++++++++
 9 files changed, 97 insertions(+), 46 deletions(-)

-- 
1.7.3.4

[PATCH v2 1/7] i82378: Remove bogus MMIO coalescing

[Jan Kiszka]

This MMIO area is an entry gate to legacy PC ISA devices, addressed via
PIO over there. Quite a few of the PIO ports have side effects on access
like starting/stopping timers that must be executed properly ordered
/wrt the CPU. So we have to remove the coalescing mark.

Acked-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i82378.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/i82378.c b/hw/i82378.c
index 9b11d90..2123c14 100644
--- a/hw/i82378.c
+++ b/hw/i82378.c
@@ -225,7 +225,6 @@ static int pci_i82378_init(PCIDevice *dev)
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io);
 
     memory_region_init_io(&s->mem, &i82378_mem_ops, s, "i82378-mem", 0x01000000);
-    memory_region_set_coalescing(&s->mem);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
     /* Make I/O address read only */
-- 
1.7.3.4

[PATCH v2 2/7] memory: Flush coalesced MMIO on selected region access

[Jan Kiszka]

Instead of flushing pending coalesced MMIO requests on every vmexit,
this provides a mechanism to selectively flush when memory regions
related to the coalesced one are accessed. This first of all includes
the coalesced region itself but can also applied to other regions, e.g.
of the same device, by calling memory_region_set_flush_coalesced.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |   21 +++++++++++++++++++++
 memory.h |   24 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index aab4a31..c29a8a3 100644
--- a/memory.c
+++ b/memory.c
@@ -311,6 +311,9 @@ static void memory_region_read_accessor(void *opaque,
     MemoryRegion *mr = opaque;
     uint64_t tmp;
 
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     tmp = mr->ops->read(mr->opaque, addr, size);
     *value |= (tmp & mask) << shift;
 }
@@ -325,6 +328,9 @@ static void memory_region_write_accessor(void *opaque,
     MemoryRegion *mr = opaque;
     uint64_t tmp;
 
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     tmp = (*value >> shift) & mask;
     mr->ops->write(mr->opaque, addr, tmp, size);
 }
@@ -826,6 +832,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->dirty_log_mask = 0;
     mr->ioeventfd_nb = 0;
     mr->ioeventfds = NULL;
+    mr->flush_coalesced_mmio = false;
 }
 
 static bool memory_region_access_valid(MemoryRegion *mr,
@@ -1176,12 +1183,15 @@ void memory_region_add_coalescing(MemoryRegion *mr,
     cmr->addr = addrrange_make(int128_make64(offset), int128_make64(size));
     QTAILQ_INSERT_TAIL(&mr->coalesced, cmr, link);
     memory_region_update_coalesced_range(mr);
+    memory_region_set_flush_coalesced(mr);
 }
 
 void memory_region_clear_coalescing(MemoryRegion *mr)
 {
     CoalescedMemoryRange *cmr;
 
+    memory_region_clear_flush_coalesced(mr);
+
     while (!QTAILQ_EMPTY(&mr->coalesced)) {
         cmr = QTAILQ_FIRST(&mr->coalesced);
         QTAILQ_REMOVE(&mr->coalesced, cmr, link);
@@ -1190,6 +1200,17 @@ void memory_region_clear_coalescing(MemoryRegion *mr)
     memory_region_update_coalesced_range(mr);
 }
 
+void memory_region_set_flush_coalesced(MemoryRegion *mr)
+{
+    mr->flush_coalesced_mmio = true;
+}
+
+void memory_region_clear_flush_coalesced(MemoryRegion *mr)
+{
+    qemu_flush_coalesced_mmio_buffer();
+    mr->flush_coalesced_mmio = false;
+}
+
 void memory_region_add_eventfd(MemoryRegion *mr,
                                target_phys_addr_t addr,
                                unsigned size,
diff --git a/memory.h b/memory.h
index 740c48e..9267423 100644
--- a/memory.h
+++ b/memory.h
@@ -133,6 +133,7 @@ struct MemoryRegion {
     bool enabled;
     bool rom_device;
     bool warning_printed; /* For reservations */
+    bool flush_coalesced_mmio;
     MemoryRegion *alias;
     target_phys_addr_t alias_offset;
     unsigned priority;
@@ -521,6 +522,29 @@ void memory_region_add_coalescing(MemoryRegion *mr,
 void memory_region_clear_coalescing(MemoryRegion *mr);
 
 /**
+ * memory_region_set_flush_coalesced: Enforce memory coalescing flush before
+ *                                    accesses.
+ *
+ * Ensure that pending coalesced MMIO request are flushed before the memory
+ * region is accessed. This property is automatically enabled for all regions
+ * passed to memory_region_set_coalescing() and memory_region_add_coalescing().
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_set_flush_coalesced(MemoryRegion *mr);
+
+/**
+ * memory_region_clear_flush_coalesced: Disable memory coalescing flush before
+ *                                      accesses.
+ *
+ * Clear the automatic coalesced MMIO flushing enabled via
+ * memory_region_set_flush_coalesced.
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_clear_flush_coalesced(MemoryRegion *mr);
+
+/**
  * memory_region_add_eventfd: Request an eventfd to be triggered when a word
  *                            is written to a location.
  *
-- 
1.7.3.4

[PATCH v2 3/7] memory: Use transaction_begin/commit also for single-step operations

[Jan Kiszka]

Wrap also simple operations consisting only of a single step with
memory_region_transaction_begin/commit. This allows to perform
additional steps like coalesced MMIO flushing from a single place.

This requires dropping some micro-optimizations: The skipping of
topology updates after updating disabled or unregistered regions.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/memory.c b/memory.c
index c29a8a3..8b1218e 100644
--- a/memory.c
+++ b/memory.c
@@ -1076,8 +1076,9 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;
 
+    memory_region_transaction_begin();
     mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
-    memory_region_update_topology(mr);
+    memory_region_transaction_commit();
 }
 
 bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr,
@@ -1110,16 +1111,18 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
 {
     if (mr->readonly != readonly) {
+        memory_region_transaction_begin();
         mr->readonly = readonly;
-        memory_region_update_topology(mr);
+        memory_region_transaction_commit();
     }
 }
 
 void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
 {
     if (mr->readable != readable) {
+        memory_region_transaction_begin();
         mr->readable = readable;
-        memory_region_update_topology(mr);
+        memory_region_transaction_commit();
     }
 }
 
@@ -1227,6 +1230,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
     };
     unsigned i;
 
+    memory_region_transaction_begin();
     for (i = 0; i < mr->ioeventfd_nb; ++i) {
         if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
             break;
@@ -1238,7 +1242,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
     memmove(&mr->ioeventfds[i+1], &mr->ioeventfds[i],
             sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i));
     mr->ioeventfds[i] = mrfd;
-    memory_region_update_topology(mr);
+    memory_region_transaction_commit();
 }
 
 void memory_region_del_eventfd(MemoryRegion *mr,
@@ -1257,6 +1261,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     };
     unsigned i;
 
+    memory_region_transaction_begin();
     for (i = 0; i < mr->ioeventfd_nb; ++i) {
         if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])) {
             break;
@@ -1268,7 +1273,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     --mr->ioeventfd_nb;
     mr->ioeventfds = g_realloc(mr->ioeventfds,
                                   sizeof(*mr->ioeventfds)*mr->ioeventfd_nb + 1);
-    memory_region_update_topology(mr);
+    memory_region_transaction_commit();
 }
 
 static void memory_region_add_subregion_common(MemoryRegion *mr,
@@ -1277,6 +1282,8 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
 {
     MemoryRegion *other;
 
+    memory_region_transaction_begin();
+
     assert(!subregion->parent);
     subregion->parent = mr;
     subregion->addr = offset;
@@ -1309,7 +1316,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
     }
     QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link);
 done:
-    memory_region_update_topology(mr);
+    memory_region_transaction_commit();
 }
 
 
@@ -1335,10 +1342,11 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
+    memory_region_transaction_begin();
     assert(subregion->parent == mr);
     subregion->parent = NULL;
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_update_topology(mr);
+    memory_region_transaction_commit();
 }
 
 void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
@@ -1346,8 +1354,9 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
     if (enabled == mr->enabled) {
         return;
     }
+    memory_region_transaction_begin();
     mr->enabled = enabled;
-    memory_region_update_topology(NULL);
+    memory_region_transaction_commit();
 }
 
 void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr)
@@ -1373,16 +1382,15 @@ void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr)
 
 void memory_region_set_alias_offset(MemoryRegion *mr, target_phys_addr_t offset)
 {
-    target_phys_addr_t old_offset = mr->alias_offset;
-
     assert(mr->alias);
-    mr->alias_offset = offset;
 
-    if (offset == old_offset || !mr->parent) {
+    if (offset == mr->alias_offset) {
         return;
     }
 
-    memory_region_update_topology(mr);
+    memory_region_transaction_begin();
+    mr->alias_offset = offset;
+    memory_region_transaction_commit();
 }
 
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
@@ -1514,14 +1522,16 @@ void memory_listener_unregister(MemoryListener *listener)
 
 void set_system_memory_map(MemoryRegion *mr)
 {
+    memory_region_transaction_begin();
     address_space_memory.root = mr;
-    memory_region_update_topology(NULL);
+    memory_region_transaction_commit();
 }
 
 void set_system_io_map(MemoryRegion *mr)
 {
+    memory_region_transaction_begin();
     address_space_io.root = mr;
-    memory_region_update_topology(NULL);
+    memory_region_transaction_commit();
 }
 
 uint64_t io_mem_read(MemoryRegion *mr, target_phys_addr_t addr, unsigned size)
-- 
1.7.3.4

[PATCH v2 4/7] memory: Fold memory_region_update_topology into memory_region_transaction_commit

[Jan Kiszka]

Simplify the code as we are using now only a subset of the original
features of memory_region_update_topology.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/memory.c b/memory.c
index 8b1218e..902a8c1 100644
--- a/memory.c
+++ b/memory.c
@@ -24,7 +24,6 @@
 #include "exec-obsolete.h"
 
 unsigned memory_region_transaction_depth = 0;
-static bool memory_region_update_pending = false;
 static bool global_dirty_log = false;
 
 static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners
@@ -732,31 +731,6 @@ static void address_space_update_topology(AddressSpace *as)
     address_space_update_ioeventfds(as);
 }
 
-static void memory_region_update_topology(MemoryRegion *mr)
-{
-    if (memory_region_transaction_depth) {
-        memory_region_update_pending |= !mr || mr->enabled;
-        return;
-    }
-
-    if (mr && !mr->enabled) {
-        return;
-    }
-
-    MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
-
-    if (address_space_memory.root) {
-        address_space_update_topology(&address_space_memory);
-    }
-    if (address_space_io.root) {
-        address_space_update_topology(&address_space_io);
-    }
-
-    MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
-
-    memory_region_update_pending = false;
-}
-
 void memory_region_transaction_begin(void)
 {
     ++memory_region_transaction_depth;
@@ -766,8 +740,17 @@ void memory_region_transaction_commit(void)
 {
     assert(memory_region_transaction_depth);
     --memory_region_transaction_depth;
-    if (!memory_region_transaction_depth && memory_region_update_pending) {
-        memory_region_update_topology(NULL);
+    if (!memory_region_transaction_depth) {
+        MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
+
+        if (address_space_memory.root) {
+            address_space_update_topology(&address_space_memory);
+        }
+        if (address_space_io.root) {
+            address_space_update_topology(&address_space_io);
+        }
+
+        MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
     }
 }
 
-- 
1.7.3.4

[PATCH v2 5/7] memory: Flush coalesced MMIO on mapping and state changes

[Jan Kiszka]

Flush pending coalesced MMIO before performing mapping or state changes
that could affect the event orderings or route the buffered requests to
a wrong region.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index 902a8c1..7cffddf 100644
--- a/memory.c
+++ b/memory.c
@@ -733,6 +733,7 @@ static void address_space_update_topology(AddressSpace *as)
 
 void memory_region_transaction_begin(void)
 {
+    qemu_flush_coalesced_mmio_buffer();
     ++memory_region_transaction_depth;
 }
 
-- 
1.7.3.4

[PATCH v2 6/7] VGA: Flush coalesced MMIO on related MMIO/PIO accesses

[Jan Kiszka]

In preparation of stopping to flush coalesced MMIO unconditionally on
vmexits, mark VGA MMIO and PIO regions as synchronous /wrt coalesced
MMIO and flush the buffer explicitly on PIO accesses that do not use
generic memory regions yet.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/cirrus_vga.c |    7 +++++++
 hw/qxl.c        |    1 +
 hw/vga-isa-mm.c |    1 +
 hw/vga.c        |    5 +++++
 hw/vmware_vga.c |    1 +
 5 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 623dd68..39636ca 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2441,6 +2441,8 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr)
     VGACommonState *s = &c->vga;
     int val, index;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (vga_ioport_invalid(s, addr)) {
 	val = 0xff;
     } else {
@@ -2534,6 +2536,8 @@ static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     VGACommonState *s = &c->vga;
     int index;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     /* check port range access depending on color/monochrome mode */
     if (vga_ioport_invalid(s, addr)) {
 	return;
@@ -2854,6 +2858,7 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
     /* I/O handler for LFB */
     memory_region_init_io(&s->cirrus_linear_io, &cirrus_linear_io_ops, s,
                           "cirrus-linear-io", VGA_RAM_SIZE);
+    memory_region_set_flush_coalesced(&s->cirrus_linear_io);
 
     /* I/O handler for LFB */
     memory_region_init_io(&s->cirrus_linear_bitblt_io,
@@ -2861,10 +2866,12 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
                           s,
                           "cirrus-bitblt-mmio",
                           0x400000);
+    memory_region_set_flush_coalesced(&s->cirrus_linear_bitblt_io);
 
     /* I/O handler for memory-mapped I/O */
     memory_region_init_io(&s->cirrus_mmio_io, &cirrus_mmio_io_ops, s,
                           "cirrus-mmio", CIRRUS_PNPMMIO_SIZE);
+    memory_region_set_flush_coalesced(&s->cirrus_mmio_io);
 
     s->real_vram_size =
         (s->device_id == CIRRUS_ID_CLGD5446) ? 4096 * 1024 : 2048 * 1024;
diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..05da2df 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1810,6 +1810,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     if (qxl->id == 0) {
         vga_dirty_log_start(&qxl->vga);
     }
+    memory_region_set_flush_coalesced(&qxl->io_bar);
 
 
     pci_register_bar(&qxl->pci, QXL_IO_RANGE_INDEX,
diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
index 44ae7d9..306e6ba 100644
--- a/hw/vga-isa-mm.c
+++ b/hw/vga-isa-mm.c
@@ -107,6 +107,7 @@ static void vga_mm_init(ISAVGAMMState *s, target_phys_addr_t vram_base,
     s_ioport_ctrl = g_malloc(sizeof(*s_ioport_ctrl));
     memory_region_init_io(s_ioport_ctrl, &vga_mm_ctrl_ops, s,
                           "vga-mm-ctrl", 0x100000);
+    memory_region_set_flush_coalesced(s_ioport_ctrl);
 
     vga_io_memory = g_malloc(sizeof(*vga_io_memory));
     /* XXX: endianness? */
diff --git a/hw/vga.c b/hw/vga.c
index acb3f7d..fd4ace3 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -357,6 +357,8 @@ uint32_t vga_ioport_read(void *opaque, uint32_t addr)
     VGACommonState *s = opaque;
     int val, index;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (vga_ioport_invalid(s, addr)) {
         val = 0xff;
     } else {
@@ -449,6 +451,8 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     VGACommonState *s = opaque;
     int index;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     /* check port range access depending on color/monochrome mode */
     if (vga_ioport_invalid(s, addr)) {
         return;
@@ -2327,6 +2331,7 @@ MemoryRegion *vga_init_io(VGACommonState *s,
     vga_mem = g_malloc(sizeof(*vga_mem));
     memory_region_init_io(vga_mem, &vga_mem_ops, s,
                           "vga-lowmem", 0x20000);
+    memory_region_set_flush_coalesced(vga_mem);
 
     return vga_mem;
 }
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 476dc89..7fd0371 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1182,6 +1182,7 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
 
     memory_region_init_io(&s->io_bar, &vmsvga_io_ops, &s->chip,
                           "vmsvga-io", 0x10);
+    memory_region_set_flush_coalesced(&s->io_bar);
     pci_register_bar(&s->card, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar);
 
     vmsvga_init(&s->chip, pci_address_space(dev),
-- 
1.7.3.4

[PATCH v2 7/7] kvm: Stop flushing coalesced MMIO on vmexit

[Jan Kiszka]

The memory subsystem will now take care of flushing whenever affected
regions are accessed or the memory mapping changes.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index f8e4328..a1d32f6 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1483,8 +1483,6 @@ int kvm_cpu_exec(CPUArchState *env)
         qemu_mutex_lock_iothread();
         kvm_arch_post_run(env, run);
 
-        kvm_flush_coalesced_mmio_buffer();
-
         if (run_ret < 0) {
             if (run_ret == -EINTR || run_ret == -EAGAIN) {
                 DPRINTF("io window exit\n");
-- 
1.7.3.4

Re: [PATCH v2 2/7] memory: Flush coalesced MMIO on selected region access

[Avi Kivity]

On 06/27/2012 07:27 PM, Jan Kiszka wrote:
> Instead of flushing pending coalesced MMIO requests on every vmexit,
> this provides a mechanism to selectively flush when memory regions
> related to the coalesced one are accessed. This first of all includes
> the coalesced region itself but can also applied to other regions, e.g.
> of the same device, by calling memory_region_set_flush_coalesced.
> 
> +
> +void memory_region_clear_flush_coalesced(MemoryRegion *mr)
> +{
> +    qemu_flush_coalesced_mmio_buffer();
> +    mr->flush_coalesced_mmio = false;
> +}


This will clear the implicit flag set by set_coalesced, but also any
explicit flag set by set_flush_coalesced.  In some convoluted setup that
will never happen, this can cause breakage.  This can be addresses
either using two flags (or perhaps using the coalesced list for the
implicit flag) or by a comment in clear_coalesced's documentation.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2 0/7] kvm: Get coalesced MMIO flushing out of the hot-path

[Avi Kivity]

On 06/27/2012 07:27 PM, Jan Kiszka wrote:
> Changes in v2:
>  - added memory_region_clear_flush_coalesced
>  - call memory_region_clear_flush_coalesced from
>    memory_region_clear_coalescing
>  - wrap all region manipulations via memory_region_transaction_begin/
>    commit internally
>  - flush coalesced MMIO only on memory_region_transaction_begin
> 
> Original description:
> 
> We currently flush the coalesced MMIO buffer on every vmexit to
> userspace. KVM only provides a single buffer per VM, so a central lock
> is required to read from it. This is a contention point given a large
> enough VCPU set. Moreover, we need to hold the BQL while replaying the
> queued requests, probably for a long time until there is more fine
> grained locking available. Good reasons to overcome the unconditional
> flush.
> 
> The series achieves this by flushing only on selected memory region
> accesses, either generically via the memory access dispatcher or
> directly on certain VGA PIO accesses that are not yet fully converted.
> Another reason to flush are remappings or other relevant region state
> changes.

Looks good; please see minor comment on patch 2.

-- 
error compiling committee.c: too many arguments to function

[PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Jan Kiszka]

Instead of flushing pending coalesced MMIO requests on every vmexit,
this provides a mechanism to selectively flush when memory regions
related to the coalesced one are accessed. This first of all includes
the coalesced region itself but can also applied to other regions, e.g.
of the same device, by calling memory_region_set_flush_coalesced.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v3:
 - refuse to clear flush_coalesced_mmio for regions that have
   coalescing enabled

 memory.c |   24 ++++++++++++++++++++++++
 memory.h |   26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index aab4a31..7221c3c 100644
--- a/memory.c
+++ b/memory.c
@@ -311,6 +311,9 @@ static void memory_region_read_accessor(void *opaque,
     MemoryRegion *mr = opaque;
     uint64_t tmp;
 
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     tmp = mr->ops->read(mr->opaque, addr, size);
     *value |= (tmp & mask) << shift;
 }
@@ -325,6 +328,9 @@ static void memory_region_write_accessor(void *opaque,
     MemoryRegion *mr = opaque;
     uint64_t tmp;
 
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     tmp = (*value >> shift) & mask;
     mr->ops->write(mr->opaque, addr, tmp, size);
 }
@@ -826,6 +832,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->dirty_log_mask = 0;
     mr->ioeventfd_nb = 0;
     mr->ioeventfds = NULL;
+    mr->flush_coalesced_mmio = false;
 }
 
 static bool memory_region_access_valid(MemoryRegion *mr,
@@ -1176,12 +1183,16 @@ void memory_region_add_coalescing(MemoryRegion *mr,
     cmr->addr = addrrange_make(int128_make64(offset), int128_make64(size));
     QTAILQ_INSERT_TAIL(&mr->coalesced, cmr, link);
     memory_region_update_coalesced_range(mr);
+    memory_region_set_flush_coalesced(mr);
 }
 
 void memory_region_clear_coalescing(MemoryRegion *mr)
 {
     CoalescedMemoryRange *cmr;
 
+    qemu_flush_coalesced_mmio_buffer();
+    mr->flush_coalesced_mmio = false;
+
     while (!QTAILQ_EMPTY(&mr->coalesced)) {
         cmr = QTAILQ_FIRST(&mr->coalesced);
         QTAILQ_REMOVE(&mr->coalesced, cmr, link);
@@ -1190,6 +1201,19 @@ void memory_region_clear_coalescing(MemoryRegion *mr)
     memory_region_update_coalesced_range(mr);
 }
 
+void memory_region_set_flush_coalesced(MemoryRegion *mr)
+{
+    mr->flush_coalesced_mmio = true;
+}
+
+void memory_region_clear_flush_coalesced(MemoryRegion *mr)
+{
+    qemu_flush_coalesced_mmio_buffer();
+    if (QTAILQ_EMPTY(&mr->coalesced)) {
+        mr->flush_coalesced_mmio = false;
+    }
+}
+
 void memory_region_add_eventfd(MemoryRegion *mr,
                                target_phys_addr_t addr,
                                unsigned size,
diff --git a/memory.h b/memory.h
index 740c48e..77167d8 100644
--- a/memory.h
+++ b/memory.h
@@ -133,6 +133,7 @@ struct MemoryRegion {
     bool enabled;
     bool rom_device;
     bool warning_printed; /* For reservations */
+    bool flush_coalesced_mmio;
     MemoryRegion *alias;
     target_phys_addr_t alias_offset;
     unsigned priority;
@@ -521,6 +522,31 @@ void memory_region_add_coalescing(MemoryRegion *mr,
 void memory_region_clear_coalescing(MemoryRegion *mr);
 
 /**
+ * memory_region_set_flush_coalesced: Enforce memory coalescing flush before
+ *                                    accesses.
+ *
+ * Ensure that pending coalesced MMIO request are flushed before the memory
+ * region is accessed. This property is automatically enabled for all regions
+ * passed to memory_region_set_coalescing() and memory_region_add_coalescing().
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_set_flush_coalesced(MemoryRegion *mr);
+
+/**
+ * memory_region_clear_flush_coalesced: Disable memory coalescing flush before
+ *                                      accesses.
+ *
+ * Clear the automatic coalesced MMIO flushing enabled via
+ * memory_region_set_flush_coalesced. Note that this service has no effect on
+ * memory regions that have MMIO coalescing enabled for themselves. For them,
+ * automatic flushing will stop once coalescing is disabled.
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_clear_flush_coalesced(MemoryRegion *mr);
+
+/**
  * memory_region_add_eventfd: Request an eventfd to be triggered when a word
  *                            is written to a location.
  *
-- 
1.7.3.4

Re: [PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Avi Kivity]

On 06/29/2012 07:37 PM, Jan Kiszka wrote:
> Instead of flushing pending coalesced MMIO requests on every vmexit,
> this provides a mechanism to selectively flush when memory regions
> related to the coalesced one are accessed. This first of all includes
> the coalesced region itself but can also applied to other regions, e.g.
> of the same device, by calling memory_region_set_flush_coalesced.

Looks fine.

I have a hard time deciding whether this should go through the kvm tree
or memory tree.  Anthony, perhaps you can commit it directly to avoid
the livelock?

Reviewed-by: Avi Kivity <avi@redhat.com>


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Avi Kivity]

On 07/02/2012 12:07 PM, Avi Kivity wrote:
> 
> Reviewed-by: Avi Kivity <avi@redhat.com>

(for the entire patchset)


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Jan Kiszka]

On 2012-07-02 11:07, Avi Kivity wrote:
> On 06/29/2012 07:37 PM, Jan Kiszka wrote:
>> Instead of flushing pending coalesced MMIO requests on every vmexit,
>> this provides a mechanism to selectively flush when memory regions
>> related to the coalesced one are accessed. This first of all includes
>> the coalesced region itself but can also applied to other regions, e.g.
>> of the same device, by calling memory_region_set_flush_coalesced.
> 
> Looks fine.
> 
> I have a hard time deciding whether this should go through the kvm tree
> or memory tree.  Anthony, perhaps you can commit it directly to avoid
> the livelock?
> 
> Reviewed-by: Avi Kivity <avi@redhat.com>
> 

Anthony, ping?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Jan Kiszka]

On 2012-07-10 12:41, Jan Kiszka wrote:
> On 2012-07-02 11:07, Avi Kivity wrote:
>> On 06/29/2012 07:37 PM, Jan Kiszka wrote:
>>> Instead of flushing pending coalesced MMIO requests on every vmexit,
>>> this provides a mechanism to selectively flush when memory regions
>>> related to the coalesced one are accessed. This first of all includes
>>> the coalesced region itself but can also applied to other regions, e.g.
>>> of the same device, by calling memory_region_set_flush_coalesced.
>>
>> Looks fine.
>>
>> I have a hard time deciding whether this should go through the kvm tree
>> or memory tree.  Anthony, perhaps you can commit it directly to avoid
>> the livelock?
>>
>> Reviewed-by: Avi Kivity <avi@redhat.com>
>>
> 
> Anthony, ping?

Argh, missed that this series was forgotten. Patch 1 is a bug fix, will
resend it separately so that it can make it into 1.2. Will repost the
rest once master reopens.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3 2/7] memory: Flush coalesced MMIO on selected region access

[Avi Kivity]

On 08/17/2012 01:55 PM, Jan Kiszka wrote:
> On 2012-07-10 12:41, Jan Kiszka wrote:
>> On 2012-07-02 11:07, Avi Kivity wrote:
>>> On 06/29/2012 07:37 PM, Jan Kiszka wrote:
>>>> Instead of flushing pending coalesced MMIO requests on every vmexit,
>>>> this provides a mechanism to selectively flush when memory regions
>>>> related to the coalesced one are accessed. This first of all includes
>>>> the coalesced region itself but can also applied to other regions, e.g.
>>>> of the same device, by calling memory_region_set_flush_coalesced.
>>>
>>> Looks fine.
>>>
>>> I have a hard time deciding whether this should go through the kvm tree
>>> or memory tree.  Anthony, perhaps you can commit it directly to avoid
>>> the livelock?
>>>
>>> Reviewed-by: Avi Kivity <avi@redhat.com>
>>>
>> 
>> Anthony, ping?
> 
> Argh, missed that this series was forgotten. Patch 1 is a bug fix, will
> resend it separately so that it can make it into 1.2. Will repost the
> rest once master reopens.

My fault, I should have just taken it into memory/core and sent a pull
request.  Sorry about that.

-- 
error compiling committee.c: too many arguments to function