[PATCH v5 0/4] xen/pvh: enable migration on PVH Dom0

[PATCH v5 0/4] xen/pvh: enable migration on PVH Dom0

[Roger Pau Monne]

Changes in this version include the removal of the red-black tree in order 
to keep track of vmalloced regions, to unmap them a VA -> PA lookup is 
done. Also this version introduces vzalloc.

Thanks, Roger.

[PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Roger Pau Monne]

The allocator uses independent calls to alloc_domheap_pages in order to get
the desired amount of memory and then maps all the independent physical
addresses into a contiguous virtual address space.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Julien Grall <julien.grall@citrix.com> (ARM)
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v4:
 - Remove the red-black tree, the same can be achieved by doing a VA -> MFN
   translation.
 - Provide a vzalloc that zeroes the allocated memory area.
 - Allocate domheap annonymous pages.
---
 xen/common/vmap.c          |  2 +-
 xen/common/xmalloc_tlsf.c  | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/mm.h   |  2 ++
 xen/include/asm-x86/page.h |  2 ++
 xen/include/xen/vmap.h     |  1 +
 xen/include/xen/xmalloc.h  |  4 +++
 6 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 739d468..248ceec 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -146,7 +146,7 @@ static unsigned int vm_index(const void *va)
            test_bit(idx, vm_bitmap) ? idx : 0;
 }
 
-static unsigned int vm_size(const void *va)
+unsigned int vm_size(const void *va)
 {
     unsigned int start = vm_index(va), end;
 
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index b13317e..fe3032e 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -27,6 +27,7 @@
 #include <xen/irq.h>
 #include <xen/mm.h>
 #include <xen/pfn.h>
+#include <xen/vmap.h>
 #include <asm/time.h>
 
 #define MAX_POOL_NAME_LEN       16
@@ -613,6 +614,13 @@ void *_xzalloc(unsigned long size, unsigned long align)
     return p ? memset(p, 0, size) : p;
 }
 
+void *vzalloc(unsigned long size)
+{
+    void *p = vmalloc(size);
+
+    return p ? memset(p, 0, size) : p;
+}
+
 void xfree(void *p)
 {
     struct bhdr *b;
@@ -651,3 +659,57 @@ void xfree(void *p)
 
     xmem_pool_free(p, xenpool);
 }
+
+void *vmalloc(unsigned long size)
+{
+    unsigned long *mfn;
+    unsigned long pages, i;
+    struct page_info *pg;
+    void *va = NULL;
+
+    ASSERT(!in_irq());
+
+    if ( size == 0 )
+        return ZERO_BLOCK_PTR;
+
+    pages = DIV_ROUND_UP(size, PAGE_SIZE);
+    mfn = xzalloc_array(unsigned long, pages);
+    if ( mfn == NULL )
+        return NULL;
+
+    for ( i = 0; i < pages; i++ )
+    {
+        pg = alloc_domheap_pages(NULL, 1, 0);
+        if ( pg == NULL )
+            goto error;
+        mfn[i] = page_to_mfn(pg);
+    }
+
+    va = vmap(mfn, pages);
+    if ( va == NULL )
+        goto error;
+
+    xfree(mfn);
+    return va;
+
+ error:
+    vunmap(va);
+    for ( i = 0; i < pages; i++ )
+        if ( mfn[i] != 0 )
+            free_domheap_pages(mfn_to_page(mfn[i]), 1);
+    xfree(mfn);
+    return NULL;
+}
+
+void vfree(void *va)
+{
+    unsigned int i, pages = vm_size(va);
+
+    if ( pages == 0 )
+        return;
+
+    for ( i = 0; i < pages; i++ )
+        free_domheap_pages(vmap_to_page(va + i * PAGE_SIZE), 1);
+
+    vunmap(va);
+}
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index d25e485..c0afcec 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -208,6 +208,8 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
 #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
+#define vmap_to_mfn(va)     paddr_to_pfn(virt_to_maddr((vaddr_t)va))
+#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
 
 /* Page-align address and convert to frame number format */
 #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index a8bc999..8977a76 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -262,6 +262,8 @@ void copy_page_sse2(void *, const void *);
 #define pfn_to_paddr(pfn)   __pfn_to_paddr(pfn)
 #define paddr_to_pfn(pa)    __paddr_to_pfn(pa)
 #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
+#define vmap_to_mfn(va)     l1e_get_pfn(*virt_to_xen_l1e((unsigned long)(va)))
+#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
 
 #endif /* !defined(__ASSEMBLY__) */
 
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index b1923dd..bc5c862 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -11,6 +11,7 @@ void *__vmap(const unsigned long *mfn, unsigned int granularity,
              unsigned int nr, unsigned int align, unsigned int flags);
 void *vmap(const unsigned long *mfn, unsigned int nr);
 void vunmap(const void *);
