From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes Date: Mon, 25 Jun 2012 12:15:11 +0200 Message-ID: <4FE83A2F.8010201@siemens.com> References: <47c076c8519f27cdfbf3f1e55432a162e5334e02.1340607659.git.jan.kiszka@siemens.com> <4FE827EA.6000603@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Anthony Liguori , Liu Ping Fan , qemu-devel , kvm , Marcelo Tosatti To: Avi Kivity Return-path: In-Reply-To: <4FE827EA.6000603@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org 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 >> >> 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