[PATCH v2 0/4] xen: enable EPT A/D bit feature

[PATCH v2 0/4] xen: enable EPT A/D bit feature

[Xudong Hao]

Changes from v1:
- Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to patch2.
- define them as bool_t instead of int.

Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
enable VMMs to efficiently implement memory management and page classification
algorithms to optimize VM memory operations.

This series of patches enable EPT dirty bit feature for guest live migration.

PATCH 1/4: Add EPT A/D bits definitions.

PATCH 2/4: Add xen parameter to control A/D bits support, it on by default.

PATCH 3/4: Introduce a log_dirty new function update_dirty_bitmap, which will 
only update the log dirty bitmap, but won't clear the EPT page dirty bit. 
The function is used by live migration peek round with EPT D bit supported.

PATCH 4/4: enable EPT dirty bit for guest live migration.

[PATCH v2 1/4] xen: Add EPT A/D bits definitions

[Xudong Hao]

Add EPT A/D bits definitions.

Signed-off-by: Haitao Shan<haitao.shan@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>
---
 xen/include/asm-x86/hvm/vmx/vmcs.h |    4 +++-
 xen/include/asm-x86/hvm/vmx/vmx.h  |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 6100619..c0b9a44 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -62,7 +62,8 @@ struct vmx_domain {
         struct {
             u64 ept_mt :3,
                 ept_wl :3,
-                rsvd   :6,
+                ept_ad :1,
+                rsvd   :5,
                 asr    :52;
         };
         u64 eptp;
@@ -194,6 +195,7 @@ extern bool_t cpu_has_vmx_ins_outs_instr_info;
 #define VMX_EPT_SUPERPAGE_2MB                   0x00010000
 #define VMX_EPT_SUPERPAGE_1GB                   0x00020000
 #define VMX_EPT_INVEPT_INSTRUCTION              0x00100000
+#define VMX_EPT_AD_BITS_SUPPORT                 0x00200000
 #define VMX_EPT_INVEPT_SINGLE_CONTEXT           0x02000000
 #define VMX_EPT_INVEPT_ALL_CONTEXT              0x04000000
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index accfa3f..416504f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -37,7 +37,8 @@ typedef union {
         emt         :   3,  /* bits 5:3 - EPT Memory type */
         ipat        :   1,  /* bit 6 - Ignore PAT memory type */
         sp          :   1,  /* bit 7 - Is this a superpage? */
-        rsvd1       :   2,  /* bits 9:8 - Reserved for future use */
+        a           :   1,  /* bit 8 - Access bit */
+        d           :   1,  /* bit 9 - Dirty bit */
         avail1      :   1,  /* bit 10 - Software available 1 */
         rsvd2_snp   :   1,  /* bit 11 - Used for VT-d snoop control
                                in shared EPT/VT-d usage */
-- 
1.5.5

[PATCH v2 2/4] xen: add xen parameter to control A/D bits support

[Xudong Hao]

Add xen parameter to control A/D bits support, it on by default.

Changes from v1:
- Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to patch2.
- define them as bool_t instead of int.

Signed-off-by: Haitao Shan<haitao.shan@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c       |    1 +
 xen/arch/x86/hvm/vmx/vmx.c        |    8 ++++++++
 xen/arch/x86/mm/hap/hap.c         |    4 ++++
 xen/arch/x86/mm/p2m.c             |    3 +++
 xen/include/asm-x86/hap.h         |    3 +++
 xen/include/asm-x86/hvm/vmx/vmx.h |    3 +++
 6 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 38b5d03..5a6be4c 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -85,6 +85,7 @@ static void __init vmx_display_features(void)
     P(cpu_has_vmx_virtualize_apic_accesses, "APIC MMIO access virtualisation");
     P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
     P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
+    P(cpu_has_vmx_ept_ad_bits, "EPT A/D Bits");
     P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
     P(cpu_has_vmx_vnmi, "Virtual NMI");
     P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index ffb86c1..cb94226 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -37,6 +37,7 @@
 #include <asm/spinlock.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
+#include <asm/hap.h>
 #include <asm/mem_sharing.h>
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/hvm.h>
@@ -89,6 +90,10 @@ static int vmx_domain_initialise(struct domain *d)
     d->arch.hvm_domain.vmx.ept_control.asr  =
         pagetable_get_pfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
 
+    /* set EPT access and dirty bits support */
+    d->arch.hvm_domain.vmx.ept_control.ept_ad =
+        cpu_has_vmx_ept_ad_bits? 1 : 0;
+
     if ( !zalloc_cpumask_var(&d->arch.hvm_domain.vmx.ept_synced) )
         return -ENOMEM;
 
@@ -1574,6 +1579,9 @@ struct hvm_function_table * __init start_vmx(void)
         if ( cpu_has_vmx_ept_1gb )
             vmx_function_table.hap_capabilities |= HVM_HAP_SUPERPAGE_1GB;
 
+        if ( cpu_has_vmx_ept_ad_bits )
+            hap_has_access_bit = hap_has_dirty_bit = 1;
+
         setup_ept_dump();
     }
 
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 13b4be2..8790c58 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -52,6 +52,10 @@
 #undef page_to_mfn
 #define page_to_mfn(_pg) _mfn(__page_to_mfn(_pg))
 
+/* Define whether HW has access and dirty bits seperately */
+bool_t hap_has_dirty_bit __read_mostly = 0;
+bool_t hap_has_access_bit __read_mostly = 0;
+
 /************************************************/
 /*          HAP VRAM TRACKING SUPPORT           */
 /************************************************/
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3cdc417..0a796f3 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -46,6 +46,9 @@ boolean_param("hap_1gb", opt_hap_1gb);
 bool_t __read_mostly opt_hap_2mb = 1;
 boolean_param("hap_2mb", opt_hap_2mb);
 
+bool_t __read_mostly opt_hap_ad_bits = 1;
+boolean_param("hap_ad_bits", opt_hap_ad_bits);
+
 /* Printouts */
 #define P2M_PRINTK(_f, _a...)                                \
     debugtrace_printk("p2m: %s(): " _f, __func__, ##_a)
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index a2532a4..bd5d732 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -64,6 +64,9 @@ int   hap_track_dirty_vram(struct domain *d,
 
 extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
 
+extern bool_t hap_has_dirty_bit __read_mostly;
+extern bool_t hap_has_access_bit __read_mostly;
+
 #endif /* XEN_HAP_H */
 
 /*
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 416504f..f552d08 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -201,6 +201,9 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_2MB)
 #define cpu_has_vmx_ept_invept_single_context   \
     (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
+extern bool_t opt_hap_ad_bits;
+#define cpu_has_vmx_ept_ad_bits                 \
+    ( opt_hap_ad_bits ? (vmx_ept_vpid_cap & VMX_EPT_AD_BITS_SUPPORT) : 0 )
 
 #define EPT_2MB_SHIFT     16
 #define EPT_1GB_SHIFT     17
-- 
1.5.5

[PATCH v2 3/4] xen: introduce new function update_dirty_bitmap

[Xudong Hao]

Introduce log_dirty new function update_dirty_bitmap, which will only update the
log dirty bitmap, but won't clear the EPT page dirty bit. The function is used
by live migration peek round with EPT D bit supported.

Set correct p2m type when EPT dirty bit supported.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Haitao Shan<haitao.shan@intel.com>
---
 xen/arch/x86/mm/hap/hap.c       |   32 +++++++++++++++++++++++++++-----
 xen/arch/x86/mm/p2m-pt.c        |    1 +
 xen/arch/x86/mm/p2m.c           |    9 +++++++++
 xen/arch/x86/mm/paging.c        |   32 +++++++++++++++++++-------------
 xen/arch/x86/mm/shadow/common.c |    2 +-
 xen/include/asm-x86/domain.h    |    1 +
 xen/include/asm-x86/hap.h       |    3 +++
 xen/include/asm-x86/p2m.h       |    5 ++++-
 xen/include/asm-x86/paging.h    |    3 ++-
 9 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 8790c58..2a4cf1d 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -76,6 +76,7 @@ static int hap_enable_vram_tracking(struct domain *d)
     p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
                           p2m_ram_rw, p2m_ram_logdirty);
 
+    /* A TLB flush is needed no matter whether hap dirty bit is supported */
     flush_tlb_mask(d->domain_dirty_cpumask);
     return 0;
 }
@@ -83,19 +84,31 @@ static int hap_enable_vram_tracking(struct domain *d)
 static int hap_disable_vram_tracking(struct domain *d)
 {
     struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
+    p2m_type_t p2mt = p2m_ram_rw;
 
     if ( !dirty_vram )
         return -EINVAL;
 
+    /* With hap dirty bit, p2m type cannot be changed from p2m_ram_logdirty
+     * to p2m_ram_rw when first fault is met. Actually, there is no such
+     * fault occurred.
+     */
+    if ( hap_has_dirty_bit )
+        p2mt = p2m_ram_logdirty;
+
     paging_lock(d);
     d->arch.paging.mode &= ~PG_log_dirty;
     paging_unlock(d);
 
     /* set l1e entries of P2M table with normal mode */
     p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
-                          p2m_ram_logdirty, p2m_ram_rw);
+                          p2mt, p2m_ram_rw);
 
-    flush_tlb_mask(d->domain_dirty_cpumask);
+    /* With hap dirty bit, we actually did not change HW sensitive bits
+     * of the P2M tables.
+     */
+    if ( !hap_has_dirty_bit )
+        flush_tlb_mask(d->domain_dirty_cpumask);
     return 0;
 }
 