+unsigned int vm_size(const void *va);
 
 void __iomem *ioremap(paddr_t, size_t);
 
diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
index 24a99ac..be157bf 100644
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -30,6 +30,10 @@ extern void xfree(void *);
 extern void *_xmalloc(unsigned long size, unsigned long align);
 extern void *_xzalloc(unsigned long size, unsigned long align);
 
+void *vmalloc(unsigned long size);
+void *vzalloc(unsigned long size);
+void vfree(void *va);
+
 static inline void *_xmalloc_array(
     unsigned long size, unsigned long align, unsigned long num)
 {
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Roger Pau Monne]

Modify shadow_track_dirty_vram to use a local buffer and then flush to the
guest without the paging_lock held. This is modeled after
hap_track_dirty_vram.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v4:
 - Use newly introduced vzalloc that zeroes memory.

Changes since v3:
 - Use the newly introduced alloc_xenheap_noncontiguous in order to allocate
   the temporary buffer.

Changes since v2:
 - Remove checks against memcpy, it always returns the address passed as
   dst.
 - Join the nested ifs in the out path.
 - Add Andrew Cooper Reviewed-by.
---
 xen/arch/x86/mm/shadow/common.c | 43 +++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 2e43d6d..cdd5b76 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3516,7 +3516,7 @@ static void sh_clean_dirty_bitmap(struct domain *d)
 int shadow_track_dirty_vram(struct domain *d,
                             unsigned long begin_pfn,
                             unsigned long nr,
-                            XEN_GUEST_HANDLE_64(uint8) dirty_bitmap)
+                            XEN_GUEST_HANDLE_64(uint8) guest_dirty_bitmap)
 {
     int rc;
     unsigned long end_pfn = begin_pfn + nr;
@@ -3526,6 +3526,7 @@ int shadow_track_dirty_vram(struct domain *d,
     p2m_type_t t;
     struct sh_dirty_vram *dirty_vram;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    uint8_t *dirty_bitmap = NULL;
 
     if ( end_pfn < begin_pfn || end_pfn > p2m->max_mapped_pfn + 1 )
         return -EINVAL;
@@ -3554,6 +3555,12 @@ int shadow_track_dirty_vram(struct domain *d,
         goto out;
     }
 
+    dirty_bitmap = vzalloc(dirty_size);
+    if ( dirty_bitmap == NULL )
+    {
+        rc = -ENOMEM;
+        goto out;
+    }
     /* This should happen seldomly (Video mode change),
      * no need to be careful. */
     if ( !dirty_vram )
@@ -3584,12 +3591,8 @@ int shadow_track_dirty_vram(struct domain *d,
         rc = -ENODATA;
     }
     else if (dirty_vram->last_dirty == -1)
-    {
         /* still completely clean, just copy our empty bitmap */
-        rc = -EFAULT;
-        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 )
-            rc = 0;
-    }
+        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
     else
     {
         unsigned long map_mfn = INVALID_MFN;
@@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
         if ( map_sl1p )
             sh_unmap_domain_page(map_sl1p);
 
-        rc = -EFAULT;
-        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
-            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
-            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
+        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
+        memset(dirty_vram->dirty_bitmap, 0, dirty_size);
+        if ( dirty_vram->last_dirty + SECONDS(2) < NOW() )
+        {
+            /* was clean for more than two seconds, try to disable guest
+             * write access */
+            for ( i = begin_pfn; i < end_pfn; i++ )
             {
-                /* was clean for more than two seconds, try to disable guest
-                 * write access */
-                for ( i = begin_pfn; i < end_pfn; i++ ) {
-                    mfn_t mfn = get_gfn_query_unlocked(d, i, &t);
-                    if (mfn_x(mfn) != INVALID_MFN)
-                        flush_tlb |= sh_remove_write_access(d, mfn, 1, 0);
-                }
-                dirty_vram->last_dirty = -1;
+                mfn_t mfn = get_gfn_query_unlocked(d, i, &t);
+                if ( mfn_x(mfn) != INVALID_MFN )
+                    flush_tlb |= sh_remove_write_access(d, mfn, 1, 0);
             }
-            rc = 0;
+            dirty_vram->last_dirty = -1;
         }
     }
     if ( flush_tlb )
@@ -3697,6 +3698,10 @@ out_dirty_vram:
 
 out:
     paging_unlock(d);
+    if ( rc == 0 && dirty_bitmap != NULL &&
+         copy_to_guest(guest_dirty_bitmap, dirty_bitmap, dirty_size) )
+        rc = -EFAULT;
+    vfree(dirty_bitmap);
     p2m_unlock(p2m_get_hostp2m(d));
     return rc;
 }
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v5 3/4] xen/hap: make hap_track_dirty_vram use non-contiguous memory for temporary map

