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@xxxxxxxxxxx>
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
