[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix phys memory client - pass guest physical ad
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH] Fix phys memory client - pass guest physical address not region offset |
Date: |
Tue, 03 May 2011 08:20:33 -0600 |
On Tue, 2011-05-03 at 15:15 +0200, Markus Armbruster wrote:
> Alex Williamson <address@hidden> 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 <address@hidden>
> > ---
> >
> > 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