[Roger Pau Monne]

Just like it's done for shadow_track_dirty_vram allocate the temporary
buffer using non-contiguous memory.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v4:
 - Use vzalloc in order to allocate zeroed memory.
---
 xen/arch/x86/mm/hap/hap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index c28a6e3..cea7990 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -87,7 +87,7 @@ int hap_track_dirty_vram(struct domain *d,
         }
 
         rc = -ENOMEM;
-        dirty_bitmap = xzalloc_bytes(size);
+        dirty_bitmap = vzalloc(size);
         if ( !dirty_bitmap )
             goto out;
 
@@ -174,7 +174,7 @@ int hap_track_dirty_vram(struct domain *d,
                                   p2m_ram_logdirty, p2m_ram_rw);
     }
 out:
-    xfree(dirty_bitmap);
+    vfree(dirty_bitmap);
 
     return rc;
 }
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v5 4/4] xen: rework paging_log_dirty_op to work with hvm guests

[Roger Pau Monne]

When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
trying to copy the dirty bitmap to the guest because the paging lock is
already held.

Fix this by independently mapping each page of the guest bitmap as needed
without the paging lock held.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v4:
 - Indent again label.
 - Replace bogus paddr_t cast with proper type.
 - Update preempt.log_dirty before dropping the paging lock.

Changes since v3:
 - Drop last parameter from map_dirty_bitmap.
 - Drop pointless initializers in paging_log_dirty_op.
 - Add a new field to paging_domain in order to copy i2 position.
 - Move the again case up to make sure we don't hold cached values of the
   contents of log_dirty.
 - Replace the BUG_ON in paging_log_dirty_op with an ASSERT.

Changes since v2:
 - Add checks for p2m_is_ram and p2m_is_discard_write when mapping a guest
   page.
 - Remove error checking from memset/memcpy, they unconditionally return
   dst.
---
 xen/arch/x86/mm/paging.c     | 102 ++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/domain.h |   1 +
 2 files changed, 92 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 59d4720..aed68f2 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -408,6 +408,51 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
     return rv;
 }
 
+static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
+                                     unsigned long pages,
+                                     struct page_info **page)
+{
+    uint32_t pfec = PFEC_page_present | PFEC_write_access;
+    unsigned long gfn;
+    p2m_type_t p2mt;
+
+    gfn = paging_gva_to_gfn(current,
+                            (unsigned long)(dirty_bitmap.p + (pages >> 3)),
+                            &pfec);
+    if ( gfn == INVALID_GFN )
+        return NULL;
+
+    *page = get_page_from_gfn(current->domain, gfn, &p2mt, P2M_UNSHARE);
+
+    if ( !p2m_is_ram(p2mt) )
+    {
+        put_page(*page);
+        return NULL;
+    }
+    if ( p2m_is_paging(p2mt) )
+    {
+        put_page(*page);
+        p2m_mem_paging_populate(current->domain, gfn);
+        return NULL;
+    }
+    if ( p2m_is_shared(p2mt) || p2m_is_discard_write(p2mt) )
+    {
+        put_page(*page);
+        return NULL;
+    }
+
+    return __map_domain_page(*page);
+}
+
+static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
+{
+    if ( addr != NULL )
+    {
+        unmap_domain_page(addr);
+        put_page(page);
+    }
+}
+
 
 /* Read a domain's log-dirty bitmap and stats.  If the operation is a CLEAN,
  * clear the bitmap and stats as well. */
