On Tue, 2011-05-03 at 15:15 +0200, Markus Armbruster wrote: > Alex Williamson <alex.william...@redhat.com> writes: > > > When we're trying to get a newly registered phys memory client updated > > with the current page mappings, we end up passing the region offset > > (a ram_addr_t) as the start address rather than the actual guest > > physical memory address (target_phys_addr_t). If your guest has less > > than 3.5G of memory, these are coincidentally the same thing. If > > there's more, the region offset for the memory above 4G starts over > > at 0, so the set_memory client will overwrite it's lower memory entries. > > > > Instead, keep track of the guest phsyical address as we're walking the > > tables and pass that to the set_memory client. > > > > Signed-off-by: Alex Williamson <alex.william...@redhat.com> > > --- > > > > exec.c | 10 ++++++---- > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 4752af1..e670929 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -1742,7 +1742,7 @@ static int cpu_notify_migration_log(int enable) > > } > > > > static void phys_page_for_each_1(CPUPhysMemoryClient *client, > > - int level, void **lp) > > + int level, void **lp, target_phys_addr_t > > addr) > > { > > int i; > > > > Aren't you abusing target_phys_addr_t here? It's not a physical > address, it needs to be shifted left to become one. By how much depends > on level. Please take pity on future maintainers and spell this out in > a comment. > > Perhaps you can code it in a way that makes the parameter an address. > Probably no need for a comment then.
Right, it's not a target_phys_addr_t on passing to the function, but it becomes one as we work, so it still seemed the appropriate data type. I rather like how the shifting works into the recursive-ness of the function, I think it removes a bit of ugliness for figuring how many levels are there, where am I, how many multiples of *_BITS do I shift. I'll add a comment and hope that helps. > > @@ -1751,16 +1751,18 @@ static void > > phys_page_for_each_1(CPUPhysMemoryClient *client, > > } > > if (level == 0) { > > PhysPageDesc *pd = *lp; > > + addr <<= L2_BITS + TARGET_PAGE_BITS; > > for (i = 0; i < L2_SIZE; ++i) { > > if (pd[i].phys_offset != IO_MEM_UNASSIGNED) { > > - client->set_memory(client, pd[i].region_offset, > > + client->set_memory(client, addr | i << TARGET_PAGE_BITS, > > TARGET_PAGE_SIZE, pd[i].phys_offset); > > } > > } > > } else { > > void **pp = *lp; > > for (i = 0; i < L2_SIZE; ++i) { > > - phys_page_for_each_1(client, level - 1, pp + i); > > + phys_page_for_each_1(client, level - 1, pp + i, > > + (addr << L2_BITS) | i); > > } > > } > > } > > @@ -1770,7 +1772,7 @@ static void phys_page_for_each(CPUPhysMemoryClient > > *client) > > int i; > > for (i = 0; i < P_L1_SIZE; ++i) { > > phys_page_for_each_1(client, P_L1_SHIFT / L2_BITS - 1, > > - l1_phys_map + i); > > + l1_phys_map + i, i); > > } > > } > > > > Fix makes sense to me, after some head scratching. A comment explaining > the phys map data structure would be helpful. l1_phys_map[] has a > comment, but it's devoid of detail. I'll see what I can do, though I'm pretty sure I'm not at the top of the list for describing the existence and format of these tables. Thanks, Alex