Patchwork Fix guest OS hangs on boot when 64bit PCI BAR present

login
register
mail settings
Submitter Alexey Korolev
Date Feb. 13, 2013, 5:14 a.m.
Message ID <511B2139.2050101@endace.com>
Download mbox | patch
Permalink /patch/220042/
State New
Headers show

Comments

Alexey Korolev - Feb. 13, 2013, 5:14 a.m.
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(-)
Michael S. Tsirkin - Feb. 13, 2013, 10:26 a.m.
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
Alexey Korolev - Feb. 14, 2013, 12:55 a.m.
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

Patch

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);