@@ -420,6 +465,9 @@ static int paging_log_dirty_op(struct domain *d,
     mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
     unsigned long *l1 = NULL;
     int i4, i3, i2;
+    uint8_t *dirty_bitmap;
+    struct page_info *page;
+    unsigned long index_mapped;
 
     if ( !resuming )
     {
@@ -433,6 +481,14 @@ static int paging_log_dirty_op(struct domain *d,
         p2m_flush_hardware_cached_dirty(d);
     }
 
+    index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
+    dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped, &page);
+    if ( dirty_bitmap == NULL )
+    {
+        domain_unpause(d);
+        return -EFAULT;
+    }
+
     paging_lock(d);
 
     if ( !d->arch.paging.preempt.dom )
@@ -455,6 +511,7 @@ static int paging_log_dirty_op(struct domain *d,
                  d->arch.paging.log_dirty.fault_count,
                  d->arch.paging.log_dirty.dirty_count);
 
+ again:
     sc->stats.fault_count = d->arch.paging.log_dirty.fault_count;
     sc->stats.dirty_count = d->arch.paging.log_dirty.dirty_count;
 
@@ -472,18 +529,18 @@ static int paging_log_dirty_op(struct domain *d,
     l4 = paging_map_log_dirty_bitmap(d);
     i4 = d->arch.paging.preempt.log_dirty.i4;
     i3 = d->arch.paging.preempt.log_dirty.i3;
+    i2 = d->arch.paging.preempt.log_dirty.i2;
     pages = d->arch.paging.preempt.log_dirty.done;
 
     for ( ; (pages < sc->pages) && (i4 < LOGDIRTY_NODE_ENTRIES); i4++, i3 = 0 )
     {
         l3 = (l4 && mfn_valid(l4[i4])) ? map_domain_page(mfn_x(l4[i4])) : NULL;
-        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES); i3++ )
+        for ( ; (pages < sc->pages) && (i3 < LOGDIRTY_NODE_ENTRIES);
+             i3++, i2 = 0 )
         {
             l2 = ((l3 && mfn_valid(l3[i3])) ?
                   map_domain_page(mfn_x(l3[i3])) : NULL);
-            for ( i2 = 0;
-                  (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES);
-                  i2++ )
+            for ( ; (pages < sc->pages) && (i2 < LOGDIRTY_NODE_ENTRIES); i2++ )
             {
                 unsigned int bytes = PAGE_SIZE;
                 l1 = ((l2 && mfn_valid(l2[i2])) ?
@@ -492,15 +549,34 @@ static int paging_log_dirty_op(struct domain *d,
                     bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
                 if ( likely(peek) )
                 {
-                    if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
-                                                    pages >> 3, (uint8_t *)l1,
-                                                    bytes)
-                             : clear_guest_offset(sc->dirty_bitmap,
-                                                  pages >> 3, bytes)) != 0 )
+                    if ( pages >> (3 + PAGE_SHIFT) !=
+                         index_mapped >> (3 + PAGE_SHIFT) )
                     {
-                        rv = -EFAULT;
-                        goto out;
+                        /* We need to map next page */
+                        d->arch.paging.preempt.log_dirty.i4 = i4;
+                        d->arch.paging.preempt.log_dirty.i3 = i3;
+                        d->arch.paging.preempt.log_dirty.i2 = i2;
+                        d->arch.paging.preempt.log_dirty.done = pages;
+                        paging_unlock(d);
+                        unmap_dirty_bitmap(dirty_bitmap, page);
+                        index_mapped = pages;
+                        dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, pages,
+                                                        &page);
+                        paging_lock(d);
+                        if ( dirty_bitmap == NULL )
+                        {
+                            rv = -EFAULT;
+                            goto out;
+                        }
+                        goto again;
                     }
+                    ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <= PAGE_SIZE);
+                    if ( l1 )
+                        memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), l1,
+                               bytes);
+                    else
+                        memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), 0,
+                               bytes);
                 }
                 pages += bytes << 3;
                 if ( l1 )
@@ -517,6 +593,7 @@ static int paging_log_dirty_op(struct domain *d,
             {
                 d->arch.paging.preempt.log_dirty.i4 = i4;
                 d->arch.paging.preempt.log_dirty.i3 = i3 + 1;
+                d->arch.paging.preempt.log_dirty.i2 = 0;
                 rv = -ERESTART;
                 break;
             }
@@ -529,6 +606,7 @@ static int paging_log_dirty_op(struct domain *d,
         {
             d->arch.paging.preempt.log_dirty.i4 = i4 + 1;
             d->arch.paging.preempt.log_dirty.i3 = 0;
+            d->arch.paging.preempt.log_dirty.i2 = 0;
             rv = -ERESTART;
         }
         if ( rv )
@@ -570,12 +648,14 @@ static int paging_log_dirty_op(struct domain *d,
          * paging modes (shadow or hap).  Safe because the domain is paused. */
         d->arch.paging.log_dirty.clean_dirty_bitmap(d);
     }
+    unmap_dirty_bitmap(dirty_bitmap, page);
     domain_unpause(d);
     return rv;
 
  out:
     d->arch.paging.preempt.dom = NULL;
     paging_unlock(d);
+    unmap_dirty_bitmap(dirty_bitmap, page);
     domain_unpause(d);
 
     if ( l1 )
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 3f83e8b..e364a2c 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -202,6 +202,7 @@ struct paging_domain {
                 unsigned long done:PADDR_BITS - PAGE_SHIFT;
                 unsigned long i4:PAGETABLE_ORDER;
                 unsigned long i3:PAGETABLE_ORDER;
+                unsigned long i2:PAGETABLE_ORDER;
             } log_dirty;
         };
     } preempt;
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Andrew Cooper]

