qemu-devel
[Top][All Lists]
Advanced

[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: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] Fix phys memory client - pass guest physical address not region offset
Date: Tue, 03 May 2011 15:15:13 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux)

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.

> @@ -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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]