[Qemu-devel] [PATCH 0/2] Xen VGA dirtybit support

[Qemu-devel] [PATCH 0/2] Xen VGA dirtybit support

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

Hi,

This two patchs provides the support for sync dirty bitmap. The sync dirty
bitmap is not provide for Xen 3.3.

This serie depends on the other serie "Xen device model support".

Anthony PERARD (2):
  xen: Add xc_domain_add_to_physmap to xen_interface.
  xen: Introduce VGA sync dirty bitmap support

 configure           |   29 ++++++-
 hw/vga.c            |    7 ++
 hw/xen.h            |    2 +
 hw/xen_interfaces.c |   31 +++++++
 hw/xen_interfaces.h |    5 +
 hw/xen_redirect.h   |    1 +
 xen-all.c           |  233 +++++++++++++++++++++++++++++++++++++++++++++++++++
 xen-stub.c          |   11 +++
 8 files changed, 318 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH 1/2] xen: Add xc_domain_add_to_physmap to xen_interface.

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

This function will be used to support sync dirty bitmap.

This come with a check against every Xen release, and special
implementation for Xen version that doesn't have this specific call.

This function will not be usable with Xen 3.3 because the behavior is
different.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 configure           |   29 ++++++++++++++++++++++++++++-
 hw/xen_interfaces.c |   31 +++++++++++++++++++++++++++++++
 hw/xen_interfaces.h |    5 +++++
 hw/xen_redirect.h   |    1 +
 4 files changed, 65 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index f3c524e..642d344 100755
--- a/configure
+++ b/configure
@@ -1154,6 +1154,7 @@ int main(void) {
   xs_daemon_open();
   xc_interface_open(0, 0, 0);
   xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+  xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
   xc_gnttab_open(xc);
   return 0;
 }
@@ -1173,10 +1174,14 @@ EOF
 # error HVM_MAX_VCPUS not defined
 #endif
 int main(void) {
+  struct xen_add_to_physmap xatp = {
+    .domid = 0, .space = XENMAPSPACE_gmfn, .idx = 0, .gpfn = 0,
+  };
   xs_daemon_open();
   xc_interface_open();
   xc_gnttab_open();
   xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+  xc_memory_op(0, XENMEM_add_to_physmap, &xatp);
   return 0;
 }
 EOF
@@ -1185,7 +1190,29 @@ EOF
     xen_ctrl_version=400
     xen=yes
 