@@ -117,7 +130,7 @@ static void hap_vram_tracking_init(struct domain *d)
 {
     paging_log_dirty_init(d, hap_enable_vram_tracking,
                           hap_disable_vram_tracking,
-                          hap_clean_vram_tracking);
+                          hap_clean_vram_tracking, NULL);
 }
 
 int hap_track_dirty_vram(struct domain *d,
@@ -220,8 +233,16 @@ static int hap_disable_log_dirty(struct domain *d)
 
 static void hap_clean_dirty_bitmap(struct domain *d)
 {
+    p2m_type_t p2mt = (hap_has_dirty_bit)? p2m_ram_logdirty : p2m_ram_rw;
     /* set l1e entries of P2M table to be read-only. */
-    p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
+    p2m_change_entry_type_global(d, p2mt, p2m_ram_logdirty);
+    flush_tlb_mask(d->domain_dirty_cpumask);
+}
+
+static void hap_update_dirty_bitmap(struct domain *d)
+{
+    /* find out dirty page by walking EPT table and update dirty bitmap. */
+    p2m_query_entry_global(d, WALK_EPT_D);
     flush_tlb_mask(d->domain_dirty_cpumask);
 }
 
@@ -238,7 +259,8 @@ void hap_logdirty_init(struct domain *d)
     /* Reinitialize logdirty mechanism */
     paging_log_dirty_init(d, hap_enable_log_dirty,
                           hap_disable_log_dirty,
-                          hap_clean_dirty_bitmap);
+                          hap_clean_dirty_bitmap,
+                          hap_update_dirty_bitmap);
 }
 
 /************************************************/
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index c97cac4..7334167 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -1141,6 +1141,7 @@ void p2m_pt_init(struct p2m_domain *p2m)
     p2m->set_entry = p2m_set_entry;
     p2m->get_entry = p2m_gfn_to_mfn;
     p2m->change_entry_type_global = p2m_change_type_global;
+    p2m->query_entry_global = NULL;
     p2m->write_p2m_entry = paging_write_p2m_entry;
 #if P2M_AUDIT
     p2m->audit_p2m = p2m_pt_audit_p2m;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0a796f3..26b1ca7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -148,6 +148,15 @@ void p2m_change_entry_type_global(struct domain *d,
     p2m_unlock(p2m);
 }
 
