On 13/02/13 23:26, Michael S. Tsirkin wrote: > On Wed, Feb 13, 2013 at 06:14:33PM +1300, Alexey Korolev wrote: >> At the moment may_overlap flag of MemoryRegion structure >> is ignored by the address range assignment process. >> This may lead to guest OS hangs if critical qemu >> resources are overlapped by PCI BARs. For example >> ivshmem 64bit PCI BAR may overlap kvm-apic-msi under >> certain conditions. This patch adds a rule that the >> regions which should not be overlapped are added to the >> view first (i.e. having highest priority). The patch >> also corrects ivshmem bar resource to be overlapable >> which is the default for PCI BARs >> >> Signed-off-by: Alexey Korolev <alexey.koro...@endace.com> > Since overlap is currently used inconsistently, it's hard to > know what this will do. Maybe we should just drop the overlap > flag and use priorities instead? It sounds like a good idea. > >> --- >> hw/ivshmem.c | 2 +- >> memory.c | 15 ++++++++++----- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/hw/ivshmem.c b/hw/ivshmem.c >> index afaf9b3..1770fa3 100644 >> --- a/hw/ivshmem.c >> +++ b/hw/ivshmem.c >> @@ -341,7 +341,7 @@ static void create_shared_memory_BAR(IVShmemState *s, >> int fd) { >> memory_region_init_ram_ptr(&s->ivshmem, "ivshmem.bar2", >> s->ivshmem_size, ptr); >> vmstate_register_ram(&s->ivshmem, &s->dev.qdev); >> - memory_region_add_subregion(&s->bar, 0, &s->ivshmem); >> + memory_region_add_subregion_overlap(&s->bar, 0, &s->ivshmem, 1); >> >> /* region for shared memory */ >> pci_register_bar(&s->dev, 2, s->ivshmem_attr, &s->bar); > So why this change, exactly? memory_region_add_subregion adds a non-overlappable memory region, this is incorrect, becauase rest PCI bars by default are overlappable. I replaced memory_region_add_subregion with memory_region_add_subregion_overlap to make the region overlappable so it doesn't compete with kvm-apic-msi for address range.
In other words without this change ivshmem.bar2 will occupy address range of kvm-apic-msi because they have same priority and flags. > >> diff --git a/memory.c b/memory.c >> index cd7d5e0..f1119e7 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -475,7 +475,8 @@ static void render_memory_region(FlatView *view, >> MemoryRegion *mr, >> Int128 base, >> AddrRange clip, >> - bool readonly) >> + bool readonly, >> + bool overlap) >> { >> MemoryRegion *subregion; >> unsigned i; >> @@ -503,16 +504,16 @@ static void render_memory_region(FlatView *view, >> if (mr->alias) { >> int128_subfrom(&base, int128_make64(mr->alias->addr)); >> int128_subfrom(&base, int128_make64(mr->alias_offset)); >> - render_memory_region(view, mr->alias, base, clip, readonly); >> + render_memory_region(view, mr->alias, base, clip, readonly, >> overlap); >> return; >> } >> >> /* Render subregions in priority order. */ >> QTAILQ_FOREACH(subregion, &mr->subregions, subregions_link) { >> - render_memory_region(view, subregion, base, clip, readonly); >> + render_memory_region(view, subregion, base, clip, readonly, >> overlap); >> } >> >> - if (!mr->terminates) { >> + if (mr->may_overlap != overlap || !mr->terminates) { >> return; >> } >> >> @@ -567,7 +568,11 @@ static FlatView generate_memory_topology(MemoryRegion >> *mr) >> >> if (mr) { >> render_memory_region(&view, mr, int128_zero(), >> - addrrange_make(int128_zero(), int128_2_64()), >> false); >> + addrrange_make(int128_zero(), int128_2_64()), >> + false, false); >> + render_memory_region(&view, mr, int128_zero(), >> + addrrange_make(int128_zero(), int128_2_64()), >> + false, true); >> } >> flatview_simplify(&view); >> >> -- >> 1.7.9.5