On 08/05/15 15:34, Roger Pau Monne wrote:
> The allocator uses independent calls to alloc_domheap_pages in order to get
> the desired amount of memory and then maps all the independent physical
> addresses into a contiguous virtual address space.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Tested-by: Julien Grall <julien.grall@citrix.com> (ARM)
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes since v4:
>  - Remove the red-black tree, the same can be achieved by doing a VA -> MFN
>    translation.
>  - Provide a vzalloc that zeroes the allocated memory area.
>  - Allocate domheap annonymous pages.
> ---
>  xen/common/vmap.c          |  2 +-
>  xen/common/xmalloc_tlsf.c  | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/mm.h   |  2 ++
>  xen/include/asm-x86/page.h |  2 ++
>  xen/include/xen/vmap.h     |  1 +
>  xen/include/xen/xmalloc.h  |  4 +++
>  6 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/vmap.c b/xen/common/vmap.c
> index 739d468..248ceec 100644
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -146,7 +146,7 @@ static unsigned int vm_index(const void *va)
>             test_bit(idx, vm_bitmap) ? idx : 0;
>  }
>  
> -static unsigned int vm_size(const void *va)
> +unsigned int vm_size(const void *va)
>  {
>      unsigned int start = vm_index(va), end;
>  
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index b13317e..fe3032e 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -27,6 +27,7 @@
>  #include <xen/irq.h>
>  #include <xen/mm.h>
>  #include <xen/pfn.h>
> +#include <xen/vmap.h>
>  #include <asm/time.h>
>  
>  #define MAX_POOL_NAME_LEN       16
> @@ -613,6 +614,13 @@ void *_xzalloc(unsigned long size, unsigned long align)
>      return p ? memset(p, 0, size) : p;
>  }
>  
> +void *vzalloc(unsigned long size)
> +{
> +    void *p = vmalloc(size);
> +
> +    return p ? memset(p, 0, size) : p;
> +}
> +
>  void xfree(void *p)
>  {
>      struct bhdr *b;
> @@ -651,3 +659,57 @@ void xfree(void *p)
>  
>      xmem_pool_free(p, xenpool);
>  }
> +
> +void *vmalloc(unsigned long size)
> +{
> +    unsigned long *mfn;
> +    unsigned long pages, i;
> +    struct page_info *pg;
> +    void *va = NULL;
> +
> +    ASSERT(!in_irq());
> +
> +    if ( size == 0 )
> +        return ZERO_BLOCK_PTR;
> +
> +    pages = DIV_ROUND_UP(size, PAGE_SIZE);
> +    mfn = xzalloc_array(unsigned long, pages);

This can avoid the zeroing if...

> +    if ( mfn == NULL )
> +        return NULL;
> +
> +    for ( i = 0; i < pages; i++ )
> +    {
> +        pg = alloc_domheap_pages(NULL, 1, 0);
> +        if ( pg == NULL )
> +            goto error;
> +        mfn[i] = page_to_mfn(pg);
> +    }
> +
> +    va = vmap(mfn, pages);
> +    if ( va == NULL )
> +        goto error;
> +
> +    xfree(mfn);
> +    return va;
> +
> + error:
> +    vunmap(va);
> +    for ( i = 0; i < pages; i++ )
> +        if ( mfn[i] != 0 )
> +            free_domheap_pages(mfn_to_page(mfn[i]), 1);

this loop turns into

while ( i )
    free_domheap_pages(mfn_to_page(mfn[--i], 1);

and even avoids the conditional.

What is the expected max value of pages?  Given that our hypervisor
stacks are quite large, it might be prudent to use C99 variable length
arrays (with an appropriate BUG_ON()) to avoid the xalloc() altogether.

> +    xfree(mfn);
> +    return NULL;
> +}
> +
> +void vfree(void *va)
> +{
> +    unsigned int i, pages = vm_size(va);
> +
> +    if ( pages == 0 )
> +        return;
> +
> +    for ( i = 0; i < pages; i++ )
> +        free_domheap_pages(vmap_to_page(va + i * PAGE_SIZE), 1);
> +
> +    vunmap(va);
> +}
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index d25e485..c0afcec 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -208,6 +208,8 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
>  #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
>  #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> +#define vmap_to_mfn(va)     paddr_to_pfn(virt_to_maddr((vaddr_t)va))
> +#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
>  
>  /* Page-align address and convert to frame number format */
>  #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
> diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
> index a8bc999..8977a76 100644
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -262,6 +262,8 @@ void copy_page_sse2(void *, const void *);
>  #define pfn_to_paddr(pfn)   __pfn_to_paddr(pfn)
>  #define paddr_to_pfn(pa)    __paddr_to_pfn(pa)
>  #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> +#define vmap_to_mfn(va)     l1e_get_pfn(*virt_to_xen_l1e((unsigned long)(va)))
> +#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
>  
>  #endif /* !defined(__ASSEMBLY__) */
>  
> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index b1923dd..bc5c862 100644
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -11,6 +11,7 @@ void *__vmap(const unsigned long *mfn, unsigned int granularity,
>               unsigned int nr, unsigned int align, unsigned int flags);
>  void *vmap(const unsigned long *mfn, unsigned int nr);
>  void vunmap(const void *);
> +unsigned int vm_size(const void *va);
>  
>  void __iomem *ioremap(paddr_t, size_t);
>  
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index 24a99ac..be157bf 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -30,6 +30,10 @@ extern void xfree(void *);
>  extern void *_xmalloc(unsigned long size, unsigned long align);
>  extern void *_xzalloc(unsigned long size, unsigned long align);
>  
> +void *vmalloc(unsigned long size);
> +void *vzalloc(unsigned long size);

I would suggest size_t to for closer consistency with C.

~Andrew

> +void vfree(void *va);
> +
>  static inline void *_xmalloc_array(
>      unsigned long size, unsigned long align, unsigned long num)
>  {


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Jan Beulich]

>>> On 08.05.15 at 16:50, <andrew.cooper3@citrix.com> wrote:
> On 08/05/15 15:34, Roger Pau Monne wrote:
>> +
>> +void *vmalloc(unsigned long size)
>> +{
>> +    unsigned long *mfn;
>> +    unsigned long pages, i;
>> +    struct page_info *pg;
>> +    void *va = NULL;
>> +
>> +    ASSERT(!in_irq());
>> +
>> +    if ( size == 0 )
>> +        return ZERO_BLOCK_PTR;
>> +
>> +    pages = DIV_ROUND_UP(size, PAGE_SIZE);
>> +    mfn = xzalloc_array(unsigned long, pages);
> 
> This can avoid the zeroing if...
> 
>> +    if ( mfn == NULL )
>> +        return NULL;
>> +
>> +    for ( i = 0; i < pages; i++ )
>> +    {
>> +        pg = alloc_domheap_pages(NULL, 1, 0);
>> +        if ( pg == NULL )
>> +            goto error;
>> +        mfn[i] = page_to_mfn(pg);
>> +    }
>> +
>> +    va = vmap(mfn, pages);
>> +    if ( va == NULL )
>> +        goto error;
>> +
>> +    xfree(mfn);
>> +    return va;
>> +
>> + error:
>> +    vunmap(va);
>> +    for ( i = 0; i < pages; i++ )
>> +        if ( mfn[i] != 0 )
>> +            free_domheap_pages(mfn_to_page(mfn[i]), 1);
> 
> this loop turns into
> 
> while ( i )
>     free_domheap_pages(mfn_to_page(mfn[--i], 1);

or (preferably imo)

while ( i-- )
    free_domheap_pages(mfn_to_page(mfn[i], 1);

Jan

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Jan Beulich]

>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -146,7 +146,7 @@ static unsigned int vm_index(const void *va)
>             test_bit(idx, vm_bitmap) ? idx : 0;
>  }
>  
> -static unsigned int vm_size(const void *va)
> +unsigned int vm_size(const void *va)

I was hoping you would avoid this. I really think vmalloc() and vfree()
should go into vmap.c. They certainly don't belong into ...

> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c

... here.

> +void *vmalloc(unsigned long size)
> +{
> +    unsigned long *mfn;
> +    unsigned long pages, i;
> +    struct page_info *pg;
> +    void *va = NULL;
> +
> +    ASSERT(!in_irq());

I don't think you explicitly need this here - the xzalloc_array() below
will catch that case, and the function here doesn't itself depend on
that.

> +    if ( size == 0 )
> +        return ZERO_BLOCK_PTR;

If you really wanted this, vfree() would need to cope. But I think it's
better to simply ASSERT(size) here.

> +    pages = DIV_ROUND_UP(size, PAGE_SIZE);

I think this would better use PFN_UP().

> +    mfn = xzalloc_array(unsigned long, pages);
> +    if ( mfn == NULL )
> +        return NULL;
> +
> +    for ( i = 0; i < pages; i++ )
> +    {
> +        pg = alloc_domheap_pages(NULL, 1, 0);
> +        if ( pg == NULL )
> +            goto error;
> +        mfn[i] = page_to_mfn(pg);
> +    }
> +
> +    va = vmap(mfn, pages);
> +    if ( va == NULL )
> +        goto error;
> +
> +    xfree(mfn);
> +    return va;
> +
> + error:
> +    vunmap(va);

I don't think vunmap() copes with NULL passed into it. But I also
don't see why you call it in the first place - no path leads here
with va != NULL.

> +void vfree(void *va)
> +{
> +    unsigned int i, pages = vm_size(va);
> +
> +    if ( pages == 0 )
> +        return;

This then shouldn't be needed either (or become an ASSERT()).

> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -30,6 +30,10 @@ extern void xfree(void *);
>  extern void *_xmalloc(unsigned long size, unsigned long align);
>  extern void *_xzalloc(unsigned long size, unsigned long align);
>  
> +void *vmalloc(unsigned long size);
> +void *vzalloc(unsigned long size);
> +void vfree(void *va);

And consequently this is the wrong place then too.

Jan

Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Jan Beulich]

>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
> @@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
>          if ( map_sl1p )
>              sh_unmap_domain_page(map_sl1p);
>  
> -        rc = -EFAULT;
> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
> -            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
> -            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
> +        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
> +        memset(dirty_vram->dirty_bitmap, 0, dirty_size);

This is certainly a behavioral change; I'm only uncertain whether it's
acceptable. Previously the memset() was done only when the copying
to guest memory succeeded, while now it happens unconditionally.

Jan

Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Andrew Cooper]

On 08/05/15 16:28, Jan Beulich wrote:
>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>> @@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
>>          if ( map_sl1p )
>>              sh_unmap_domain_page(map_sl1p);
>>  
>> -        rc = -EFAULT;
>> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
>> -            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
>> -            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
>> +        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
>> +        memset(dirty_vram->dirty_bitmap, 0, dirty_size);
> This is certainly a behavioral change; I'm only uncertain whether it's
> acceptable. Previously the memset() was done only when the copying
> to guest memory succeeded, while now it happens unconditionally.

On the one hand, if the toolstack logdirty buffer suffers an EFAULT,
most bets are probably off.

However, it would better if Xen didn't then clobber the dirty bitmap, in
case the toolstack's kernel is doing some particularly funky memory
management which would succeed on a retry.

~Andrew

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Jan Beulich]

>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
> +void vfree(void *va)
> +{
> +    unsigned int i, pages = vm_size(va);
> +
> +    if ( pages == 0 )
> +        return;
> +
> +    for ( i = 0; i < pages; i++ )
> +        free_domheap_pages(vmap_to_page(va + i * PAGE_SIZE), 1);
> +
> +    vunmap(va);
> +}

There actually is another problem here: You must not free the
pages before having torn down the mapping. An option (to
avoid the need for a memory allocation) might be to zap the
present bits (or whatever their equivalent on ARM) in a first
pass (making sure to flush TLBs), free, and then vunmap().

And you would of course better use the single page shortcut
macros of the domheap allocation/free functions.

Jan

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Roger Pau Monné]

