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.kis...@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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html