[uq/master patch 0/5] prepare for qemu-kvm's usage of upstream memslot code

[uq/master patch 0/5] prepare for qemu-kvm's usage of upstream memslot code

[Marcelo Tosatti]

[Qemu-devel] [uq/master patch 0/5] prepare for qemu-kvm's usage of upstream memslot code

[Marcelo Tosatti]

[uq/master patch 1/5] vga: fix typo in length passed to kvm_log_stop

[Marcelo Tosatti]

[-- Attachment #1: fix-vga-thinko --]
[-- Type: text/plain, Size: 594 bytes --]

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/hw/vga.c
===================================================================
--- qemu-kvm.orig/hw/vga.c
+++ qemu-kvm/hw/vga.c
@@ -1613,8 +1613,8 @@ void vga_dirty_log_stop(VGACommonState *
 	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
 
     if (kvm_enabled() && s->lfb_vram_mapped) {
-	kvm_log_stop(isa_mem_base + 0xa0000, 0x80000);
-	kvm_log_stop(isa_mem_base + 0xa8000, 0x80000);
+	kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
+	kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE

[Qemu-devel] [uq/master patch 1/5] vga: fix typo in length passed to kvm_log_stop

[Marcelo Tosatti]

[-- Attachment #1: fix-vga-thinko --]
[-- Type: text/plain, Size: 592 bytes --]

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/hw/vga.c
===================================================================
--- qemu-kvm.orig/hw/vga.c
+++ qemu-kvm/hw/vga.c
@@ -1613,8 +1613,8 @@ void vga_dirty_log_stop(VGACommonState *
 	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
 
     if (kvm_enabled() && s->lfb_vram_mapped) {
-	kvm_log_stop(isa_mem_base + 0xa0000, 0x80000);
-	kvm_log_stop(isa_mem_base + 0xa8000, 0x80000);
+	kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
+	kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE

[uq/master patch 2/5] kvm: add logging count to slots

[Marcelo Tosatti]

[-- Attachment #1: memslot-logging-count --]
[-- Type: text/plain, Size: 4174 bytes --]

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 <mtosatti@redhat.com>

Index: qemu-kvm/kvm-all.c
===================================================================
--- qemu-kvm.orig/kvm-all.c
+++ qemu-kvm/kvm-all.c
@@ -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",

[Qemu-devel] [uq/master patch 2/5] kvm: add logging count to slots

[Marcelo Tosatti]

[-- Attachment #1: memslot-logging-count --]
[-- Type: text/plain, Size: 4172 bytes --]

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 <mtosatti@redhat.com>

Index: qemu-kvm/kvm-all.c
===================================================================
--- qemu-kvm.orig/kvm-all.c
+++ qemu-kvm/kvm-all.c
@@ -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",

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5024 bytes --]

Marcelo Tosatti wrote:
> 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.

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.

Jan

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu-kvm/kvm-all.c
> ===================================================================
> --- qemu-kvm.orig/kvm-all.c
> +++ qemu-kvm/kvm-all.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5024 bytes --]

Marcelo Tosatti wrote:
> 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.

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.

Jan

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu-kvm/kvm-all.c
> ===================================================================
> --- qemu-kvm.orig/kvm-all.c
> +++ qemu-kvm/kvm-all.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/24/2010 10:34 AM, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>    
>> 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.
>>      
> 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()/).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/24/2010 10:34 AM, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>    
>> 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.
>>      
> 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()/).

-- 
error compiling committee.c: too many arguments to function

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

Avi Kivity wrote:
> On 04/24/2010 10:34 AM, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>   
>>> 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.
>>>      
>> 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()/).
> 

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)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

Avi Kivity wrote:
> On 04/24/2010 10:34 AM, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>   
>>> 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.
>>>      
>> 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()/).
> 

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)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>
>> 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()/).
>>
>>      
> 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.

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

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

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>
>> 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()/).
>>
>>      
> 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.

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

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

-- 
error compiling committee.c: too many arguments to function

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>>
>>> 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()/).
>>>
>>>      
>> 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.

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

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

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>>
>>> 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()/).
>>>
>>>      
>> 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.

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

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

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>
>> 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.

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

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>
>> 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.

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

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.