Hello,

El 11/05/15 a les 9.42, Jan Beulich ha escrit:
>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>> +void vfree(void *va)
>> +{
>> +    unsigned int i, pages = vm_size(va);
>> +
>> +    if ( pages == 0 )
>> +        return;
>> +
>> +    for ( i = 0; i < pages; i++ )
>> +        free_domheap_pages(vmap_to_page(va + i * PAGE_SIZE), 1);
>> +
>> +    vunmap(va);
>> +}
> 
> There actually is another problem here: You must not free the
> pages before having torn down the mapping. An option (to
> avoid the need for a memory allocation) might be to zap the
> present bits (or whatever their equivalent on ARM) in a first
> pass (making sure to flush TLBs), free, and then vunmap().

The aim of this is to catch reads/writes to those mappings while we are
tearing them down?

I could do that on x86 without problems using the present bit in the
pte, but I'm not sure how to do it on ARM because AFAICT there's no
present bit. The valid bit on ARM looks like an option, but then I don't
know if va_to_par is going to fail because the valid bit is not set.

Roger.

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Jan Beulich]

>>> On 11.05.15 at 11:52, <roger.pau@citrix.com> wrote:
> El 11/05/15 a les 9.42, Jan Beulich ha escrit:
>>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>>> +void vfree(void *va)
>>> +{
>>> +    unsigned int i, pages = vm_size(va);
>>> +
>>> +    if ( pages == 0 )
>>> +        return;
>>> +
>>> +    for ( i = 0; i < pages; i++ )
>>> +        free_domheap_pages(vmap_to_page(va + i * PAGE_SIZE), 1);
>>> +
>>> +    vunmap(va);
>>> +}
>> 
>> There actually is another problem here: You must not free the
>> pages before having torn down the mapping. An option (to
>> avoid the need for a memory allocation) might be to zap the
>> present bits (or whatever their equivalent on ARM) in a first
>> pass (making sure to flush TLBs), free, and then vunmap().
> 
> The aim of this is to catch reads/writes to those mappings while we are
> tearing them down?

