| Message ID | 20131104060608.GA3322@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 04, 2013 at 08:06:08AM +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> BTW I think this is -stable material too. > --- > 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)); > address_space_init(&address_space_memory, system_memory, "memory"); > > system_io = g_malloc(sizeof(*system_io)); > -- > MST
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));
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 <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. 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)); > >
Il 04/11/2013 07:06, Michael S. Tsirkin ha scritto: > 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)); > address_space_init(&address_space_memory, system_memory, "memory"); > > system_io = g_malloc(sizeof(*system_io)); > You can include either this patch or Marcel's with my Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>. I don't have any preference. Paolo
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)); address_space_init(&address_space_memory, system_memory, "memory"); system_io = g_malloc(sizeof(*system_io));
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(-)