-- 
error compiling committee.c: too many arguments to function

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Avi Kivity wrote:
> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>
>>> 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.

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

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Avi Kivity wrote:
> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>
>>> 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.

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

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 05:51 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>      
>>>        
>>>> 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.
>>      
> 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.

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

Sure (except that we don't have memory hotplug).

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

PCI hotplugging should be handled in migration as well.  Introducing 
dependencies among unrelated features and expecting upper layers to 
apply the correct constraints is unreasonable.

Currently we don't handle this, but we should.  One way to do it is to 
forward the hotplug/hotunplug along the migration channel.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 05:51 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>      
>>>        
>>>> 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.
>>      
> 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.

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

Sure (except that we don't have memory hotplug).

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

PCI hotplugging should be handled in migration as well.  Introducing 
dependencies among unrelated features and expecting upper layers to 
apply the correct constraints is unreasonable.

Currently we don't handle this, but we should.  One way to do it is to 
forward the hotplug/hotunplug along the migration channel.

-- 
error compiling committee.c: too many arguments to function

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

Avi Kivity wrote:
> On 04/25/2010 05:51 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>>     
>>>>       
>>>>> 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.
>>>      
>> 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.

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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

Avi Kivity wrote:
> On 04/25/2010 05:51 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>>     
>>>>       
>>>>> 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.
>>>      
>> 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.

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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>
>> 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)?


-- 

error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>
>> 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)?


-- 

error compiling committee.c: too many arguments to function

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

Avi Kivity wrote:
> On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>>
>>> 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).

Yes, an explicit slot reference in the API would be clearer.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

Avi Kivity wrote:
> On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>>
>>> 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).

Yes, an explicit slot reference in the API would be clearer.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 07:42 PM, Jan Kiszka wrote:
>
>> 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?
>
>    

I was worried the bitmap was indexed by physical addresses, but now I 
remember it is indexed by ram addresses, so it all works out.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Avi Kivity]

On 04/25/2010 07:42 PM, Jan Kiszka wrote:
>
>> 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?
>
>    

I was worried the bitmap was indexed by physical addresses, but now I 
remember it is indexed by ram addresses, so it all works out.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [uq/master patch 2/5] kvm: add logging count to slots

[Marcelo Tosatti]

On Sun, Apr 25, 2010 at 05:17:55PM +0300, Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
> >
> >>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()/).
> >>
> >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.
> 
> >  - 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.
> 
> >  - 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?

Don't see any guarantee that it is symmetrical. Anyway, will drop 
the patch from the series.

[Qemu-devel] Re: [uq/master patch 2/5] kvm: add logging count to slots

[Marcelo Tosatti]

On Sun, Apr 25, 2010 at 05:17:55PM +0300, Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
> >
> >>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()/).
> >>
> >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.
> 
> >  - 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.
> 
> >  - 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?

Don't see any guarantee that it is symmetrical. Anyway, will drop 
the patch from the series.

[uq/master patch 3/5] introduce leul_to_cpu

[Marcelo Tosatti]