+void p2m_query_entry_global(struct domain *d, int mask)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    ASSERT(p2m);
+    p2m_lock(p2m);
+    p2m->query_entry_global(p2m, mask);
+    p2m_unlock(p2m);
+}
+
 mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
                     p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
                     unsigned int *page_order, bool_t locked)
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index ca879f9..12ee552 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -335,9 +335,24 @@ int paging_log_dirty_op(struct domain *d, struct xen_domctl_shadow_op *sc)
     int i4, i3, i2;
 
     domain_pause(d);
-    paging_lock(d);
 
     clean = (sc->op == XEN_DOMCTL_SHADOW_OP_CLEAN);
+    if ( clean )
+    {
+        /* We need to further call clean_dirty_bitmap() functions of specific
+         * paging modes (shadow or hap).  Safe because the domain is paused.
+         * And this call must be made before actually transferring the dirty
+         * bitmap since with HW hap dirty bit support, dirty bitmap is
+         * produced by hooking on this call. */
+        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
+    }
+
+    if ( peek && d->arch.paging.log_dirty.update_dirty_bitmap)
+    {
+        d->arch.paging.log_dirty.update_dirty_bitmap(d);
+    }
+
+    paging_lock(d);
 
     PAGING_DEBUG(LOGDIRTY, "log-dirty %s: dom %u faults=%u dirty=%u\n",
                  (clean) ? "clean" : "peek",
@@ -420,17 +435,6 @@ int paging_log_dirty_op(struct domain *d, struct xen_domctl_shadow_op *sc)
     if ( pages < sc->pages )
         sc->pages = pages;
 
-    paging_unlock(d);
-
-    if ( clean )
-    {
-        /* We need to further call clean_dirty_bitmap() functions of specific
-         * paging modes (shadow or hap).  Safe because the domain is paused. */
-        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
-    }
-    domain_unpause(d);
-    return rv;
-
  out:
     paging_unlock(d);
     domain_unpause(d);
@@ -600,11 +604,13 @@ int paging_log_dirty_range(struct domain *d,
 void paging_log_dirty_init(struct domain *d,
                            int    (*enable_log_dirty)(struct domain *d),
                            int    (*disable_log_dirty)(struct domain *d),
-                           void   (*clean_dirty_bitmap)(struct domain *d))
+                           void   (*clean_dirty_bitmap)(struct domain *d),
+                           void   (*update_dirty_bitmap)(struct domain *d))
 {
     d->arch.paging.log_dirty.enable_log_dirty = enable_log_dirty;
     d->arch.paging.log_dirty.disable_log_dirty = disable_log_dirty;
     d->arch.paging.log_dirty.clean_dirty_bitmap = clean_dirty_bitmap;
+    d->arch.paging.log_dirty.update_dirty_bitmap = update_dirty_bitmap;
 }
 
 /* This function fress log dirty bitmap resources. */
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index dc245be..f4e7566 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -50,7 +50,7 @@ void shadow_domain_init(struct domain *d, unsigned int domcr_flags)
 
     /* Use shadow pagetables for log-dirty support */
     paging_log_dirty_init(d, shadow_enable_log_dirty, 
-                          shadow_disable_log_dirty, shadow_clean_dirty_bitmap);
+                          shadow_disable_log_dirty, shadow_clean_dirty_bitmap, NULL);
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     d->arch.paging.shadow.oos_active = 0;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index aecee68..22cd0f9 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -179,6 +179,7 @@ struct log_dirty_domain {
     int            (*enable_log_dirty   )(struct domain *d);
     int            (*disable_log_dirty  )(struct domain *d);
     void           (*clean_dirty_bitmap )(struct domain *d);
+    void           (*update_dirty_bitmap )(struct domain *d);
 };
 
 struct paging_domain {
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index bd5d732..fb43b27 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -31,6 +31,9 @@
 #define HAP_ERROR(_f, _a...)                                          \
     printk("hap error: %s(): " _f, __func__, ##_a)
 
+#define WALK_EPT_UNUSED    0
+#define WALK_EPT_D         2
+
 /************************************************/
 /*          hap domain page mapping             */
 /************************************************/
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 63bc7cf..21ed9a5 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -249,7 +249,7 @@ struct p2m_domain {
     void               (*change_entry_type_global)(struct p2m_domain *p2m,
                                                    p2m_type_t ot,
                                                    p2m_type_t nt);
-    
+    void               (*query_entry_global)(struct p2m_domain *p2m, int mask);
     void               (*write_p2m_entry)(struct p2m_domain *p2m,
                                           unsigned long gfn, l1_pgentry_t *p,
                                           mfn_t table_mfn, l1_pgentry_t new,
@@ -506,6 +506,9 @@ int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
 void p2m_change_entry_type_global(struct domain *d, 
                                   p2m_type_t ot, p2m_type_t nt);
 
+/* Query across all p2m entries in a domain */
+void p2m_query_entry_global(struct domain *d, int mask);
+
 /* Change types across a range of p2m entries (start ... end-1) */
 void p2m_change_type_range(struct domain *d, 
                            unsigned long start, unsigned long end,
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index c432a97..9d95e51 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -160,7 +160,8 @@ int paging_log_dirty_disable(struct domain *d);
 void paging_log_dirty_init(struct domain *d,
                            int  (*enable_log_dirty)(struct domain *d),
                            int  (*disable_log_dirty)(struct domain *d),
-                           void (*clean_dirty_bitmap)(struct domain *d));
+                           void (*clean_dirty_bitmap)(struct domain *d),
+                           void (*update_dirty_bitmap)(struct domain *d));
 
 /* mark a page as dirty */
 void paging_mark_dirty(struct domain *d, unsigned long guest_mfn);
-- 
1.5.5

Re: [PATCH v2 3/4] xen: introduce new function update_dirty_bitmap

[Tim Deegan]

At 09:57 +0800 on 20 Jun (1340186266), Xudong Hao wrote:
> @@ -83,19 +84,31 @@ static int hap_enable_vram_tracking(struct domain *d)
>  static int hap_disable_vram_tracking(struct domain *d)
>  {
>      struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
> +    p2m_type_t p2mt = p2m_ram_rw;
>  
>      if ( !dirty_vram )
>          return -EINVAL;
>  
> +    /* With hap dirty bit, p2m type cannot be changed from p2m_ram_logdirty
> +     * to p2m_ram_rw when first fault is met. Actually, there is no such
> +     * fault occurred.
> +     */
> +    if ( hap_has_dirty_bit )
> +        p2mt = p2m_ram_logdirty;
> +
>      paging_lock(d);
>      d->arch.paging.mode &= ~PG_log_dirty;
>      paging_unlock(d);
>  
>      /* set l1e entries of P2M table with normal mode */
>      p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
> -                          p2m_ram_logdirty, p2m_ram_rw);
> +                          p2mt, p2m_ram_rw);

What's the intent of this change?  

AFAICS it will break the hap_has_dirty_bit==0 case, by basically making
that p2m_change_type_range() into a noop.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -148,6 +148,15 @@ void p2m_change_entry_type_global(struct domain *d,
>      p2m_unlock(p2m);
>  }
>  
> +void p2m_query_entry_global(struct domain *d, int mask)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    ASSERT(p2m);
> +    p2m_lock(p2m);
> +    p2m->query_entry_global(p2m, mask);
> +    p2m_unlock(p2m);
> +}
> +

Given that when you implement this, it's basically just the same as
p2m_change_type_global() with p2m_ram_logdirty for both types:

 - Why not just use p2m_change_type_global() instead of adding the new
   function pointer?
 - If you do need a new function pointer, this name is very confusing:
   it's not doing any kind of query.

> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -335,9 +335,24 @@ int paging_log_dirty_op(struct domain *d, struct xen_domctl_shadow_op *sc)
>      int i4, i3, i2;
>  
>      domain_pause(d);
> -    paging_lock(d);
>  
>      clean = (sc->op == XEN_DOMCTL_SHADOW_OP_CLEAN);
> +    if ( clean )
> +    {
> +        /* We need to further call clean_dirty_bitmap() functions of specific
> +         * paging modes (shadow or hap).  Safe because the domain is paused.
> +         * And this call must be made before actually transferring the dirty
> +         * bitmap since with HW hap dirty bit support, dirty bitmap is
> +         * produced by hooking on this call. */
> +        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> +    }

> +    if ( peek && d->arch.paging.log_dirty.update_dirty_bitmap)
> +    {
> +        d->arch.paging.log_dirty.update_dirty_bitmap(d);
> +    }

Doesn't this end up doing _two_ passes over the p2m table?

I think it would be better to leave the clean() op at the end of the
function, and arrange for it to be a noop in the EPT/D-bit case.  Then
with the addition of this update_dirty_bitmap() call, the p2m only gets
walked once, for all configurations.

Cheers,

Tim.

Re: [PATCH v2 3/4] xen: introduce new function update_dirty_bitmap

[Hao, Xudong]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, June 28, 2012 9:42 PM
> To: Hao, Xudong
> Cc: xen-devel@lists.xen.org; Shan, Haitao; keir@xen.org; Zhang, Xiantao;
> JBeulich@suse.com
> Subject: Re: [Xen-devel] [PATCH v2 3/4] xen: introduce new function
> update_dirty_bitmap
> 
> At 09:57 +0800 on 20 Jun (1340186266), Xudong Hao wrote:
> > @@ -83,19 +84,31 @@ static int hap_enable_vram_tracking(struct domain
> *d)
> >  static int hap_disable_vram_tracking(struct domain *d)
> >  {
> >      struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
> > +    p2m_type_t p2mt = p2m_ram_rw;
> >
> >      if ( !dirty_vram )
> >          return -EINVAL;
> >
> > +    /* With hap dirty bit, p2m type cannot be changed from
> p2m_ram_logdirty
> > +     * to p2m_ram_rw when first fault is met. Actually, there is no such
> > +     * fault occurred.
> > +     */
> > +    if ( hap_has_dirty_bit )
> > +        p2mt = p2m_ram_logdirty;
> > +
> >      paging_lock(d);
> >      d->arch.paging.mode &= ~PG_log_dirty;
> >      paging_unlock(d);
> >
> >      /* set l1e entries of P2M table with normal mode */
> >      p2m_change_type_range(d, dirty_vram->begin_pfn,
> dirty_vram->end_pfn,
> > -                          p2m_ram_logdirty, p2m_ram_rw);
> > +                          p2mt, p2m_ram_rw);
> 
> What's the intent of this change?
> 
> AFAICS it will break the hap_has_dirty_bit==0 case, by basically making
> that p2m_change_type_range() into a noop.
> 

Thanks for the review, it's a code mistake indeed.
It should be: 
      p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
-                          p2m_ram_logdirty, p2m_ram_rw);
+                          p2m_ram_logdirty, p2mt);


> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -148,6 +148,15 @@ void p2m_change_entry_type_global(struct domain
> *d,
> >      p2m_unlock(p2m);
> >  }
> >
> > +void p2m_query_entry_global(struct domain *d, int mask)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    ASSERT(p2m);
> > +    p2m_lock(p2m);
> > +    p2m->query_entry_global(p2m, mask);
> > +    p2m_unlock(p2m);
> > +}
> > +
> 
> Given that when you implement this, it's basically just the same as
> p2m_change_type_global() with p2m_ram_logdirty for both types:
> 
>  - Why not just use p2m_change_type_global() instead of adding the new
>    function pointer?

Because it's need to add a mask to indentify D bit or not, I do not want to change current ept_change_entry_type_global() interface. So add a new function pointer.

>  - If you do need a new function pointer, this name is very confusing:
>    it's not doing any kind of query.
> 

The new function query dirty page in whole p2m table and update the dirty bitmap. 

> > --- a/xen/arch/x86/mm/paging.c
> > +++ b/xen/arch/x86/mm/paging.c
> > @@ -335,9 +335,24 @@ int paging_log_dirty_op(struct domain *d, struct
> xen_domctl_shadow_op *sc)
> >      int i4, i3, i2;
> >
> >      domain_pause(d);
> > -    paging_lock(d);
> >
> >      clean = (sc->op == XEN_DOMCTL_SHADOW_OP_CLEAN);
> > +    if ( clean )
> > +    {
> > +        /* We need to further call clean_dirty_bitmap() functions of
> specific
> > +         * paging modes (shadow or hap).  Safe because the domain is
> paused.
> > +         * And this call must be made before actually transferring the
> dirty
> > +         * bitmap since with HW hap dirty bit support, dirty bitmap is
> > +         * produced by hooking on this call. */
> > +        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> > +    }
> 
> > +    if ( peek && d->arch.paging.log_dirty.update_dirty_bitmap)
> > +    {
> > +        d->arch.paging.log_dirty.update_dirty_bitmap(d);
> > +    }
> 
> Doesn't this end up doing _two_ passes over the p2m table?
> 

Oh, it does 2 passes over the p2m table. I wanted to differentiate the CLEAN and PEEK op by variable "clean" and "peek", seems peek=1 even when it's a CLEAN hapercall.
I do the following modification: 
+    if ( (sc->op == XEN_DOMCTL_SHADOW_OP_PEEK) && d->arch.paging.log_dirty.update_dirty_bitmap)
+    {
+        d->arch.paging.log_dirty.update_dirty_bitmap(d);
+    }

> I think it would be better to leave the clean() op at the end of the
> function, and arrange for it to be a noop in the EPT/D-bit case.  Then

We need to clean D bit in clean() op for the EPT/D bit case. How about the modification above?
 
> with the addition of this update_dirty_bitmap() call, the p2m only gets
> walked once, for all configurations.
> 
> Cheers,
> 
> Tim.

[PATCH v2 4/4] xen: enable EPT dirty bit for guest live migration

[Xudong Hao]

When p2m type is p2m_ram_logdirty, page should be read/write/execute(d) with EPT
dirty bit support.

When guest live migration with EPT dirty bit support, it does not trigger EPT
violation any longer, we flush dirty bitmap in ept_change_entry_type_page()
function, by walking the EPT page table.

Signed-off-by: Haitao Shan<haitao.shan@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>
---
 xen/arch/x86/mm/p2m-ept.c |   50 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f373905..e07004d 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -24,6 +24,7 @@
 #include <asm/types.h>
 #include <asm/domain.h>
 #include <asm/p2m.h>
+#include <asm/hap.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/vmx/vmcs.h>
 #include <xen/iommu.h>
@@ -69,6 +70,11 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces
                                                     entry->mfn);
             break;
         case p2m_ram_logdirty:
+            entry->w = hap_has_dirty_bit;
+            entry->r = entry->x = 1;
+            /* Not necessarily need to clear A bit, but it is safe anyway */
+            entry->a = entry->d = 0;
+            break;
         case p2m_ram_ro:
         case p2m_ram_shared:
             entry->r = entry->x = 1;
@@ -373,6 +379,9 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
                 need_modify_vtd_table = 0;
 
             ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
+
+            if ( old_entry.d && (old_entry.sa_p2mt == p2m_ram_logdirty) )
+                paging_mark_dirty(d, mfn_x(mfn));
         }
 
         atomic_write_ept_entry(ept_entry, new_entry);
@@ -749,7 +758,8 @@ void ept_change_entry_emt_with_range(struct domain *d,
  * quickly enable or diable log-dirty tracking
  */
 static void ept_change_entry_type_page(mfn_t ept_page_mfn, int ept_page_level,
-                                       p2m_type_t ot, p2m_type_t nt)
+                                       p2m_type_t ot, p2m_type_t nt,
+                                       struct domain *d, int mask)
 {
     ept_entry_t e, *epte = map_domain_page(mfn_x(ept_page_mfn));
 
@@ -760,15 +770,33 @@ static void ept_change_entry_type_page(mfn_t ept_page_mfn, int ept_page_level,
 
         if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
             ept_change_entry_type_page(_mfn(epte[i].mfn),
-                                       ept_page_level - 1, ot, nt);
+                                       ept_page_level - 1, ot, nt, d, mask);
         else
         {
             e = atomic_read_ept_entry(&epte[i]);
             if ( e.sa_p2mt != ot )
                 continue;
 
+            if ( e.d && (e.sa_p2mt == p2m_ram_logdirty) )
+            {
+                int j, nr_pages;
+                struct p2m_domain *p2m = p2m_get_hostp2m(d);
+                for ( j = 0, nr_pages = 1; j < ept_page_level;
+                      j++, nr_pages *= 512 ) {}
+                for ( j = 0; j < nr_pages; j++ )
+                    paging_mark_dirty(d, e.mfn + j);
+
+                /* split super page to 4k page, so that dirty bitmap can 
+                 * map the dirty page
+                 */
+                if ( !ept_split_super_page(p2m, &e, ept_page_level, 0) )
+                    continue;
+                atomic_write_ept_entry(&epte[i], e);
+            }
             e.sa_p2mt = nt;
             ept_p2m_type_to_flags(&e, nt, e.access);
+            if (!mask)
+                e.a = e.d = 0;
             atomic_write_ept_entry(&epte[i], e);
         }
     }
@@ -786,7 +814,22 @@ static void ept_change_entry_type_global(struct p2m_domain *p2m,
     BUG_ON(p2m_is_grant(ot) || p2m_is_grant(nt));
     BUG_ON(ot != nt && (ot == p2m_mmio_direct || nt == p2m_mmio_direct));
 
-    ept_change_entry_type_page(_mfn(ept_get_asr(d)), ept_get_wl(d), ot, nt);
+    ept_change_entry_type_page(_mfn(ept_get_asr(d)), ept_get_wl(d),
+                               ot, nt, p2m->domain, WALK_EPT_UNUSED);
+
+    ept_sync_domain(d);
+}
+
+static void ept_query_entry_global(struct p2m_domain *p2m, int mask)
+{
+    struct domain *d = p2m->domain;
+    p2m_type_t ot = p2m_ram_logdirty;
+    p2m_type_t nt = p2m_ram_logdirty;
+    if ( ept_get_asr(d) == 0 )
+        return;
+
+    ept_change_entry_type_page(_mfn(ept_get_asr(d)), ept_get_wl(d),
+                               ot, nt, p2m->domain, mask);
 
     ept_sync_domain(d);
 }
@@ -796,6 +839,7 @@ void ept_p2m_init(struct p2m_domain *p2m)
     p2m->set_entry = ept_set_entry;
     p2m->get_entry = ept_get_entry;
     p2m->change_entry_type_global = ept_change_entry_type_global;
+    p2m->query_entry_global = ept_query_entry_global;
     p2m->audit_p2m = NULL;
 }
 
-- 
1.5.5

Re: [PATCH v2 4/4] xen: enable EPT dirty bit for guest live migration

[Tim Deegan]

At 09:57 +0800 on 20 Jun (1340186267), Xudong Hao wrote:
> @@ -69,6 +70,11 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces
>                                                      entry->mfn);
>              break;
>          case p2m_ram_logdirty:
> +            entry->w = hap_has_dirty_bit;
> +            entry->r = entry->x = 1;
> +            /* Not necessarily need to clear A bit, but it is safe anyway */
> +            entry->a = entry->d = 0;
> +            break;

I think it's better not to clear the A bit - it will just cause the
hardware to issue extra writes to set it.

> @@ -760,15 +770,33 @@ static void ept_change_entry_type_page(mfn_t ept_page_mfn, int ept_page_level,
>  
>          if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
>              ept_change_entry_type_page(_mfn(epte[i].mfn),
> -                                       ept_page_level - 1, ot, nt);
> +                                       ept_page_level - 1, ot, nt, d, mask);
>          else
>          {
>              e = atomic_read_ept_entry(&epte[i]);
>              if ( e.sa_p2mt != ot )
>                  continue;
>  
> +            if ( e.d && (e.sa_p2mt == p2m_ram_logdirty) )
> +            {
> +                int j, nr_pages;
> +                struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +                for ( j = 0, nr_pages = 1; j < ept_page_level;
> +                      j++, nr_pages *= 512 ) {}
> +                for ( j = 0; j < nr_pages; j++ )
> +                    paging_mark_dirty(d, e.mfn + j);
> +
> +                /* split super page to 4k page, so that dirty bitmap can 
> +                 * map the dirty page
> +                 */
> +                if ( !ept_split_super_page(p2m, &e, ept_page_level, 0) )
> +                    continue;

AIUI this code splits log-dirty superpages _after_ they are first seen
to be dirty.  So the first time they're dirtied, the entire superpage is
marked, but OTOH if they're never dirtied you don't need to split.  Is
that right?  If so, I think it needs a comment explaining it.  It took
me a few minutes to understand this code and convince myself it was
safe. :)

> +                atomic_write_ept_entry(&epte[i], e);
> +            }
>              e.sa_p2mt = nt;
>              ept_p2m_type_to_flags(&e, nt, e.access);
> +            if (!mask)
> +                e.a = e.d = 0;

Again, probably best to leave the A-bit set.

Cheers,

Tim.

Re: [PATCH v2 4/4] xen: enable EPT dirty bit for guest live migration

[Hao, Xudong]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, June 28, 2012 9:52 PM
> To: Hao, Xudong
> Cc: xen-devel@lists.xen.org; Shan, Haitao; keir@xen.org; Zhang, Xiantao;
> JBeulich@suse.com
> Subject: Re: [Xen-devel] [PATCH v2 4/4] xen: enable EPT dirty bit for guest live
> migration
> 
> At 09:57 +0800 on 20 Jun (1340186267), Xudong Hao wrote:
> > @@ -69,6 +70,11 @@ static void ept_p2m_type_to_flags(ept_entry_t
> *entry, p2m_type_t type, p2m_acces
> >
> entry->mfn);
> >              break;
> >          case p2m_ram_logdirty:
> > +            entry->w = hap_has_dirty_bit;
> > +            entry->r = entry->x = 1;
> > +            /* Not necessarily need to clear A bit, but it is safe anyway */
> > +            entry->a = entry->d = 0;
> > +            break;
> 
> I think it's better not to clear the A bit - it will just cause the
> hardware to issue extra writes to set it.
> 

It's ok for me to not clear A bit here.

> > @@ -760,15 +770,33 @@ static void ept_change_entry_type_page(mfn_t
> ept_page_mfn, int ept_page_level,
> >
> >          if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
> >              ept_change_entry_type_page(_mfn(epte[i].mfn),
> > -                                       ept_page_level - 1, ot, nt);
> > +                                       ept_page_level - 1, ot, nt, d,
> mask);
> >          else
> >          {
> >              e = atomic_read_ept_entry(&epte[i]);
> >              if ( e.sa_p2mt != ot )
> >                  continue;
> >
> > +            if ( e.d && (e.sa_p2mt == p2m_ram_logdirty) )
> > +            {
> > +                int j, nr_pages;
> > +                struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +                for ( j = 0, nr_pages = 1; j < ept_page_level;
> > +                      j++, nr_pages *= 512 ) {}
> > +                for ( j = 0; j < nr_pages; j++ )
> > +                    paging_mark_dirty(d, e.mfn + j);
> > +
> > +                /* split super page to 4k page, so that dirty bitmap can
> > +                 * map the dirty page
> > +                 */
> > +                if ( !ept_split_super_page(p2m, &e, ept_page_level, 0) )
> > +                    continue;
> 
> AIUI this code splits log-dirty superpages _after_ they are first seen
> to be dirty.  So the first time they're dirtied, the entire superpage is
> marked, but OTOH if they're never dirtied you don't need to split.  Is
> that right?  

Right, it did need to split superpage which aren't dirtyied.

> If so, I think it needs a comment explaining it.  It took

I will add comment in code.

> me a few minutes to understand this code and convince myself it was
> safe. :)
> 
> > +                atomic_write_ept_entry(&epte[i], e);
> > +            }
> >              e.sa_p2mt = nt;
> >              ept_p2m_type_to_flags(&e, nt, e.access);
> > +            if (!mask)
> > +                e.a = e.d = 0;
> 
> Again, probably best to leave the A-bit set.
> 

Will remove A bit clearing here.

> Cheers,
> 
> Tim.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Hao, Xudong]

Hi, Keir and Jan

Any other comments for this series of patches?

Thanks,
-Xudong


> -----Original Message-----
> From: Hao, Xudong
> Sent: Wednesday, June 20, 2012 9:58 AM
> To: xen-devel@lists.xen.org
> Cc: JBeulich@suse.com; keir@xen.org; Zhang, Xiantao; Shan, Haitao; Hao,
> Xudong
> Subject: [PATCH v2 0/4] xen: enable EPT A/D bit feature
> 
> Changes from v1:
> - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to
> patch2.
> - define them as bool_t instead of int.
> 
> Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
> enable VMMs to efficiently implement memory management and page
> classification
> algorithms to optimize VM memory operations.
> 
> This series of patches enable EPT dirty bit feature for guest live migration.
> 
> PATCH 1/4: Add EPT A/D bits definitions.
> 
> PATCH 2/4: Add xen parameter to control A/D bits support, it on by default.
> 
> PATCH 3/4: Introduce a log_dirty new function update_dirty_bitmap, which will
> only update the log dirty bitmap, but won't clear the EPT page dirty bit.
> The function is used by live migration peek round with EPT D bit supported.
> 
> PATCH 4/4: enable EPT dirty bit for guest live migration.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Keir Fraser]

Review will probably come from Tim Deegan. There's no rush as it is too late
for 4.2 now.

 -- Keir

On 25/06/2012 07:27, "Hao, Xudong" <xudong.hao@intel.com> wrote:

> Hi, Keir and Jan
> 
> Any other comments for this series of patches?
> 
> Thanks,
> -Xudong
> 
> 
>> -----Original Message-----
>> From: Hao, Xudong
>> Sent: Wednesday, June 20, 2012 9:58 AM
>> To: xen-devel@lists.xen.org
>> Cc: JBeulich@suse.com; keir@xen.org; Zhang, Xiantao; Shan, Haitao; Hao,
>> Xudong
>> Subject: [PATCH v2 0/4] xen: enable EPT A/D bit feature
>> 
>> Changes from v1:
>> - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to
>> patch2.
>> - define them as bool_t instead of int.
>> 
>> Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
>> enable VMMs to efficiently implement memory management and page
>> classification
>> algorithms to optimize VM memory operations.
>> 
>> This series of patches enable EPT dirty bit feature for guest live migration.
>> 
>> PATCH 1/4: Add EPT A/D bits definitions.
>> 
>> PATCH 2/4: Add xen parameter to control A/D bits support, it on by default.
>> 
>> PATCH 3/4: Introduce a log_dirty new function update_dirty_bitmap, which will
>> only update the log dirty bitmap, but won't clear the EPT page dirty bit.
>> The function is used by live migration peek round with EPT D bit supported.
>> 
>> PATCH 4/4: enable EPT dirty bit for guest live migration.
> 

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Tim Deegan]

At 09:57 +0800 on 20 Jun (1340186263), Xudong Hao wrote:
> Changes from v1:
> - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to patch2.
> - define them as bool_t instead of int.
> 
> Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
> enable VMMs to efficiently implement memory management and page classification
> algorithms to optimize VM memory operations.
> 
> This series of patches enable EPT dirty bit feature for guest live migration.

Thanks for this.  I have a few high-level questions:

- You're proposing this for after 4.2, right?

- Have you measured what difference this makes?  I take it it improves
  performance during live migration but it would be good to know how much
  before taking on extra code.

- Is this intended to replace the old log-dirty mechanism for vram
  tracking as well?  It looks like it does that, but the description
  doesn't mention it. 

I'll be reviewing te patches in more detail later in the week. 

Cheers,

Tim.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Hao, Xudong]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Monday, June 25, 2012 5:02 PM
> To: Hao, Xudong
> Cc: xen-devel@lists.xen.org; Shan, Haitao; keir@xen.org; Zhang, Xiantao;
> JBeulich@suse.com
> Subject: Re: [Xen-devel] [PATCH v2 0/4] xen: enable EPT A/D bit feature
> 
> At 09:57 +0800 on 20 Jun (1340186263), Xudong Hao wrote:
> > Changes from v1:
> > - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to
> patch2.
> > - define them as bool_t instead of int.
> >
> > Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
> > enable VMMs to efficiently implement memory management and page
> classification
> > algorithms to optimize VM memory operations.
> >
> > This series of patches enable EPT dirty bit feature for guest live migration.
> 
> Thanks for this.  I have a few high-level questions:
> 
> - You're proposing this for after 4.2, right?
> 

Yes, they are not targeted to 4.2.

> - Have you measured what difference this makes?  I take it it improves
>   performance during live migration but it would be good to know how much
>   before taking on extra code.
> 

We did live migration performance testing with patches, it's embarrassed to say but the result show there is no performance gain and no performance loss indeed.

> - Is this intended to replace the old log-dirty mechanism for vram
>   tracking as well?  It looks like it does that, but the description
>   doesn't mention it.
> 

Yes, it changes log-dirty mechanism for vram tracking when EPT dirty bit support, but it does not replace old log-dirty if no EPT dirty bit feature.
I'll add description after you further review.

> I'll be reviewing te patches in more detail later in the week.
> 

