prepare for qemu-kvm's usage of upstream memslot code

Otherwise there is no way to differentiate between global and slot specific logging, so for example

vga dirty log start
migration start
migration fail

Disables dirty logging for the vga slot.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: qemu-kvm/kvm-all.c
===================================================================

kvm-all.c +44 -23

@@ -47,6 +47,7 @@ typedef struct KVMSlot
     ram_addr_t phys_offset;
     int slot;
     int flags;
+    int logging_count;
 } KVMSlot;
 
 typedef struct kvm_dirty_log KVMDirtyLog;

@@ -218,20 +219,11 @@ err:
 /*
  * dirty pages logging control
  */
-static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
-                                      ram_addr_t size, int flags, int mask)
+static int kvm_dirty_pages_log_change(KVMSlot *mem, int flags, int mask)
 {
     KVMState *s = kvm_state;
-    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
     int old_flags;
 
-    if (mem == NULL)  {
-            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
-                    TARGET_FMT_plx "\n", __func__, phys_addr,
-                    (target_phys_addr_t)(phys_addr + size - 1));
-            return -EINVAL;
-    }
-
     old_flags = mem->flags;
 
     flags = (mem->flags & ~mask) | flags;

@@ -250,16 +242,42 @@ static int kvm_dirty_pages_log_change(ta
 
 int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
 {
-        return kvm_dirty_pages_log_change(phys_addr, size,
-                                          KVM_MEM_LOG_DIRTY_PAGES,
-                                          KVM_MEM_LOG_DIRTY_PAGES);
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
+
+    if (mem == NULL)  {
+            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
+                    TARGET_FMT_plx "\n", __func__, phys_addr,
+                    (target_phys_addr_t)(phys_addr + size - 1));
+            return -EINVAL;
+    }
+
+    if (mem->logging_count++)
+        return 0;
+
+    return kvm_dirty_pages_log_change(mem,
+                                      KVM_MEM_LOG_DIRTY_PAGES,
+                                      KVM_MEM_LOG_DIRTY_PAGES);
 }
 
 int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
 {
-        return kvm_dirty_pages_log_change(phys_addr, size,
-                                          0,
-                                          KVM_MEM_LOG_DIRTY_PAGES);
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
+
+    if (mem == NULL)  {
+            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
+                    TARGET_FMT_plx "\n", __func__, phys_addr,
+                    (target_phys_addr_t)(phys_addr + size - 1));
+            return -EINVAL;
+    }
+
+    if (--mem->logging_count)
+        return 0;
+
+    return kvm_dirty_pages_log_change(mem,
+                                      0,
+                                      KVM_MEM_LOG_DIRTY_PAGES);
 }
 
 static int kvm_set_migration_log(int enable)

@@ -273,12 +291,15 @@ static int kvm_set_migration_log(int ena
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         mem = &s->slots[i];
 
-        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
-            continue;
-        }
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            return err;
+        if (mem->memory_size) {
+            if (enable) {
+                err = kvm_log_start(mem->start_addr, mem->memory_size);
+            } else {
+                err = kvm_log_stop(mem->start_addr, mem->memory_size);
+            }
+            if (err) {
+                return err;
+            }
         }
     }
     return 0;