[-- Attachment #1: leul_to_cpu --]
[-- Type: text/plain, Size: 518 bytes --]

To be used by next patch.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-memslot/bswap.h
===================================================================
--- qemu-memslot.orig/bswap.h
+++ qemu-memslot/bswap.h
@@ -205,8 +205,10 @@ static inline void cpu_to_be32wu(uint32_
 
 #ifdef HOST_WORDS_BIGENDIAN
 #define cpu_to_32wu cpu_to_be32wu
+#define leul_to_cpu(v) le ## HOST_LONG_BITS ## _to_cpu(v)
 #else
 #define cpu_to_32wu cpu_to_le32wu
+#define leul_to_cpu(v) (v)
 #endif
 
 #undef le_bswap

[Qemu-devel] [uq/master patch 3/5] introduce leul_to_cpu

[Marcelo Tosatti]

[-- Attachment #1: leul_to_cpu --]
[-- Type: text/plain, Size: 516 bytes --]

To be used by next patch.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-memslot/bswap.h
===================================================================
--- qemu-memslot.orig/bswap.h
+++ qemu-memslot/bswap.h
@@ -205,8 +205,10 @@ static inline void cpu_to_be32wu(uint32_
 
 #ifdef HOST_WORDS_BIGENDIAN
 #define cpu_to_32wu cpu_to_be32wu
+#define leul_to_cpu(v) le ## HOST_LONG_BITS ## _to_cpu(v)
 #else
 #define cpu_to_32wu cpu_to_le32wu
+#define leul_to_cpu(v) (v)
 #endif
 
 #undef le_bswap

[uq/master patch 4/5] kvm: port qemu-kvm's bitmap scanning

[Marcelo Tosatti]

[-- Attachment #1: use-qemukvm-bitmap-scan --]
[-- Type: text/plain, Size: 3519 bytes --]

Which is significantly faster.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/kvm-all.c
===================================================================
--- qemu-kvm.orig/kvm-all.c
+++ qemu-kvm/kvm-all.c
@@ -26,6 +26,7 @@
 #include "hw/hw.h"
 #include "gdbstub.h"
 #include "kvm.h"
+#include "bswap.h"
 
 /* KVM uses PAGE_SIZE in it's definition of COALESCED_MMIO_MAX */
 #define PAGE_SIZE TARGET_PAGE_SIZE
@@ -305,11 +306,41 @@ static int kvm_set_migration_log(int ena
     return 0;
 }
 
-static int test_le_bit(unsigned long nr, unsigned char *addr)
-{
-    return (addr[nr >> 3] >> (nr & 7)) & 1;
+/* get kvm's dirty pages bitmap and update qemu's */
+static int kvm_get_dirty_pages_log_range(unsigned long start_addr,
+                                         unsigned long *bitmap,
+                                         unsigned long offset,
+                                         unsigned long mem_size)
+{
+    unsigned int i, j;
+    unsigned long page_number, addr, addr1, c;
+    ram_addr_t ram_addr;
+    unsigned int len = ((mem_size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) /
+        HOST_LONG_BITS;
+
+    /*
+     * bitmap-traveling is faster than memory-traveling (for addr...)
+     * especially when most of the memory is not dirty.
+     */
+    for (i = 0; i < len; i++) {
+        if (bitmap[i] != 0) {
+            c = leul_to_cpu(bitmap[i]);
+            do {
+                j = ffsl(c) - 1;
+                c &= ~(1ul << j);
+                page_number = i * HOST_LONG_BITS + j;
+                addr1 = page_number * TARGET_PAGE_SIZE;
+                addr = offset + addr1;
+                ram_addr = cpu_get_physical_page_desc(addr);
+                cpu_physical_memory_set_dirty(ram_addr);
+            } while (c != 0);
+        }
+    }
+    return 0;
 }
 
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
+
 /**
  * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
  * This function updates qemu's dirty bitmap using cpu_physical_memory_set_dirty().
@@ -323,8 +354,6 @@ static int kvm_physical_sync_dirty_bitma
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
-    target_phys_addr_t phys_addr;
-    ram_addr_t addr;
     KVMDirtyLog d;
     KVMSlot *mem;
     int ret = 0;
@@ -336,7 +365,7 @@ static int kvm_physical_sync_dirty_bitma
             break;
         }
 
-        size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
+        size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS), HOST_LONG_BITS) / 8;
         if (!d.dirty_bitmap) {
             d.dirty_bitmap = qemu_malloc(size);
         } else if (size > allocated_size) {
@@ -353,17 +382,9 @@ static int kvm_physical_sync_dirty_bitma
             break;
         }
 
-        for (phys_addr = mem->start_addr, addr = mem->phys_offset;
-             phys_addr < mem->start_addr + mem->memory_size;
-             phys_addr += TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
-            unsigned char *bitmap = (unsigned char *)d.dirty_bitmap;
-            unsigned nr = (phys_addr - mem->start_addr) >> TARGET_PAGE_BITS;
-
-            if (test_le_bit(nr, bitmap)) {
-                cpu_physical_memory_set_dirty(addr);
-            }
-        }
-        start_addr = phys_addr;
+        kvm_get_dirty_pages_log_range(mem->start_addr, d.dirty_bitmap,
+                                      mem->start_addr, mem->memory_size);
+        start_addr = mem->start_addr + mem->memory_size;
     }
     qemu_free(d.dirty_bitmap);
 

[Qemu-devel] [uq/master patch 4/5] kvm: port qemu-kvm's bitmap scanning

[Marcelo Tosatti]

[-- Attachment #1: use-qemukvm-bitmap-scan --]
[-- Type: text/plain, Size: 3517 bytes --]

Which is significantly faster.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/kvm-all.c
===================================================================
--- qemu-kvm.orig/kvm-all.c
+++ qemu-kvm/kvm-all.c
@@ -26,6 +26,7 @@
 #include "hw/hw.h"
 #include "gdbstub.h"
 #include "kvm.h"
+#include "bswap.h"
 
 /* KVM uses PAGE_SIZE in it's definition of COALESCED_MMIO_MAX */
 #define PAGE_SIZE TARGET_PAGE_SIZE
@@ -305,11 +306,41 @@ static int kvm_set_migration_log(int ena
     return 0;
 }
 
-static int test_le_bit(unsigned long nr, unsigned char *addr)
-{
-    return (addr[nr >> 3] >> (nr & 7)) & 1;
+/* get kvm's dirty pages bitmap and update qemu's */
+static int kvm_get_dirty_pages_log_range(unsigned long start_addr,
+                                         unsigned long *bitmap,
+                                         unsigned long offset,
+                                         unsigned long mem_size)
+{
+    unsigned int i, j;
+    unsigned long page_number, addr, addr1, c;
+    ram_addr_t ram_addr;
+    unsigned int len = ((mem_size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) /
+        HOST_LONG_BITS;
+
+    /*
+     * bitmap-traveling is faster than memory-traveling (for addr...)
+     * especially when most of the memory is not dirty.
+     */
+    for (i = 0; i < len; i++) {
+        if (bitmap[i] != 0) {
+            c = leul_to_cpu(bitmap[i]);
+            do {
+                j = ffsl(c) - 1;
+                c &= ~(1ul << j);
+                page_number = i * HOST_LONG_BITS + j;
+                addr1 = page_number * TARGET_PAGE_SIZE;
+                addr = offset + addr1;
+                ram_addr = cpu_get_physical_page_desc(addr);
+                cpu_physical_memory_set_dirty(ram_addr);
+            } while (c != 0);
+        }
+    }
+    return 0;
 }
 
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
+
 /**
  * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
  * This function updates qemu's dirty bitmap using cpu_physical_memory_set_dirty().
@@ -323,8 +354,6 @@ static int kvm_physical_sync_dirty_bitma
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
-    target_phys_addr_t phys_addr;
-    ram_addr_t addr;
     KVMDirtyLog d;
     KVMSlot *mem;
     int ret = 0;
@@ -336,7 +365,7 @@ static int kvm_physical_sync_dirty_bitma
             break;
         }
 
-        size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
+        size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS), HOST_LONG_BITS) / 8;
         if (!d.dirty_bitmap) {
             d.dirty_bitmap = qemu_malloc(size);
         } else if (size > allocated_size) {
@@ -353,17 +382,9 @@ static int kvm_physical_sync_dirty_bitma
             break;
         }
 
-        for (phys_addr = mem->start_addr, addr = mem->phys_offset;
-             phys_addr < mem->start_addr + mem->memory_size;
-             phys_addr += TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
-            unsigned char *bitmap = (unsigned char *)d.dirty_bitmap;
-            unsigned nr = (phys_addr - mem->start_addr) >> TARGET_PAGE_BITS;
-
-            if (test_le_bit(nr, bitmap)) {
-                cpu_physical_memory_set_dirty(addr);
-            }
-        }
-        start_addr = phys_addr;
+        kvm_get_dirty_pages_log_range(mem->start_addr, d.dirty_bitmap,
+                                      mem->start_addr, mem->memory_size);
+        start_addr = mem->start_addr + mem->memory_size;
     }
     qemu_free(d.dirty_bitmap);
 

[uq/master patch 5/5] introduce qemu_ram_map

[Marcelo Tosatti]

[-- Attachment #1: qemu-ram-mmap --]
[-- Type: text/plain, Size: 1691 bytes --]

Which allows drivers to register an mmaped region into ram block mappings.
To be used by device assignment driver.

CC: Cam Macdonell <cam@cs.ualberta.ca>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/cpu-common.h
===================================================================
--- qemu-kvm.orig/cpu-common.h
+++ qemu-kvm/cpu-common.h
@@ -32,6 +32,7 @@ static inline void cpu_register_physical
 }
 
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
 ram_addr_t qemu_ram_alloc(ram_addr_t);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
Index: qemu-kvm/exec.c
===================================================================
--- qemu-kvm.orig/exec.c
+++ qemu-kvm/exec.c
@@ -2710,6 +2710,34 @@ static void *file_ram_alloc(ram_addr_t m
 }
 #endif
 
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+{
+    RAMBlock *new_block;
+
+    size = TARGET_PAGE_ALIGN(size);
+    new_block = qemu_malloc(sizeof(*new_block));
+
+    new_block->host = host;
+
+    new_block->offset = last_ram_offset;
+    new_block->length = size;
+
+    new_block->next = ram_blocks;
+    ram_blocks = new_block;
+
+    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
+        (last_ram_offset + size) >> TARGET_PAGE_BITS);
+    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+           0xff, size >> TARGET_PAGE_BITS);
+
+    last_ram_offset += size;
+
+    if (kvm_enabled())
+        kvm_setup_guest_memory(new_block->host, size);
+
+    return new_block->offset;
+}
+
 ram_addr_t qemu_ram_alloc(ram_addr_t size)
 {
     RAMBlock *new_block;

[Qemu-devel] [uq/master patch 5/5] introduce qemu_ram_map

[Marcelo Tosatti]

[-- Attachment #1: qemu-ram-mmap --]
[-- Type: text/plain, Size: 1689 bytes --]

Which allows drivers to register an mmaped region into ram block mappings.
To be used by device assignment driver.

CC: Cam Macdonell <cam@cs.ualberta.ca>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-kvm/cpu-common.h
===================================================================
--- qemu-kvm.orig/cpu-common.h
+++ qemu-kvm/cpu-common.h
@@ -32,6 +32,7 @@ static inline void cpu_register_physical
 }
 
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
 ram_addr_t qemu_ram_alloc(ram_addr_t);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
Index: qemu-kvm/exec.c
===================================================================
--- qemu-kvm.orig/exec.c
+++ qemu-kvm/exec.c
@@ -2710,6 +2710,34 @@ static void *file_ram_alloc(ram_addr_t m
 }
 #endif
 
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+{
+    RAMBlock *new_block;
+
+    size = TARGET_PAGE_ALIGN(size);
+    new_block = qemu_malloc(sizeof(*new_block));
+
+    new_block->host = host;
+
+    new_block->offset = last_ram_offset;
+    new_block->length = size;
+
+    new_block->next = ram_blocks;
+    ram_blocks = new_block;
+
+    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
+        (last_ram_offset + size) >> TARGET_PAGE_BITS);
+    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+           0xff, size >> TARGET_PAGE_BITS);
+
+    last_ram_offset += size;
+
+    if (kvm_enabled())
+        kvm_setup_guest_memory(new_block->host, size);
+
+    return new_block->offset;
+}
+
 ram_addr_t qemu_ram_alloc(ram_addr_t size)
 {
     RAMBlock *new_block;

Re: [uq/master patch 0/5] prepare for qemu-kvm's usage of upstream memslot code

[Avi Kivity]

On 04/23/2010 08:04 PM, Marcelo Tosatti wrote:

Looks good.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [uq/master patch 0/5] prepare for qemu-kvm's usage of upstream memslot code

[Avi Kivity]

On 04/23/2010 08:04 PM, Marcelo Tosatti wrote:

Looks good.

-- 
error compiling committee.c: too many arguments to function