From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44249) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJmUj-00028j-7K for qemu-devel@nongnu.org; Thu, 04 Oct 2012 10:33:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJmUc-0000PO-UC for qemu-devel@nongnu.org; Thu, 04 Oct 2012 10:33:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJmUc-0000PJ-MA for qemu-devel@nongnu.org; Thu, 04 Oct 2012 10:33:38 -0400 Message-ID: <506D9E3C.2070603@redhat.com> Date: Thu, 04 Oct 2012 16:33:32 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1349280245-16341-1-git-send-email-avi@redhat.com> <1349280245-16341-15-git-send-email-avi@redhat.com> <87mx02ib3r.fsf@codemonkey.ws> In-Reply-To: <87mx02ib3r.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v1 14/22] memory: manage coalesced mmio via a MemoryListener List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Blue Swirl , Paolo Bonzini , "Michael S. Tsirkin" , qemu-devel@nongnu.org, liu ping fan On 10/04/2012 04:08 PM, Anthony Liguori wrote: > Avi Kivity writes: > >> Instead of calling a global function on coalesced mmio changes, which >> routes the call to kvm if enabled, add coalesced mmio hooks to >> MemoryListener and make kvm use that instead. >> >> The motivation is support for multiple address spaces (which means we >> we need to filter the call on the right address space) but the result >> is cleaner as well. >> >> -int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size) >> +static void kvm_coalesce_mmio_region(MemoryListener *listener, >> + MemoryRegionSection *secion, >> + target_phys_addr_t start, ram_addr_t size) >> { >> - int ret = -ENOSYS; >> KVMState *s = kvm_state; >> >> if (s->coalesced_mmio) { >> @@ -466,15 +467,14 @@ int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size) >> zone.size = size; >> zone.pad = 0; >> >> - ret = kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone); >> + (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone); > > g_assert on error instead of ignoring it. I'll do that in a separate patch, since the existing behaviour is to ignore the error. If errors really do happen, I don't want a refactoring patch to trigger them. >> >> diff --git a/memory.c b/memory.c >> index efefcb8..eb75349 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -1130,11 +1130,19 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa >> FlatRange *fr; >> CoalescedMemoryRange *cmr; >> AddrRange tmp; >> + MemoryRegionSection section; >> >> FOR_EACH_FLAT_RANGE(fr, as->current_map) { >> if (fr->mr == mr) { >> - qemu_unregister_coalesced_mmio(int128_get64(fr->addr.start), >> - int128_get64(fr->addr.size)); >> + section = (MemoryRegionSection) { >> + .address_space = as->root, >> + .offset_within_address_space = int128_get64(fr->addr.start), >> + .size = int128_get64(fr->addr.size), >> + }; > > I think this is a bit too clever. You can move the definition of > section into this block with a zero initializer, and then just set the > fields directly. You'll end up losing 2 lines of code in the process > too. I happen to like it (I dislike repeating the variable name each line) but have no problem changing it. -- error compiling committee.c: too many arguments to function