@@ -442,6 +463,7 @@ static void kvm_set_phys_mem(target_phys
 
         /* unregister the overlapping slot */
         mem->memory_size = 0;
+        mem->logging_count = 0;
         err = kvm_set_user_memory_region(s, mem);
         if (err) {
             fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",
 
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Marcelo Tosatti

Otherwise there is no way to differentiate between global and slot specific logging, so for example

vga dirty log start
migration start
migration fail

Quoted 2 lines

Disables dirty logging for the vga slot.

This is not true (unless there is a bug): Migration logging is tracked via KVMState::migration_log and vga logging via KVMSlot::flags. Both are merged in kvm_set_user_memory_region. Thus no such change is required for upstream.

Quoted 9 lines · Jan Kiszka

Quoted 7 lines · Marcelo Tosatti

Otherwise there is no way to differentiate between global and slot specific logging, so for example

vga dirty log start
migration start
migration fail

Quoted 7 lines

Quoted 2 lines

Disables dirty logging for the vga slot.

This is not true (unless there is a bug): Migration logging is tracked via KVMState::migration_log and vga logging via KVMSlot::flags. Both are merged in kvm_set_user_memory_region. Thus no such change is required for upstream.

It's still a good idea. The current API assumes that there will be only one slot-based client (or that multiple clients will keep the refcount themselves).

After the bytemap -> multiple bitmaps conversion this can be extended to each client getting its own bitmap (and therefore, s/refcount/list of bitmaps/ and s/!refcount/list_empty()/).

Quoted 5 lines

It's still a good idea. The current API assumes that there will be only one slot-based client (or that multiple clients will keep the refcount themselves).

Quoted 5 lines

After the bytemap -> multiple bitmaps conversion this can be extended to each client getting its own bitmap (and therefore, s/refcount/list of bitmaps/ and s/!refcount/list_empty()/).

No concerns if
- there is an existing use case for multiple clients, at least in qemu-kvm

- the logging API is consistently converted, not just extended (IOW, migration_log is converted to logging_count)

- someone signs he checked that current use of start/stop in qemu is completely symmetrical (I think to remember this used to be not the case, but I might be wrong)

Quoted 11 lines · Jan Kiszka

Quoted 11 lines

It's still a good idea. The current API assumes that there will be only one slot-based client (or that multiple clients will keep the refcount themselves).

After the bytemap -> multiple bitmaps conversion this can be extended to
each client getting its own bitmap (and therefore, s/refcount/list of bitmaps/ and s/!refcount/list_empty()/).

Quoted 4 lines

No concerns if
- there is an existing use case for multiple clients, at least in qemu-kvm

There isn't. But I don't like hidden breakage.

Quoted 4 lines

- the logging API is consistently converted, not just extended (IOW, migration_log is converted to logging_count)

migration_log needs to remain global, since we want hotplug memory to autostart logging.

Quoted 5 lines

- someone signs he checked that current use of start/stop in qemu is completely symmetrical (I think to remember this used to be not the case, but I might be wrong)

I remember this too. Marcelo?

Quoted 3 lines

There isn't. But I don't like hidden breakage.

It's (so far) an unproblematic API property we can document. I don't like changing APIs just for "there might be the case that...".

Quoted 7 lines

Quoted 4 lines

- the logging API is consistently converted, not just extended (IOW, migration_log is converted to logging_count)

migration_log needs to remain global, since we want hotplug memory to autostart logging.

Can't follow yet, what will be the usage pattern of kvm_set_migration_log? Or would the hotplug code require a separate interface? Is it already the multi-client use case I'm looking for?

Quoted 6 lines · Jan Kiszka

Quoted 3 lines

There isn't. But I don't like hidden breakage.

It's (so far) an unproblematic API property we can document. I don't like changing APIs just for "there might be the case that...".

I guess it's one of those agree to disagree things. I dislike known broken APIs even if their none of their users are affected.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

- the logging API is consistently converted, not just extended (IOW, migration_log is converted to logging_count)

migration_log needs to remain global, since we want hotplug memory to autostart logging.

Quoted 4 lines

Can't follow yet, what will be the usage pattern of kvm_set_migration_log? Or would the hotplug code require a separate interface? Is it already the multi-client use case I'm looking for?

kvm_set_migration_log() means, start logging now for all current and future memory, until disabled.

It could be implemented in terms of kvm_log_start() (which would provide a multi-client use case), but it isn't now.

I guess it is a logical example of how two clients can exist, even though they don't step on each others toes in practice since their enable flags are kept separate by the implementation.

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

Quoted 4 lines

There isn't. But I don't like hidden breakage.

It's (so far) an unproblematic API property we can document. I don't like changing APIs just for "there might be the case that...".

Quoted 3 lines

I guess it's one of those agree to disagree things. I dislike known broken APIs even if their none of their users are affected.

The API is not broken. I intentionally designed it for the single user as I saw no need for more. If I oversaw something, I would really like to learn about these cases.

Quoted 3 lines

kvm_set_migration_log() means, start logging now for all current and future memory, until disabled.

Hmm, you mean plugging memory during ongoing migration is valid and can be handled?

I'm a bit skeptical. What makes this different from, say, PCI hotplugging which should be a no-go during migration as well?

Quoted 12 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

Quoted 4 lines

There isn't. But I don't like hidden breakage.

It's (so far) an unproblematic API property we can document. I don't like changing APIs just for "there might be the case that...".

Quoted 7 lines

Quoted 3 lines

I guess it's one of those agree to disagree things. I dislike known broken APIs even if their none of their users are affected.

The API is not broken. I intentionally designed it for the single user as I saw no need for more. If I oversaw something, I would really like to learn about these cases.

The fact that the API assumes a single user is what's broken IMO.

If the API were to take a memory slot as parameter you could say it is the responsibility of the slot's owner to multiplex (and since vga has a single owner, no need to multiplex). But it takes a range.

Quoted 7 lines

The fact that the API assumes a single user is what's broken IMO.

If the API were to take a memory slot as parameter you could say it is the responsibility of the slot's owner to multiplex (and since vga has a single owner, no need to multiplex). But it takes a range.

No, the API accepts only a single slot. If you try passing bogus ranges that span multiple or incomplete slots, you get what you deserve - a bug message.

Quoted 11 lines · Jan Kiszka

Quoted 7 lines

The fact that the API assumes a single user is what's broken IMO.

If the API were to take a memory slot as parameter you could say it is the responsibility of the slot's owner to multiplex (and since vga has a single owner, no need to multiplex). But it takes a range.

No, the API accepts only a single slot. If you try passing bogus ranges that span multiple or incomplete slots, you get what you deserve - a bug message.

I see. In its qemu-kvm iteration, it would iterate over slots and accept incomplete slots (it's okay to log more than requested). If the API is for a slot, it should accept a slot, not a range (when we move to a slots representation in qemu).

Unrelated:

        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);

Will this sync to the right place (whatever those windows alias to)?

Quoted 9 lines

Unrelated:

         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);

Will this sync to the right place (whatever those windows alias to)?

It should. Or where do your worries come from?

Quoted 5 lines · Jan Kiszka

Quoted 5 lines

It's still a good idea. The current API assumes that there will be only one slot-based client (or that multiple clients will keep the refcount themselves).

Quoted 7 lines

Quoted 4 lines

After the bytemap -> multiple bitmaps conversion this can be extended to each client getting its own bitmap (and therefore, s/refcount/list of bitmaps/ and s/!refcount/list_empty()/).

No concerns if
- there is an existing use case for multiple clients, at least in qemu-kvm

There isn't. But I don't like hidden breakage.

Quoted 2 lines

- the logging API is consistently converted, not just extended (IOW, migration_log is converted to logging_count)

migration_log needs to remain global, since we want hotplug memory to autostart logging.

Quoted 3 lines

- someone signs he checked that current use of start/stop in qemu is completely symmetrical (I think to remember this used to be not the case, but I might be wrong)

I remember this too. Marcelo?