No, that wouldn't be worth it. The aim is to avoid cache attribute
conflicts between the to be torn down mapping and possible
immediate re-uses of the pages elsewhere.

Jan

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Roger Pau Monné]

El 11/05/15 a les 12.41, Jan Beulich ha escrit:
>>>> On 11.05.15 at 11:52, <roger.pau@citrix.com> wrote:
>> El 11/05/15 a les 9.42, Jan Beulich ha escrit:
>>>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>>> There actually is another problem here: You must not free the
>>> pages before having torn down the mapping. An option (to
>>> avoid the need for a memory allocation) might be to zap the
>>> present bits (or whatever their equivalent on ARM) in a first
>>> pass (making sure to flush TLBs), free, and then vunmap().
>>
>> The aim of this is to catch reads/writes to those mappings while we are
>> tearing them down?
> 
> No, that wouldn't be worth it. The aim is to avoid cache attribute
> conflicts between the to be torn down mapping and possible
> immediate re-uses of the pages elsewhere.

After looking into this I think that this approach is complex to
implement on ARM. OTOH I've realized we could fetch all pages before
vunmap and add them to a temporary linked list (using pg->list) unless
there's a chance this page might be already member of a list. Then
vunmap and finally free all the pages. Does this sound sensible?

Roger.

Re: [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

[Jan Beulich]

>>> On 11.05.15 at 14:11, <roger.pau@citrix.com> wrote:
> El 11/05/15 a les 12.41, Jan Beulich ha escrit:
>>>>> On 11.05.15 at 11:52, <roger.pau@citrix.com> wrote:
>>> El 11/05/15 a les 9.42, Jan Beulich ha escrit:
>>>>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>>>> There actually is another problem here: You must not free the
>>>> pages before having torn down the mapping. An option (to
>>>> avoid the need for a memory allocation) might be to zap the
>>>> present bits (or whatever their equivalent on ARM) in a first
>>>> pass (making sure to flush TLBs), free, and then vunmap().
>>>
>>> The aim of this is to catch reads/writes to those mappings while we are
>>> tearing them down?
>> 
>> No, that wouldn't be worth it. The aim is to avoid cache attribute
>> conflicts between the to be torn down mapping and possible
>> immediate re-uses of the pages elsewhere.
> 
> After looking into this I think that this approach is complex to
> implement on ARM. OTOH I've realized we could fetch all pages before
> vunmap and add them to a temporary linked list (using pg->list) unless
> there's a chance this page might be already member of a list. Then
> vunmap and finally free all the pages. Does this sound sensible?

Yes, of course - very good thought!

Jan

Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Roger Pau Monné]

