Re: [Qemu-devel] [PATCH] exec: limit system memory size

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2013-11-04 at 08:06 +0200, Michael S. Tsirkin wrote:
> The page table logic in exec.c assumes
> that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> 
> But pci addresses are full 64 bit so if we try to render them ignoring
> the extra bits, we get strange effects with sections overlapping each
> other.
> 
> To fix, simply limit the system memory size to
>  1 << TARGET_PHYS_ADDR_SPACE_BITS,
> pci addresses will be rendered within that.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  exec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 030118e..c7a8df5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1801,7 +1801,12 @@ void address_space_destroy_dispatch(AddressSpace *as)
>  static void memory_map_init(void)
>  {
>      system_memory = g_malloc(sizeof(*system_memory));
> -    memory_region_init(system_memory, NULL, "system", INT64_MAX);
> +
> +    assert(TARGET_PHYS_ADDR_SPACE_BITS <= 64);
> +
> +    memory_region_init(system_memory, NULL, "system",
> +                       TARGET_PHYS_ADDR_SPACE_BITS == 64 ?
> +                       UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS));

Michael, thanks again for the help.

I am concerned that we cannot use all the UINT64_MAX
address space.

By the way, this patch makes the memory size aligned to page size,
so the call to register_subpage (the asserted code) is diverted
to register_multipage that does not have an assert.
That leads me to another question?
Maybe the fact that INT64_MAX is not aligned to page size makes
all the trouble? 

What do you think?

Regarding this patch:
Maybe we should to add an assert inside memory_region_init 
in order to protect all the code that creates memory regions?

And also maybe we should add a define MAX_MEMORY_REGION_SIZE
to be used in all places we want a "maximum size" memory region?

Thanks,
Marcel

>      address_space_init(&address_space_memory, system_memory, "memory");
>  
>      system_io = g_malloc(sizeof(*system_io));