-  # Xen 3.3.0, 3.4.0
+  # Xen 3.4.0
+  elif (
+      cat > $TMPC <<EOF
+#include <xenctrl.h>
+#include <xs.h>
+int main(void) {
+  struct xen_add_to_physmap xatp = {
+    .domid = 0, .space = XENMAPSPACE_gmfn, .idx = 0, .gpfn = 0,
+  };
+  xs_daemon_open();
+  xc_interface_open();
+  xc_gnttab_open();
+  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+  xc_memory_op(0, XENMEM_add_to_physmap, &xatp);
+  return 0;
+}
+EOF
+      compile_prog "" "$xen_libs"
+    ) ; then
+    xen_ctrl_version=340
+    xen=yes
+
+  # Xen 3.3.0
   elif (
       cat > $TMPC <<EOF
 #include <xenctrl.h>
diff --git a/hw/xen_interfaces.c b/hw/xen_interfaces.c
index 129e8b2..4d9862b 100644
--- a/hw/xen_interfaces.c
+++ b/hw/xen_interfaces.c
@@ -124,6 +124,20 @@ static void *map_foreign_batch(int xc_handle, uint32_t dom, int prot,
     return xc_map_foreign_batch(xc_handle, dom, prot, (xen_pfn_t*)arr, num);
 }
 
+static int domain_add_to_physmap(qemu_xc_interface xc_handle, uint32_t domid,
+                                 unsigned int space, unsigned long idx,
+                                 xen_pfn_t gpfn)
+{
+    struct xen_add_to_physmap xatp = {
+        .domid = domid,
+        .space = space,
+        .idx = idx,
+        .gpfn = gpfn,
+    };
+
+    return xc_memory_op(xc_handle, XENMEM_add_to_physmap, &xatp);
+}
+
 struct XenIfOps xc_xen = {
     .interface_open                 = interface_open,
     .interface_close                = xc_interface_close,
@@ -131,6 +145,7 @@ struct XenIfOps xc_xen = {
     .map_foreign_pages              = xc_map_foreign_pages,
     .map_foreign_bulk               = map_foreign_batch,
     .domain_populate_physmap_exact  = xc_domain_memory_populate_physmap,
+    .domain_add_to_physmap          = domain_add_to_physmap,
 };
 
 #  elif CONFIG_XEN_CTRL_INTERFACE_VERSION < 410
@@ -141,6 +156,20 @@ static qemu_xc_interface interface_open(xentoollog_logger *logger,
     return xc_interface_open();
 }
 
+static int domain_add_to_physmap(qemu_xc_interface xc_handle, uint32_t domid,
+                                 unsigned int space, unsigned long idx,
+                                 xen_pfn_t gpfn)
+{
+    struct xen_add_to_physmap xatp = {
+        .domid = domid,
+        .space = space,
+        .idx = idx,
+        .gpfn = gpfn,
+    };
+
+    return xc_memory_op(xc_handle, XENMEM_add_to_physmap, &xatp);
+}
+
 struct XenIfOps xc_xen = {
     .interface_open                 = interface_open,
     .interface_close                = xc_interface_close,
@@ -148,6 +177,7 @@ struct XenIfOps xc_xen = {
     .map_foreign_pages              = xc_map_foreign_pages,
     .map_foreign_bulk               = xc_map_foreign_bulk,
     .domain_populate_physmap_exact  = xc_domain_memory_populate_physmap,
+    .domain_add_to_physmap          = domain_add_to_physmap,
 };
 
 #  else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 410 */
@@ -158,6 +188,7 @@ struct XenIfOps xc_xen = {
     .map_foreign_pages              = xc_map_foreign_pages,
     .map_foreign_bulk               = xc_map_foreign_bulk,
     .domain_populate_physmap_exact  = xc_domain_populate_physmap_exact,
+    .domain_add_to_physmap          = xc_domain_add_to_physmap,
 };
 #  endif
 
diff --git a/hw/xen_interfaces.h b/hw/xen_interfaces.h
index 0f698dc..be38345 100644
--- a/hw/xen_interfaces.h
+++ b/hw/xen_interfaces.h
@@ -108,6 +108,11 @@ struct XenIfOps {
                                          unsigned int extent_order,
                                          unsigned int mem_flags,
                                          xen_pfn_t *extent_start);
+    int (*domain_add_to_physmap)(qemu_xc_interface xc_handle,
+                                 uint32_t domid,
+                                 unsigned int space,
+                                 unsigned long idx,
+                                 xen_pfn_t gpfn);
 };
 extern struct XenIfOps xc;
 
diff --git a/hw/xen_redirect.h b/hw/xen_redirect.h
index 7cd6492..1b7c603 100644
--- a/hw/xen_redirect.h
+++ b/hw/xen_redirect.h
@@ -30,6 +30,7 @@
 #define xc_map_foreign_bulk         xc.map_foreign_bulk
 #define xc_domain_populate_physmap_exact \
                                     xc.domain_populate_physmap_exact
+#define xc_domain_add_to_physmap    xc.domain_add_to_physmap
 
 /* xenstore interface */
 #define xs_daemon_open              xs.daemon_open
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

This patch introduces phys memory client for Xen.

Only sync dirty_bitmap and set_memory are actually implemented.
migration_log will stay empty for the moment.

Xen can only log one range for bit change, so only the range in the
first call will be synced.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/vga.c   |    7 ++
 hw/xen.h   |    2 +
 xen-all.c  |  233 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xen-stub.c |   11 +++
 4 files changed, 253 insertions(+), 0 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index c057f4f..5f7a181 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -29,6 +29,7 @@
 #include "pixel_ops.h"
 #include "qemu-timer.h"
 #include "kvm.h"
+#include "xen.h"
 
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
@@ -1599,6 +1600,9 @@ void vga_dirty_log_start(VGACommonState *s)
 {
     if (kvm_enabled() && s->map_addr)
         kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+    if (xen_enabled() && s->map_addr) {
+        xen_log_start(s->map_addr, s->map_end - s->map_addr);
+    }
 
     if (kvm_enabled() && s->lfb_vram_mapped) {
         kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
@@ -1616,6 +1620,9 @@ void vga_dirty_log_stop(VGACommonState *s)
 {
     if (kvm_enabled() && s->map_addr)
 	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
+    if (xen_enabled() && s->map_addr) {
+        xen_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, 0x8000);
diff --git a/hw/xen.h b/hw/xen.h
index 8920550..b6cb098 100644
--- a/hw/xen.h
+++ b/hw/xen.h
@@ -50,6 +50,8 @@ int xen_init(int smp_cpus);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
+int xen_log_start(target_phys_addr_t phys_addr, ram_addr_t size);
+int xen_log_stop(target_phys_addr_t phys_addr, ram_addr_t size);
 #endif
 
 #endif /* QEMU_HW_XEN_H */
diff --git a/xen-all.c b/xen-all.c
index 939d9b7..f38c803 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -63,6 +63,18 @@ typedef struct XenIOState {
     Notifier exit;
 } XenIOState;
 
+typedef struct XenPhysmap {
+    target_phys_addr_t start_addr;
+    ram_addr_t size;
+    target_phys_addr_t phys_offset;
+    int flags;
+
+    QLIST_ENTRY(XenPhysmap) list;
+} XenPhysmap;
+
+static QLIST_HEAD(, XenPhysmap) xen_physmap =
+    QLIST_HEAD_INITIALIZER(xen_physmap);
+
 /* Xen specific function for piix pci */
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
@@ -162,6 +174,226 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size)
     qemu_free(pfn_list);
 }
 
+static XenPhysmap *link_exist(target_phys_addr_t start_addr)
+{
+    XenPhysmap *physmap = NULL;
+
+    start_addr = TARGET_PAGE_ALIGN(start_addr);
+    QLIST_FOREACH(physmap, &xen_physmap, list) {
+        if (physmap->size > 0 && physmap->start_addr == start_addr) {
+            return physmap;
+        }
+    }
+    return NULL;
+}
+
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
+static int already_physmapped(target_phys_addr_t phys_offset)
+{
+    XenPhysmap *physmap = NULL;
+
+    phys_offset = TARGET_PAGE_ALIGN(phys_offset);
+    QLIST_FOREACH(physmap, &xen_physmap, list) {
+        if (physmap->size > 0 && physmap->phys_offset <= phys_offset &&
+            phys_offset <= (physmap->phys_offset + physmap->size)) {
+            return 1;
+        }
+    }
+    return 0;
+}
+
+static int xen_add_to_physmap(target_phys_addr_t start_addr,
+                              ram_addr_t size,
+                              target_phys_addr_t phys_offset)
+{
+    unsigned long i = 0;
+    int rc = 0;
+    XenPhysmap *physmap = NULL;
+
+    if (already_physmapped(phys_offset)) {
+        return 0;
+    }
+
+    DPRINTF("mapping vram to %llx - %llx, from %llx\n", start_addr, start_addr + size, phys_offset);
+    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
+        unsigned long idx = (phys_offset >> TARGET_PAGE_BITS) + i;
+
+        xen_pfn_t gpfn = (start_addr >> TARGET_PAGE_BITS) + i;
+        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
+        if (rc) {
+            fprintf(stderr, "xen: add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
+                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
+            return -rc;
+        }
+    }
+
+    physmap = qemu_malloc(sizeof (XenPhysmap));
+
+    physmap->start_addr = start_addr;
+    physmap->size = size;
+    physmap->phys_offset = phys_offset;
+
+    QLIST_INSERT_HEAD(&xen_physmap, physmap, list);
+
+    xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
+                                   start_addr >> TARGET_PAGE_BITS,
+                                   (start_addr + size) >> TARGET_PAGE_BITS,
+                                   XEN_DOMCTL_MEM_CACHEATTR_WB);
+    return 0;
+}
+
+static int xen_remove_from_physmap(target_phys_addr_t start_addr,
+                                   ram_addr_t size)
+{
+    unsigned long i = 0;
+    int rc = 0;
+    XenPhysmap *physmap = NULL;
+    target_phys_addr_t phys_offset = 0;
+
+    physmap = link_exist(start_addr);
+    if (physmap == NULL) {
+        return -1;
+    }
+
+    phys_offset = physmap->phys_offset;
+    size = physmap->size;
+
+    DPRINTF("unmapping vram to %llx - %llx, from %llx\n", phys_offset, phys_offset + size, start_addr);
+    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
+        unsigned long idx = (start_addr >> TARGET_PAGE_BITS) + i;
+
+        xen_pfn_t gpfn = (phys_offset >> TARGET_PAGE_BITS) + i;
+        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
+        if (rc) {
+            fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
+                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
+            return -rc;
+        }
+    }
+
+    QLIST_REMOVE(physmap, list);
+    free(physmap);
+
+    return 0;
+}
+
+#else
+static int xen_add_to_physmap(target_phys_addr_t start_addr,
+                              ram_addr_t size,
+                              target_phys_addr_t phys_offset)
+{
+    return -ENOSYS;
+}
+
+static int xen_remove_from_physmap(target_phys_addr_t start_addr,
+                                   ram_addr_t size)
+{
+    return -ENOSYS;
+}
+#endif
+
+static void xen_client_set_memory(struct CPUPhysMemoryClient *client,
+				  target_phys_addr_t start_addr,
+				  ram_addr_t size,
+				  ram_addr_t phys_offset)
+{
+    ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
+    hvmmem_type_t mem_type;
+
+    start_addr = TARGET_PAGE_ALIGN(start_addr);
+    size = TARGET_PAGE_ALIGN(size);
+    phys_offset = TARGET_PAGE_ALIGN(phys_offset);
+
+    /* Xen does not need to know about this memory */
+    if (flags > IO_MEM_UNASSIGNED) {
+        return;
+    }
+
+    switch (flags) {
+    case IO_MEM_RAM:
+        xen_add_to_physmap(start_addr, size, phys_offset);
+        break;
+    case IO_MEM_ROM:
+        mem_type = HVMMEM_ram_ro;
+        if (xc_hvm_set_mem_type(xen_xc, xen_domid, mem_type,
+                                start_addr >> TARGET_PAGE_BITS,
+                                size >> TARGET_PAGE_BITS)) {
+            DPRINTF("xc_hvm_set_mem_type error, addr: "TARGET_FMT_plx"\n",
+                    start_addr);
+        }
+        break;
+    case IO_MEM_UNASSIGNED:
+        if (xen_remove_from_physmap(start_addr, size) < 0) {
+            DPRINTF("physmapping does not exist at "TARGET_FMT_plx"\n", start_addr);
+        }
+        break;
+    }
+}
+
+static int xen_sync_dirty_bitmap(target_phys_addr_t start_addr,
+                                 ram_addr_t size)
+{
+    target_phys_addr_t npages = size >> TARGET_PAGE_BITS;
+    target_phys_addr_t vram_offset = 0;
+    const int width = sizeof(unsigned long) * 8;
+    unsigned long bitmap[(npages + width - 1) / width];
+    int rc, i, j;
+    XenPhysmap *physmap = NULL;
+
+    physmap = link_exist(start_addr);
+    if (physmap) {
+        vram_offset = physmap->phys_offset;
+    } else {
+        vram_offset = start_addr;
+    }
+    rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
+                                 start_addr >> TARGET_PAGE_BITS, npages,
+                                 bitmap);
+    if (rc) {
+        return rc;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(bitmap); i++) {
+        unsigned long map = bitmap[i];
+        while (map != 0) {
+            j = ffsl(map) - 1;
+            map &= ~(1ul << j);
+            cpu_physical_memory_set_dirty(vram_offset + (i * width + j) * TARGET_PAGE_SIZE);
+        };
+    }
+
+    return 0;
+}
+
+int xen_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return xen_sync_dirty_bitmap(phys_addr, size);
+}
+
+int xen_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    /* Disable dirty bit tracking */
+    return xc_hvm_track_dirty_vram(xen_xc, xen_domid, 0, 0, NULL);
+}
+
+static int xen_client_sync_dirty_bitmap(struct CPUPhysMemoryClient *client,
+					target_phys_addr_t start_addr,
+					target_phys_addr_t end_addr)
+{
+    return xen_sync_dirty_bitmap(start_addr, end_addr - start_addr);
+}
+
+static int xen_client_migration_log(struct CPUPhysMemoryClient *client,
+				    int enable)
+{
+    return -1;
+}
+
+static CPUPhysMemoryClient xen_cpu_phys_memory_client = {
+    .set_memory = xen_client_set_memory,
+    .sync_dirty_bitmap = xen_client_sync_dirty_bitmap,
+    .migration_log = xen_client_migration_log,
+};
 
 /* VCPU Operations, MMIO, IO ring ... */
 
