Re: [Qemu-devel] [PATCH] Fix guest OS hangs on boot when 64bit PCI BAR present

[Alexey Korolev]

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 <address@hidden>
> 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