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

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

[Jan Kiszka]

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.

Please review carefully.

CC: Andreas Färber <andreas.faerber@web.de>
CC: Hervé Poussineau <hpoussin@reactos.org>

Jan Kiszka (5):
  i82378: Remove bogus MMIO coalescing
  memory: Flush coalesced MMIO on selected region access
  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        |   36 ++++++++++++++++++++++++++++++++++++
 memory.h        |   13 +++++++++++++
 9 files changed, 64 insertions(+), 3 deletions(-)

-- 
1.7.3.4

[PATCH 1/5] 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.

CC: Andreas Färber <andreas.faerber@web.de>
CC: 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 2/5] 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 |   13 +++++++++++++
 memory.h |   13 +++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index aab4a31..ba55b3e 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,6 +1183,7 @@ 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)
@@ -1190,6 +1198,11 @@ 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_add_eventfd(MemoryRegion *mr,
                                target_phys_addr_t addr,
                                unsigned size,
diff --git a/memory.h b/memory.h
index 740c48e..dca7a86 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,18 @@ 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_add_eventfd: Request an eventfd to be triggered when a word
  *                            is written to a location.
  *
-- 
1.7.3.4

[PATCH 3/5] 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>

In addition, we also have to
---
 memory.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index ba55b3e..141f92b 100644
--- a/memory.c
+++ b/memory.c
@@ -759,6 +759,7 @@ static void memory_region_update_topology(MemoryRegion *mr)
 
 void memory_region_transaction_begin(void)
 {
+    qemu_flush_coalesced_mmio_buffer();
     ++memory_region_transaction_depth;
 }
 
@@ -1109,6 +1110,9 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 
 void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
 {
+    if (!QTAILQ_EMPTY(&mr->coalesced)) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     if (mr->readonly != readonly) {
         mr->readonly = readonly;
         memory_region_update_topology(mr);
@@ -1117,6 +1121,9 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
 
 void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
 {
+    if (!QTAILQ_EMPTY(&mr->coalesced)) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     if (mr->readable != readable) {
         mr->readable = readable;
         memory_region_update_topology(mr);
@@ -1190,6 +1197,8 @@ void memory_region_clear_coalescing(MemoryRegion *mr)
 {
     CoalescedMemoryRange *cmr;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     while (!QTAILQ_EMPTY(&mr->coalesced)) {
         cmr = QTAILQ_FIRST(&mr->coalesced);
         QTAILQ_REMOVE(&mr->coalesced, cmr, link);
@@ -1219,6 +1228,9 @@ void memory_region_add_eventfd(MemoryRegion *mr,
     };
     unsigned i;
 
+    if (!QTAILQ_EMPTY(&mr->coalesced)) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     for (i = 0; i < mr->ioeventfd_nb; ++i) {
         if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
             break;
@@ -1249,6 +1261,9 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     };
     unsigned i;
 
+    if (!QTAILQ_EMPTY(&mr->coalesced)) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
     for (i = 0; i < mr->ioeventfd_nb; ++i) {
         if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])) {
             break;
@@ -1269,6 +1284,8 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
 {
     MemoryRegion *other;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     assert(!subregion->parent);
     subregion->parent = mr;
     subregion->addr = offset;
@@ -1327,6 +1344,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
+    qemu_flush_coalesced_mmio_buffer();
+
     assert(subregion->parent == mr);
     subregion->parent = NULL;
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
@@ -1335,6 +1354,8 @@ void memory_region_del_subregion(MemoryRegion *mr,
 
 void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
 {
+    qemu_flush_coalesced_mmio_buffer();
+
     if (enabled == mr->enabled) {
         return;
     }
@@ -1367,6 +1388,8 @@ void memory_region_set_alias_offset(MemoryRegion *mr, target_phys_addr_t offset)
 {
     target_phys_addr_t old_offset = mr->alias_offset;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     assert(mr->alias);
     mr->alias_offset = offset;
 
-- 
1.7.3.4

[PATCH 4/5] 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 afedaa4..cf551a3 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2439,6 +2439,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 {
@@ -2532,6 +2534,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;
@@ -2852,6 +2856,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,
@@ -2859,10 +2864,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 3da3399..ef21176 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1778,6 +1778,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 f8984c6..aab78de 100644
--- a/hw/vga-isa-mm.c
+++ b/hw/vga-isa-mm.c
@@ -105,6 +105,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 d784df7..2a6f040 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;
@@ -2320,6 +2324,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 142d9f4..cf2638d 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, VGA_RAM_SIZE, pci_address_space(dev),
-- 
1.7.3.4

[PATCH 5/5] 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 1/5] i82378: Remove bogus MMIO coalescing

[Hervé Poussineau]

Jan Kiszka a écrit :
> 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.
> 
> CC: Andreas Färber <andreas.faerber@web.de>
> CC: 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(-)

Acked-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [PATCH 1/5] i82378: Remove bogus MMIO coalescing

[Andreas Färber]

Am 25.06.2012 09:11, schrieb Hervé Poussineau:
> Jan Kiszka a écrit :
>> 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.
>>
>> CC: Andreas Färber <andreas.faerber@web.de>
>> CC: 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(-)
> 
> Acked-by: Hervé Poussineau <hpoussin@reactos.org>

Fine with me then.

Andreas

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

[Avi Kivity]

On 06/25/2012 10:00 AM, 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.

> @@ -521,6 +522,18 @@ 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);
> +
> +/**


Please provide a way to clear the flag (and autoclear on clear_coalesced).


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

Re: [Qemu-devel] [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Andreas Färber]

Am 25.06.2012 09:01, schrieb 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>

> 
> In addition, we also have to

Stray paste or missing addition to the above?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Avi Kivity]

On 06/25/2012 10:01 AM, Jan Kiszka wrote:
> 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>
> 
> In addition, we also have to

Yes, we do.

>  
>  void memory_region_transaction_begin(void)
>  {
> +    qemu_flush_coalesced_mmio_buffer();
>      ++memory_region_transaction_depth;
>  }

Why is this needed?

>  
> @@ -1109,6 +1110,9 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>  
>  void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>  {
> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
> +        qemu_flush_coalesced_mmio_buffer();
> +    }

The readonly attribute is inherited by subregions and alias targets, so
this check is insufficient.  See render_memory_region().  Need to flush
unconditionally.

>      if (mr->readonly != readonly) {
>          mr->readonly = readonly;
>          memory_region_update_topology(mr);
> @@ -1117,6 +1121,9 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>  
>  void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
>  {
> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
> +        qemu_flush_coalesced_mmio_buffer();
> +    }

This property is not inherited, but let's flush unconditionally just the
same, to reduce fragility.

> @@ -1219,6 +1228,9 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>      };
>      unsigned i;
>  
> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
> +        qemu_flush_coalesced_mmio_buffer();
> +    }

Ditto.  It's possible that an eventfd overlays a subregion which has
coalescing enabled.  It's not really defined what happens in this case,
and such code and its submitter should be perma-nacked, but let's play
it safe here since there isn't much to be gained by avoiding the flush.
 This code is a very slow path anyway, including and rcu and/or srcu
synchronization, and a rebuild of the dispatch radix tree (trees when we
dma-enable this code).

>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
>          if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
>              break;
> @@ -1249,6 +1261,9 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>      };
>      unsigned i;
>  
> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
> +        qemu_flush_coalesced_mmio_buffer();
> +    }

Same.

The repetitiveness of this code suggests a different way of doing this:
make every API call be its own subtransaction and perform the flush in
memory_region_begin_transaction() (maybe that's the answer to my
question above).

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

Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Jan Kiszka]

On 2012-06-25 10:57, Avi Kivity wrote:
> On 06/25/2012 10:01 AM, Jan Kiszka wrote:
>> 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>
>>
>> In addition, we also have to
> 
> Yes, we do.
> 
>>  
>>  void memory_region_transaction_begin(void)
>>  {
>> +    qemu_flush_coalesced_mmio_buffer();
>>      ++memory_region_transaction_depth;
>>  }
> 
> Why is this needed?
> 
>>  
>> @@ -1109,6 +1110,9 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>>  
>>  void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>>  {
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> The readonly attribute is inherited by subregions and alias targets, so
> this check is insufficient.  See render_memory_region().  Need to flush
> unconditionally.
> 
>>      if (mr->readonly != readonly) {
>>          mr->readonly = readonly;
>>          memory_region_update_topology(mr);
>> @@ -1117,6 +1121,9 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>>  
>>  void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
>>  {
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> This property is not inherited, but let's flush unconditionally just the
> same, to reduce fragility.
> 
>> @@ -1219,6 +1228,9 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>>      };
>>      unsigned i;
>>  
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> Ditto.  It's possible that an eventfd overlays a subregion which has
> coalescing enabled.  It's not really defined what happens in this case,
> and such code and its submitter should be perma-nacked, but let's play
> it safe here since there isn't much to be gained by avoiding the flush.
>  This code is a very slow path anyway, including and rcu and/or srcu
> synchronization, and a rebuild of the dispatch radix tree (trees when we
> dma-enable this code).
> 
>>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
>>          if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
>>              break;
>> @@ -1249,6 +1261,9 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>>      };
>>      unsigned i;
>>  
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> Same.

OK.

> 
> The repetitiveness of this code suggests a different way of doing this:
> make every API call be its own subtransaction and perform the flush in
> memory_region_begin_transaction() (maybe that's the answer to my
> question above).

So you want me to wrap the core of those services in
begin/commit_transaction instead? Just to be sure I got the idea.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Jan Kiszka]

On 2012-06-25 12:15, Jan Kiszka wrote:
> On 2012-06-25 10:57, Avi Kivity wrote:
>> The repetitiveness of this code suggests a different way of doing this:
>> make every API call be its own subtransaction and perform the flush in
>> memory_region_begin_transaction() (maybe that's the answer to my
>> question above).
> 
> So you want me to wrap the core of those services in
> begin/commit_transaction instead? Just to be sure I got the idea.

What we would lose this way (transaction_commit) is the ability to skip
updates on !mr->enabled.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Avi Kivity]

On 06/25/2012 01:26 PM, Jan Kiszka wrote:
> On 2012-06-25 12:15, Jan Kiszka wrote:
>> On 2012-06-25 10:57, Avi Kivity wrote:
>>> The repetitiveness of this code suggests a different way of doing this:
>>> make every API call be its own subtransaction and perform the flush in
>>> memory_region_begin_transaction() (maybe that's the answer to my
>>> question above).
>> 
>> So you want me to wrap the core of those services in
>> begin/commit_transaction instead? Just to be sure I got the idea.
> 
> What we would lose this way (transaction_commit) is the ability to skip
> updates on !mr->enabled.

We could have an internal memory_region_begin_transaction_mr() which
checks mr->enabled and notes it if its clear (but anything else would
have to undo this).  I don't think it's worth it, let's lose the
optimization and see if it shows up anywhere.

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

Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Jan Kiszka]

On 2012-06-25 13:01, Avi Kivity wrote:
> On 06/25/2012 01:26 PM, Jan Kiszka wrote:
>> On 2012-06-25 12:15, Jan Kiszka wrote:
>>> On 2012-06-25 10:57, Avi Kivity wrote:
>>>> The repetitiveness of this code suggests a different way of doing this:
>>>> make every API call be its own subtransaction and perform the flush in
>>>> memory_region_begin_transaction() (maybe that's the answer to my
>>>> question above).
>>>
>>> So you want me to wrap the core of those services in
>>> begin/commit_transaction instead? Just to be sure I got the idea.
>>
>> What we would lose this way (transaction_commit) is the ability to skip
>> updates on !mr->enabled.
> 
> We could have an internal memory_region_begin_transaction_mr() which
> checks mr->enabled and notes it if its clear (but anything else would
> have to undo this).  I don't think it's worth it, let's lose the
> optimization and see if it shows up anywhere.

OK, will send a new series with a conversion of this included.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux