Re: [Qemu-devel] [PATCH] Fix phys memory client - pass guest physical address not region offset

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2011-05-03 at 15:15 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@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.williamson@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