From patchwork Wed Apr 24 10:22:08 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 239149 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 2B2422C0114 for ; Wed, 24 Apr 2013 20:22:45 +1000 (EST) Received: from localhost ([::1]:43457 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUwqY-0003g6-Qv for incoming@patchwork.ozlabs.org; Wed, 24 Apr 2013 06:22:42 -0400 Received: from eggs.gnu.org ([208.118.235.92]:43099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUwqH-0003fp-Nm for qemu-devel@nongnu.org; Wed, 24 Apr 2013 06:22:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUwqG-0000g5-31 for qemu-devel@nongnu.org; Wed, 24 Apr 2013 06:22:25 -0400 Received: from mail-we0-x231.google.com ([2a00:1450:400c:c03::231]:43094) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUwqF-0000f2-Ps for qemu-devel@nongnu.org; Wed, 24 Apr 2013 06:22:24 -0400 Received: by mail-we0-f177.google.com with SMTP id s47so716363wey.8 for ; Wed, 24 Apr 2013 03:22:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:from:to:cc:subject:date:message-id:x-mailer :in-reply-to; bh=aScsXBGpTpGg1EiN9r7NHUkakyjJmbU0LyqW+kqVwFA=; b=rZsgF4vTCGC0+d8c860z0/TR3kbga89m+bhzN9sLPqRpXXz7AGwmFRrNnV8m7o3Xy1 ZufYyjKCMbzFj+CzABK3hIH2osWxgX3iFW9Z3ef8/G8GRiV0DM66yhSuHkmrE+vKX+pF wY2HYNolsvfpjILi+dR+BcYQl1lYE9PVmXGmaCL0X8tjG/UF1EN4AFhvCRwZ5EvWfajB ES6t5VDYjte3L3vjhZmG9RMMAC2ha5mhsUuVOp14BdBtx2bGX9oRf700SqiFC+by/r1/ QBQW8zvoUD2n/iLDTT2scbhz5ILfq/m8PAlRq3V80SiTYQVroHItJFKNVjp3QJvSSvKX IOgQ== X-Received: by 10.194.88.138 with SMTP id bg10mr67651478wjb.13.1366798942997; Wed, 24 Apr 2013 03:22:22 -0700 (PDT) Received: from yakj.lan (93-34-176-20.ip50.fastwebnet.it. [93.34.176.20]) by mx.google.com with ESMTPSA id s6sm16547188wij.4.2013.04.24.03.22.19 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 24 Apr 2013 03:22:21 -0700 (PDT) From: Paolo Bonzini To: peter.maydell@linaro.org Date: Wed, 24 Apr 2013 12:22:08 +0200 Message-Id: <1366798928-14418-1-git-send-email-pbonzini@redhat.com> X-Mailer: git-send-email 1.8.2 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c03::231 Cc: "kwolf@redhat.com" , "aliguori@us.ibm.com" , "ehabkost@redhat.com" , "gleb@redhat.com" , "mst@redhat.com" , Jan Kiszka , "quintela@redhat.com" , claudio.fontana@huawei.com, "qemu-devel@nongnu.org" , "aderumier@odiso.com" , "lcapitulino@redhat.com" , "blauwirbel@gmail.com" , "yang.z.zhang@intel.com" , "alex.williamson@redhat.com" , "kraxel@redhat.com" , "anthony.perard@citrix.com" , "armbru@redhat.com" , "stefano.stabellini@eu.citrix.com" , "rth@twiddle.net" Subject: Re: [Qemu-devel] [PATCH 17/21] introduce memory_region_get_address() and use it in kvm/ioapic X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org > So how about: > MemoryRegionSection memory_region_find(MemoryRegion *address_space, > hwaddr addr, uint64_t size); > becomes > MemoryRegionSection address_space_find_region_by_addr( > AddressSpace *address_space, > hwaddr addr, uint64_t size); > (bit of a mouthful, but never mind) > > void memory_global_sync_dirty_bitmap(MemoryRegion *address_space); > becomes > void address_space_sync_dirty_bitmap(AddressSpace *address_space); I think the latter makes definite sense, I am not quite as sure about the former. Looking at framebuffer.c's use of memory_region_find, here you really want to go from the "local" view to a global one in order to look at global data structures such as the dirty bitmap. So it is right to have memory_region_find as a MemoryRegion operation, even though right now it is always passed an AddressSpace. Communicating the absolute address to KVM is another example of this local->global translation. Hence, my suggestion is to remove memory_region_find's limitation on the first argument, and make it work on nested regions too. With this change, memory_region_find() nicely fits Igor's use case. Not coincidentially, the additional code in memory_region_find() is very similar to Igor's memory_region_get_address(). See the attached patch, which I tested on master. Igor, can you try it with iccbus? Paolo -------------- 8< -------------------- From 953460fa8ee9f9e7243ea34eb57a901102be9307 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 23 Apr 2013 10:29:51 +0200 Subject: [RFC PATCH 17/21] extend memory_region_find() and use it in kvm/ioapic kvm/ioapic is relying on the fact that SysBus device maps mmio regions with offset counted from start of system memory. But if ioapic's region is moved to another sub-region which doesn't start at the beginning of system memory then using offset isn't correct. To fix kvm/ioapic, extend memory_region_find() so that it can help retrieving the absolute region address and the respective address space. The patch is a no-op in case mr is parentless, i.e. mr->addr == 0 and mr->parent == NULL. Based on a patch by Igor Mammedov. Signed-off-by: Paolo Bonzini --- hw/i386/kvm/ioapic.c | 9 ++++++++- include/exec/memory.h | 13 +++++++------ memory.c | 19 ++++++++++++++----- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c index a3bd519..7564d07 100644 --- a/hw/i386/kvm/ioapic.c +++ b/hw/i386/kvm/ioapic.c @@ -89,14 +89,21 @@ static void kvm_ioapic_put(IOAPICCommonState *s) { struct kvm_irqchip chip; struct kvm_ioapic_state *kioapic; + MemoryRegionSection mrs; int ret, i; + mrs = memory_region_find(&s->io_memory, 0, 0x1000); + if (mrs.mr != &s->io_memory || mrs.offset_within_region != 0) { + fprintf(stderr, "cannot find IOAPIC base\n"); + abort(); + } + chip.chip_id = KVM_IRQCHIP_IOAPIC; kioapic = &chip.chip.ioapic; kioapic->id = s->id; kioapic->ioregsel = s->ioregsel; - kioapic->base_address = s->busdev.mmio[0].addr; + kioapic->base_address = mrs.offset_within_address_space; kioapic->irr = s->irr; for (i = 0; i < IOAPIC_NUM_PINS; i++) { kioapic->redirtbl[i].bits = s->ioredtbl[i]; diff --git a/include/exec/memory.h b/include/exec/memory.h index eb9e659..5854d19 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -725,17 +725,18 @@ void memory_region_set_alias_offset(MemoryRegion *mr, * * Returns a #MemoryRegionSection that describes a contiguous overlap. * It will have the following characteristics: - * .@offset_within_address_space >= @addr - * .@offset_within_address_space + .@size <= @addr + @size * .@size = 0 iff no overlap was found * .@mr is non-%NULL iff an overlap was found * - * @address_space: a top-level (i.e. parentless) region that contains - * the region to be found - * @addr: start of the area within @address_space to be searched + * If @mr is parent-less, + * .@offset_within_address_space >= @addr + * .@offset_within_address_space + .@size <= @addr + @size + * + * @mr: a (possibly indirect) parent that contains the region to be found + * @addr: start of the area within @as to be searched * @size: size of the area to be searched */ -MemoryRegionSection memory_region_find(MemoryRegion *address_space, +MemoryRegionSection memory_region_find(MemoryRegion *mr, hwaddr addr, uint64_t size); /** diff --git a/memory.c b/memory.c index c82bd12..dba0a4b 100644 --- a/memory.c +++ b/memory.c @@ -1451,15 +1451,24 @@ static FlatRange *address_space_lookup(AddressSpace *as, AddrRange addr) sizeof(FlatRange), cmp_flatrange_addr); } -MemoryRegionSection memory_region_find(MemoryRegion *address_space, +MemoryRegionSection memory_region_find(MemoryRegion *mr, hwaddr addr, uint64_t size) { - AddressSpace *as = memory_region_to_address_space(address_space); - AddrRange range = addrrange_make(int128_make64(addr), - int128_make64(size)); - FlatRange *fr = address_space_lookup(as, range); MemoryRegionSection ret = { .mr = NULL, .size = 0 }; + MemoryRegion *root; + AddressSpace *as; + AddrRange range; + FlatRange *fr; + + addr += mr->addr; + for (root = mr; root->parent; ) { + root = root->parent; + addr += root->addr; + } + as = memory_region_to_address_space(root); + range = addrrange_make(int128_make64(addr), int128_make64(size)); + fr = address_space_lookup(as, range); if (!fr) { return ret; }