Thanks.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Tim Deegan]

At 05:31 +0000 on 26 Jun (1340688686), Hao, Xudong wrote:
> > - Have you measured what difference this makes?  I take it it improves
> >   performance during live migration but it would be good to know how much
> >   before taking on extra code.
> 
> We did live migration performance testing with patches, it's
> embarrassed to say but the result show there is no performance gain
> and no performance loss indeed.

Ah. 

But maybe they have some performance advantage for guests with
graphics-heavy workloads (by making the VRAM tracking faster)?
Could you test that to see if that's the case?

If it's not, I think you'll understand if we don't take these patches.

Cheers,

Tim.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Hao, Xudong]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, June 28, 2012 6:30 PM
> To: Hao, Xudong
> Cc: keir@xen.org; Shan, Haitao; JBeulich@suse.com; Zhang, Xiantao;
> xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v2 0/4] xen: enable EPT A/D bit feature
> 
> At 05:31 +0000 on 26 Jun (1340688686), Hao, Xudong wrote:
> > > - Have you measured what difference this makes?  I take it it improves
> > >   performance during live migration but it would be good to know how
> much
> > >   before taking on extra code.
> >
> > We did live migration performance testing with patches, it's
> > embarrassed to say but the result show there is no performance gain
> > and no performance loss indeed.
> 
> Ah.
> 
> But maybe they have some performance advantage for guests with
> graphics-heavy workloads (by making the VRAM tracking faster)?
> Could you test that to see if that's the case?
> 

Ok, I'll test it.

> If it's not, I think you'll understand if we don't take these patches.
> 
> Cheers,
> 
> Tim.

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[George Dunlap]

On Tue, Jun 26, 2012 at 6:31 AM, Hao, Xudong <xudong.hao@intel.com> wrote:
>> -----Original Message-----
>> From: Tim Deegan [mailto:tim@xen.org]
>> Sent: Monday, June 25, 2012 5:02 PM
>> To: Hao, Xudong
>> Cc: xen-devel@lists.xen.org; Shan, Haitao; keir@xen.org; Zhang, Xiantao;
>> JBeulich@suse.com
>> Subject: Re: [Xen-devel] [PATCH v2 0/4] xen: enable EPT A/D bit feature
>>
>> At 09:57 +0800 on 20 Jun (1340186263), Xudong Hao wrote:
>> > Changes from v1:
>> > - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3 to
>> patch2.
>> > - define them as bool_t instead of int.
>> >
>> > Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
>> > enable VMMs to efficiently implement memory management and page
>> classification
>> > algorithms to optimize VM memory operations.
>> >
>> > This series of patches enable EPT dirty bit feature for guest live migration.
>>
>> Thanks for this.  I have a few high-level questions:
>>
>> - You're proposing this for after 4.2, right?
>>
>
> Yes, they are not targeted to 4.2.
>
>> - Have you measured what difference this makes?  I take it it improves
>>   performance during live migration but it would be good to know how much
>>   before taking on extra code.
>>
>
> We did live migration performance testing with patches, it's embarrassed to say but the result show there is no performance gain and no performance loss indeed.

What exactly did you measure?  If you measured the speed of the
migration itself on an empty system, it might not be noticeable; but
if, for instance, it increased the performance of a workload running
*inside* the guest during the migration, or if it decreased the cpu
usage of qemu during the migraiton, that might be worth it.

 -George

Re: [PATCH v2 0/4] xen: enable EPT A/D bit feature

[Hao, Xudong]

> -----Original Message-----
> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of George
> Dunlap
> Sent: Thursday, June 28, 2012 8:15 PM
> To: Hao, Xudong
> Cc: Tim Deegan; keir@xen.org; Shan, Haitao; JBeulich@suse.com; Zhang,
> Xiantao; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v2 0/4] xen: enable EPT A/D bit feature
> 
> On Tue, Jun 26, 2012 at 6:31 AM, Hao, Xudong <xudong.hao@intel.com>
> wrote:
> >> -----Original Message-----
> >> From: Tim Deegan [mailto:tim@xen.org]
> >> Sent: Monday, June 25, 2012 5:02 PM
> >> To: Hao, Xudong
> >> Cc: xen-devel@lists.xen.org; Shan, Haitao; keir@xen.org; Zhang, Xiantao;
> >> JBeulich@suse.com
> >> Subject: Re: [Xen-devel] [PATCH v2 0/4] xen: enable EPT A/D bit feature
> >>
> >> At 09:57 +0800 on 20 Jun (1340186263), Xudong Hao wrote:
> >> > Changes from v1:
> >> > - Move hap_has_dirty_bit and hap_has_access_bit definition from patch 3
> to
> >> patch2.
> >> > - define them as bool_t instead of int.
> >> >
> >> > Extended Page Tables introduce two bit: access bit and dirty bit, A/D bits
> >> > enable VMMs to efficiently implement memory management and page
> >> classification
> >> > algorithms to optimize VM memory operations.
> >> >
> >> > This series of patches enable EPT dirty bit feature for guest live migration.
> >>
> >> Thanks for this.  I have a few high-level questions:
> >>
> >> - You're proposing this for after 4.2, right?
> >>
> >
> > Yes, they are not targeted to 4.2.
> >
> >> - Have you measured what difference this makes?  I take it it improves
> >>   performance during live migration but it would be good to know how
> much
> >>   before taking on extra code.
> >>
> >
> > We did live migration performance testing with patches, it's embarrassed to
> say but the result show there is no performance gain and no performance loss
> indeed.
> 
> What exactly did you measure?  If you measured the speed of the
> migration itself on an empty system, it might not be noticeable; but
> if, for instance, it increased the performance of a workload running
> *inside* the guest during the migration, 

Measured both idle guest and w/workload(specjbb) running guest migration.

> or if it decreased the cpu
> usage of qemu during the migraiton, that might be worth it.
> 

Thanks this point, will look at if cpu usage decreasing.

>  -George