> 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 <pbonz...@redhat.com>
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. <imamm...@redhat.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
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;
}
--
1.7.1
