From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state
 changes
Date: Mon, 25 Jun 2012 11:57:14 +0300
Message-ID: <4FE827EA.6000603@redhat.com>
References: <cover.1340607659.git.jan.kiszka@siemens.com> <47c076c8519f27cdfbf3f1e55432a162e5334e02.1340607659.git.jan.kiszka@siemens.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Cc: Anthony Liguori <aliguori@us.ibm.com>,
	qemu-devel <qemu-devel@nongnu.org>, kvm <kvm@vger.kernel.org>,
	Liu Ping Fan <pingfank@linux.vnet.ibm.com>,
	Marcelo Tosatti <mtosatti@redhat.com>
To: Jan Kiszka <jan.kiszka@siemens.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:14459 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751719Ab2FYI51 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 25 Jun 2012 04:57:27 -0400
In-Reply-To: <47c076c8519f27cdfbf3f1e55432a162e5334e02.1340607659.git.jan.kiszka@siemens.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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