El 08/05/15 a les 17.34, Andrew Cooper ha escrit:
> On 08/05/15 16:28, Jan Beulich wrote:
>>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>>> @@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
>>>          if ( map_sl1p )
>>>              sh_unmap_domain_page(map_sl1p);
>>>  
>>> -        rc = -EFAULT;
>>> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
>>> -            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
>>> -            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
>>> +        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
>>> +        memset(dirty_vram->dirty_bitmap, 0, dirty_size);
>> This is certainly a behavioral change; I'm only uncertain whether it's
>> acceptable. Previously the memset() was done only when the copying
>> to guest memory succeeded, while now it happens unconditionally.
> 
> On the one hand, if the toolstack logdirty buffer suffers an EFAULT,
> most bets are probably off.
> 
> However, it would better if Xen didn't then clobber the dirty bitmap, in
> case the toolstack's kernel is doing some particularly funky memory
> management which would succeed on a retry.

A possible workaround to this would be to do acquire the paging_lock
again if copy_to_guest fails and set the dirty_bitmap to 0xff, although
it's not very elegant. Or do an OR of dirty_bitmap and
dirty_vram->dirty_bitmap.

Roger.

Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

[Tim Deegan]

At 15:46 +0200 on 11 May (1431359176), Roger Pau Monné wrote:
> El 08/05/15 a les 17.34, Andrew Cooper ha escrit:
> > On 08/05/15 16:28, Jan Beulich wrote:
> >>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
> >>> @@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
> >>>          if ( map_sl1p )
> >>>              sh_unmap_domain_page(map_sl1p);
> >>>  
> >>> -        rc = -EFAULT;
> >>> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
> >>> -            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
> >>> -            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
> >>> +        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
> >>> +        memset(dirty_vram->dirty_bitmap, 0, dirty_size);
> >> This is certainly a behavioral change; I'm only uncertain whether it's
> >> acceptable. Previously the memset() was done only when the copying
> >> to guest memory succeeded, while now it happens unconditionally.
> > 
> > On the one hand, if the toolstack logdirty buffer suffers an EFAULT,
> > most bets are probably off.
> > 
> > However, it would better if Xen didn't then clobber the dirty bitmap, in
> > case the toolstack's kernel is doing some particularly funky memory
> > management which would succeed on a retry.
> 
> A possible workaround to this would be to do acquire the paging_lock
> again if copy_to_guest fails and set the dirty_bitmap to 0xff, although
> it's not very elegant.

Equivalently, you could set a bit that makes the _next_ pass over the
bitmap return all 0xff.  IIRC, this is what happens when the vram area
changes.  I think it's a reasobnable thing to do if the toolstack
messed up its hypercall buffers.

Tim.