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

[Michael S. Tsirkin]

On Mon, Nov 04, 2013 at 11:50:05AM +0200, Marcel Apfelbaum wrote:
> 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 <address@hidden>
> > ---
> >  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.

Well, exec isn't ready for this, it expects at most
TARGET_PHYS_ADDR_SPACE_BITS.
Fortunately there's no way for CPU to initiate io outside
this area.

So this is another place where device to device IO
would be broken.

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

I tested with (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS) - 1
as well, works fine.

> That leads me to another question?
> Maybe the fact that INT64_MAX is not aligned to page size makes
> all the trouble? 

It's not what's causing the trouble, it's merely making
the bug visible since subpage is the only path that actually checks that
sections do not overlap.

> 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?

To make sure sections don't overlap? Sure. Go for it.

> 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

I think we can't define this in a target independent way really.

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