| Message ID | 511B2139.2050101@endace.com |
|---|---|
| State | New |
| Headers | show |
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.korolev@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? > --- > 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? > 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
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.korolev@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
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); 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);
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.korolev@endace.com> --- hw/ivshmem.c | 2 +- memory.c | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-)