diff mbox series

[v1,5/6] kvm: kvm_log_start/stop are only called with known sections

Message ID 20170911174933.20789-6-david@redhat.com
State New
Headers show
Series QEMU: kvm: cleanup kvm_slot handling | expand

Commit Message

David Hildenbrand Sept. 11, 2017, 5:49 p.m. UTC 
Let's properly align the sections first and bail out if we would ever
get called with a memory section we don't know yet.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Thomas Huth Oct. 10, 2017, 9:06 a.m. UTC | #1 
On 11.09.2017 19:49, David Hildenbrand wrote:
> Let's properly align the sections first and bail out if we would ever
> get called with a memory section we don't know yet.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b677d1b13e..2ae459453d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>                                      MemoryRegionSection *section)
>  {
> -    hwaddr phys_addr = section->offset_within_address_space;
> -    ram_addr_t size = int128_get64(section->size);
> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
> +    hwaddr start_addr, size;
> +    KVMSlot *mem;
>  
> -    if (mem == NULL)  {
> +    size = kvm_align_section(section, &start_addr);
> +    if (!size) {
>          return 0;
> -    } else {
> -        return kvm_slot_update_flags(kml, mem, section->mr);
>      }
> +
> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
> +    if (!mem) {
> +        fprintf(stderr, "%s: error finding slot\n", __func__);
> +        abort();
> +    }

FYI, this abort now triggers with the "isa-vga" device:

$ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
kvm_section_update_flags: error finding slot
Aborted (core dumped)

Any ideas how to fix this?

 Thomas
Paolo Bonzini Oct. 10, 2017, 1:29 p.m. UTC | #2 
On 10/10/2017 11:06, Thomas Huth wrote:
> On 11.09.2017 19:49, David Hildenbrand wrote:
>> Let's properly align the sections first and bail out if we would ever
>> get called with a memory section we don't know yet.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index b677d1b13e..2ae459453d 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>                                      MemoryRegionSection *section)
>>  {
>> -    hwaddr phys_addr = section->offset_within_address_space;
>> -    ram_addr_t size = int128_get64(section->size);
>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>> +    hwaddr start_addr, size;
>> +    KVMSlot *mem;
>>  
>> -    if (mem == NULL)  {
>> +    size = kvm_align_section(section, &start_addr);
>> +    if (!size) {
>>          return 0;
>> -    } else {
>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>      }
>> +
>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>> +    if (!mem) {
>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>> +        abort();
>> +    }
> 
> FYI, this abort now triggers with the "isa-vga" device:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
> kvm_section_update_flags: error finding slot
> Aborted (core dumped)
> 
> Any ideas how to fix this?

Reverting, unless David has some time to look into it.

Paolo
David Hildenbrand Oct. 16, 2017, 7:16 a.m. UTC | #3 
On 10.10.2017 15:29, Paolo Bonzini wrote:
> On 10/10/2017 11:06, Thomas Huth wrote:
>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>> Let's properly align the sections first and bail out if we would ever
>>> get called with a memory section we don't know yet.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index b677d1b13e..2ae459453d 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>                                      MemoryRegionSection *section)
>>>  {
>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>> -    ram_addr_t size = int128_get64(section->size);
>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>> +    hwaddr start_addr, size;
>>> +    KVMSlot *mem;
>>>  
>>> -    if (mem == NULL)  {
>>> +    size = kvm_align_section(section, &start_addr);
>>> +    if (!size) {
>>>          return 0;
>>> -    } else {
>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>      }
>>> +
>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>> +    if (!mem) {
>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>> +        abort();
>>> +    }
>>
>> FYI, this abort now triggers with the "isa-vga" device:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>> kvm_section_update_flags: error finding slot
>> Aborted (core dumped)
>>
>> Any ideas how to fix this?
> 
> Reverting, unless David has some time to look into it.
> 
> Paolo
> 

Just returned from vacation, I'll have a look this week.

Thanks for that nice reproducer Thomas, that should help a lot!
Thomas Huth Oct. 16, 2017, 8:52 a.m. UTC | #4 
On 16.10.2017 09:16, David Hildenbrand wrote:
> On 10.10.2017 15:29, Paolo Bonzini wrote:
>> On 10/10/2017 11:06, Thomas Huth wrote:
>>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>>> Let's properly align the sections first and bail out if we would ever
>>>> get called with a memory section we don't know yet.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index b677d1b13e..2ae459453d 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>>                                      MemoryRegionSection *section)
>>>>  {
>>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>>> -    ram_addr_t size = int128_get64(section->size);
>>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>>> +    hwaddr start_addr, size;
>>>> +    KVMSlot *mem;
>>>>  
>>>> -    if (mem == NULL)  {
>>>> +    size = kvm_align_section(section, &start_addr);
>>>> +    if (!size) {
>>>>          return 0;
>>>> -    } else {
>>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>>      }
>>>> +
>>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>>> +    if (!mem) {
>>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>>> +        abort();
>>>> +    }
>>>
>>> FYI, this abort now triggers with the "isa-vga" device:
>>>
>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>>> kvm_section_update_flags: error finding slot
>>> Aborted (core dumped)
>>>
>>> Any ideas how to fix this?
>>
>> Reverting, unless David has some time to look into it.
>>
>> Paolo
>>
> 
> Just returned from vacation, I'll have a look this week.
> 
> Thanks for that nice reproducer Thomas, that should help a lot!

FWIW, I've found the problem with the scripts/device-crash-test script,
so you might want to run that, too, before submitting a new version.

 Thomas
David Hildenbrand Oct. 16, 2017, 9:19 a.m. UTC | #5 
On 16.10.2017 10:52, Thomas Huth wrote:
> On 16.10.2017 09:16, David Hildenbrand wrote:
>> On 10.10.2017 15:29, Paolo Bonzini wrote:
>>> On 10/10/2017 11:06, Thomas Huth wrote:
>>>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>>>> Let's properly align the sections first and bail out if we would ever
>>>>> get called with a memory section we don't know yet.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>> index b677d1b13e..2ae459453d 100644
>>>>> --- a/accel/kvm/kvm-all.c
>>>>> +++ b/accel/kvm/kvm-all.c
>>>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>>>                                      MemoryRegionSection *section)
>>>>>  {
>>>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>>>> -    ram_addr_t size = int128_get64(section->size);
>>>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>>>> +    hwaddr start_addr, size;
>>>>> +    KVMSlot *mem;
>>>>>  
>>>>> -    if (mem == NULL)  {
>>>>> +    size = kvm_align_section(section, &start_addr);
>>>>> +    if (!size) {
>>>>>          return 0;
>>>>> -    } else {
>>>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>>>      }
>>>>> +
>>>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>>>> +    if (!mem) {
>>>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>>>> +        abort();
>>>>> +    }
>>>>
>>>> FYI, this abort now triggers with the "isa-vga" device:
>>>>
>>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>>>> kvm_section_update_flags: error finding slot
>>>> Aborted (core dumped)
>>>>
>>>> Any ideas how to fix this?
>>>
>>> Reverting, unless David has some time to look into it.
>>>
>>> Paolo
>>>
>>
>> Just returned from vacation, I'll have a look this week.
>>
>> Thanks for that nice reproducer Thomas, that should help a lot!
> 
> FWIW, I've found the problem with the scripts/device-crash-test script,
> so you might want to run that, too, before submitting a new version.
> 
>  Thomas
> 

This one was easy to fix, there was just one place where the asssumption
"only called with known sections" was wrong.

See "memory: call log_start after region_add"
diff mbox series

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b677d1b13e..2ae459453d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -411,15 +411,21 @@  static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
 static int kvm_section_update_flags(KVMMemoryListener *kml,
                                     MemoryRegionSection *section)
 {
-    hwaddr phys_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
+    hwaddr start_addr, size;
+    KVMSlot *mem;
 
-    if (mem == NULL)  {
+    size = kvm_align_section(section, &start_addr);
+    if (!size) {
         return 0;
-    } else {
-        return kvm_slot_update_flags(kml, mem, section->mr);
     }
+
+    mem = kvm_lookup_matching_slot(kml, start_addr, size);
+    if (!mem) {
+        fprintf(stderr, "%s: error finding slot\n", __func__);
+        abort();
+    }
+
+    return kvm_slot_update_flags(kml, mem, section->mr);
 }
 
 static void kvm_log_start(MemoryListener *listener,