Skip to contentSkip to conversation

memory: call log_start after region_add

PATCH In review David Hildenbrand 2 participants 4 messages Oct 16, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 memory: call log_start after region_add

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.

---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
memory.c +3 -3
index 5e6351a6c1..f39b8592bf 100644
@@ -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);
Signature · 1 line
2.13.5
#1 lore raw
Quoted 25 lines · 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.

---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
memory.c +0 -2
index 5e6351a6c1..f39b8592bf 100644
@@ -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);

memory.c static void listener_add_address_space(MemoryListener *listener, · line 2618

Quoted 10 lines
memory.c +3 -1
-        }
         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.

Signature · 3 lines
Thanks,

David
#2 lore raw
Quoted 13 lines · David Hildenbrand

memory.c static void listener_add_address_space(MemoryListener *listener, · line 2618

Quoted 13 lines
memory.c +3 -3
-        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);
Quoted 2 lines

... 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

#3 lore raw
Quoted 14 lines · Paolo Bonzini
Quoted 13 lines · David Hildenbrand
Quoted 13 lines
memory.c +3 -3
-        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);
Quoted 5 lines
Quoted 2 lines

... 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!

Signature · 3 lines
Thanks,

David