@@ -552,6 +784,7 @@ int xen_init(int smp_cpus)
     xen_ram_init(ram_size);
 
     qemu_add_vm_change_state_handler(xen_vm_change_state_handler, state);
+    cpu_register_phys_memory_client(&xen_cpu_phys_memory_client);
 
     return 0;
 }
diff --git a/xen-stub.c b/xen-stub.c
index d22f475..f6feee7 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -28,6 +28,17 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size)
 {
 }
 
+int xen_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int xen_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+
 void xen_set_hvm_sleep_state(void)
 {
 }
-- 
1.7.1

[Qemu-devel] Re: [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[Jan Kiszka]

Am 11.01.2011 16:37, anthony.perard@citrix.com wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> This patch introduces phys memory client for Xen.
> 
> Only sync dirty_bitmap and set_memory are actually implemented.
> migration_log will stay empty for the moment.
> 
> Xen can only log one range for bit change, so only the range in the
> first call will be synced.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/vga.c   |    7 ++
>  hw/xen.h   |    2 +
>  xen-all.c  |  233 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  xen-stub.c |   11 +++
>  4 files changed, 253 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index c057f4f..5f7a181 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -29,6 +29,7 @@
>  #include "pixel_ops.h"
>  #include "qemu-timer.h"
>  #include "kvm.h"
> +#include "xen.h"
>  
>  //#define DEBUG_VGA
>  //#define DEBUG_VGA_MEM
> @@ -1599,6 +1600,9 @@ void vga_dirty_log_start(VGACommonState *s)
>  {
>      if (kvm_enabled() && s->map_addr)
>          kvm_log_start(s->map_addr, s->map_end - s->map_addr);
> +    if (xen_enabled() && s->map_addr) {
> +        xen_log_start(s->map_addr, s->map_end - s->map_addr);
> +    }
>  
>      if (kvm_enabled() && s->lfb_vram_mapped) {
>          kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
> @@ -1616,6 +1620,9 @@ void vga_dirty_log_stop(VGACommonState *s)
>  {
>      if (kvm_enabled() && s->map_addr)
>  	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
> +    if (xen_enabled() && s->map_addr) {
> +        xen_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, 0x8000);

This is probably the right time to make dirty_log_start/stop callbacks
in CPUPhysMemoryClient as well. Would remove any KVM or Xen reference
from vga code.

We just need to think about how to deal with the quirks of its users:
KVM requires the isa vram to be reported in two chunks, Xen can't handle
more than one region at all. If Xen is able to filter out those events
it can handle, we could replace kvm_log_start/stop with some
cpu_notify_log_start/stop.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [Xen-devel] [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[Stefano Stabellini]

On Tue, 11 Jan 2011, anthony.perard@citrix.com wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> This patch introduces phys memory client for Xen.
> 
> Only sync dirty_bitmap and set_memory are actually implemented.
> migration_log will stay empty for the moment.
> 
> Xen can only log one range for bit change, so only the range in the
> first call will be synced.

The patch looks much better than I expected, one of the benefits of
reusing the ramblock architecture!

 
> +static XenPhysmap *link_exist(target_phys_addr_t start_addr)
> +{
> +    XenPhysmap *physmap = NULL;
> +
> +    start_addr = TARGET_PAGE_ALIGN(start_addr);
> +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> +        if (physmap->size > 0 && physmap->start_addr == start_addr) {
> +            return physmap;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
> +static int already_physmapped(target_phys_addr_t phys_offset)
> +{
> +    XenPhysmap *physmap = NULL;
> +
> +    phys_offset = TARGET_PAGE_ALIGN(phys_offset);
> +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> +        if (physmap->size > 0 && physmap->phys_offset <= phys_offset &&
> +            phys_offset <= (physmap->phys_offset + physmap->size)) {
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +

why are you searching for an address within a range in
already_physmapped while you are just looking for a simple match in
link_exist? Shouldn't it be the same in both?


> +static int xen_sync_dirty_bitmap(target_phys_addr_t start_addr,
> +                                 ram_addr_t size)
> +{
> +    target_phys_addr_t npages = size >> TARGET_PAGE_BITS;
> +    target_phys_addr_t vram_offset = 0;
> +    const int width = sizeof(unsigned long) * 8;
> +    unsigned long bitmap[(npages + width - 1) / width];
> +    int rc, i, j;
> +    XenPhysmap *physmap = NULL;
> +
> +    physmap = link_exist(start_addr);
> +    if (physmap) {
> +        vram_offset = physmap->phys_offset;
> +    } else {
> +        vram_offset = start_addr;
> +    }

link_exist is always supposed to return a value here anyway, right?
With the current code is not possible to get here without a mapping, I
think.
However if you are trying to make xen_sync_dirty_bitmap more generic
you also need to handle the case in which start_addr is contained within
an already mapped range.

[Qemu-devel] Re: [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[Anthony PERARD]

On Wed, 12 Jan 2011, Jan Kiszka wrote:

> Am 11.01.2011 16:37, anthony.perard@citrix.com wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> >
> > This patch introduces phys memory client for Xen.
> >
> > Only sync dirty_bitmap and set_memory are actually implemented.
> > migration_log will stay empty for the moment.
> >
> > Xen can only log one range for bit change, so only the range in the
> > first call will be synced.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  hw/vga.c   |    7 ++
> >  hw/xen.h   |    2 +
> >  xen-all.c  |  233 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  xen-stub.c |   11 +++
> >  4 files changed, 253 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/vga.c b/hw/vga.c
> > index c057f4f..5f7a181 100644
> > --- a/hw/vga.c
> > +++ b/hw/vga.c
> > @@ -29,6 +29,7 @@
> >  #include "pixel_ops.h"
> >  #include "qemu-timer.h"
> >  #include "kvm.h"
> > +#include "xen.h"
> >
> >  //#define DEBUG_VGA
> >  //#define DEBUG_VGA_MEM
> > @@ -1599,6 +1600,9 @@ void vga_dirty_log_start(VGACommonState *s)
> >  {
> >      if (kvm_enabled() && s->map_addr)
> >          kvm_log_start(s->map_addr, s->map_end - s->map_addr);
> > +    if (xen_enabled() && s->map_addr) {
> > +        xen_log_start(s->map_addr, s->map_end - s->map_addr);
> > +    }
> >
> >      if (kvm_enabled() && s->lfb_vram_mapped) {
> >          kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
> > @@ -1616,6 +1620,9 @@ void vga_dirty_log_stop(VGACommonState *s)
> >  {
> >      if (kvm_enabled() && s->map_addr)
> >  	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
> > +    if (xen_enabled() && s->map_addr) {
> > +        xen_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, 0x8000);
>
> This is probably the right time to make dirty_log_start/stop callbacks
> in CPUPhysMemoryClient as well. Would remove any KVM or Xen reference
> from vga code.
>
> We just need to think about how to deal with the quirks of its users:
> KVM requires the isa vram to be reported in two chunks, Xen can't handle
> more than one region at all. If Xen is able to filter out those events
> it can handle, we could replace kvm_log_start/stop with some
> cpu_notify_log_start/stop.

The first thing I think of, is to handle only the first call of
_log_start.

The other choice is to sync dirty bitmap only when a call to
xc_domain_add_to_physmap is working. This call is done inside the
set_memory client when the flags is IO_MEM_RAM.

-- 
Anthony PERARD

[Qemu-devel] Re: [Xen-devel] [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[Anthony PERARD]

On Wed, 12 Jan 2011, Stefano Stabellini wrote:

> On Tue, 11 Jan 2011, anthony.perard@citrix.com wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> >
> > This patch introduces phys memory client for Xen.
> >
> > Only sync dirty_bitmap and set_memory are actually implemented.
> > migration_log will stay empty for the moment.
> >
> > Xen can only log one range for bit change, so only the range in the
> > first call will be synced.
>
> The patch looks much better than I expected, one of the benefits of
> reusing the ramblock architecture!
>
>
> > +static XenPhysmap *link_exist(target_phys_addr_t start_addr)
> > +{
> > +    XenPhysmap *physmap = NULL;
> > +
> > +    start_addr = TARGET_PAGE_ALIGN(start_addr);
> > +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> > +        if (physmap->size > 0 && physmap->start_addr == start_addr) {
> > +            return physmap;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
> > +static int already_physmapped(target_phys_addr_t phys_offset)
> > +{
> > +    XenPhysmap *physmap = NULL;
> > +
> > +    phys_offset = TARGET_PAGE_ALIGN(phys_offset);
> > +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> > +        if (physmap->size > 0 && physmap->phys_offset <= phys_offset &&
> > +            phys_offset <= (physmap->phys_offset + physmap->size)) {
> > +            return 1;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
>
> why are you searching for an address within a range in
> already_physmapped while you are just looking for a simple match in
> link_exist? Shouldn't it be the same in both?

For link_exist, I want to be sure to remove the same physmapping adds
previously, I should check also the size.

> > +static int xen_sync_dirty_bitmap(target_phys_addr_t start_addr,
> > +                                 ram_addr_t size)
> > +{
> > +    target_phys_addr_t npages = size >> TARGET_PAGE_BITS;
> > +    target_phys_addr_t vram_offset = 0;
> > +    const int width = sizeof(unsigned long) * 8;
> > +    unsigned long bitmap[(npages + width - 1) / width];
> > +    int rc, i, j;
> > +    XenPhysmap *physmap = NULL;
> > +
> > +    physmap = link_exist(start_addr);
> > +    if (physmap) {
> > +        vram_offset = physmap->phys_offset;
> > +    } else {
> > +        vram_offset = start_addr;
> > +    }
>
> link_exist is always supposed to return a value here anyway, right?
> With the current code is not possible to get here without a mapping, I
> think.
> However if you are trying to make xen_sync_dirty_bitmap more generic
> you also need to handle the case in which start_addr is contained within
> an already mapped range.

Actually, before I sent the patch, the second case was an error, and it
worked well. Now, sync_dirty_bitmap is done for two regions, for the
physmapped memory and for the isa memory. :(

But I will add the case when the start_addr is inside the region.

-- 
Anthony PERARD