[Qemu-devel] [PATCH v1] memory: call log_start after region_add

[Qemu-devel] [PATCH v1] memory: call log_start after region_add

[David Hildenbrand]

It might be confusing for some listener implementations that implement
both, region_add and log_start (e.g. KVM) if we call log_start before an
actual region was added using region_add.

This makes current KVM code trigger an assertion
("kvm_section_update_flags: error finding slot"). So let's just reverse
the order instead of tolerating log_start on yet unknown regions.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 5e6351a6c1..f39b8592bf 100644
--- a/memory.c
+++ b/memory.c
@@ -2607,12 +2607,12 @@ static void listener_add_address_space(MemoryListener *listener,
             .offset_within_address_space = int128_get64(fr->addr.start),
             .readonly = fr->readonly,
         };
-        if (fr->dirty_log_mask && listener->log_start) {
-            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
-        }
         if (listener->region_add) {
             listener->region_add(listener, &section);
         }
+        if (fr->dirty_log_mask && listener->log_start) {
+            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
+        }
     }
     if (listener->commit) {
         listener->commit(listener);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1] memory: call log_start after region_add

[David Hildenbrand]

On 16.10.2017 11:18, David Hildenbrand wrote:
> It might be confusing for some listener implementations that implement
> both, region_add and log_start (e.g. KVM) if we call log_start before an
> actual region was added using region_add.
> 
> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot"). So let's just reverse
> the order instead of tolerating log_start on yet unknown regions.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 5e6351a6c1..f39b8592bf 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2607,12 +2607,12 @@ static void listener_add_address_space(MemoryListener *listener,
>              .offset_within_address_space = int128_get64(fr->addr.start),
>              .readonly = fr->readonly,
>          };
> -        if (fr->dirty_log_mask && listener->log_start) {
> -            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
> -        }
>          if (listener->region_add) {
>              listener->region_add(listener, &section);
>          }
> +        if (fr->dirty_log_mask && listener->log_start) {
> +            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
> +        }
>      }
>      if (listener->commit) {
>          listener->commit(listener);
> 

... deciding to tolerate it in KVM instead (just as it used to be), so
please ignore this patch.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1] memory: call log_start after region_add

[Paolo Bonzini]

On 16/10/2017 11:36, David Hildenbrand wrote:
>> -        if (fr->dirty_log_mask && listener->log_start) {
>> -            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
>> -        }
>>          if (listener->region_add) {
>>              listener->region_add(listener, &section);
>>          }
>> +        if (fr->dirty_log_mask && listener->log_start) {
>> +            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
>> +        }
>>      }
>>      if (listener->commit) {
>>          listener->commit(listener);
>>
> ... deciding to tolerate it in KVM instead (just as it used to be), so
> please ignore this patch.

Why?  It makes no sense to start logging on a non-existent region, and
this behavior is unique to listener_add_address_space.

Paolo

Re: [Qemu-devel] [PATCH v1] memory: call log_start after region_add

[David Hildenbrand]

On 16.10.2017 14:19, Paolo Bonzini wrote:
> On 16/10/2017 11:36, David Hildenbrand wrote:
>>> -        if (fr->dirty_log_mask && listener->log_start) {
>>> -            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
>>> -        }
>>>          if (listener->region_add) {
>>>              listener->region_add(listener, &section);
>>>          }
>>> +        if (fr->dirty_log_mask && listener->log_start) {
>>> +            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
>>> +        }
>>>      }
>>>      if (listener->commit) {
>>>          listener->commit(listener);
>>>
>> ... deciding to tolerate it in KVM instead (just as it used to be), so
>> please ignore this patch.
> 
> Why?  It makes no sense to start logging on a non-existent region, and
> this behavior is unique to listener_add_address_space.

As you requested, will add everything into one patch series and send out
for review.

Thanks!

-- 

Thanks,

David