[v2 00/11] PML (Paging Modification Logging) support

[v2 00/11] PML (Paging Modification Logging) support

[Kai Huang]

This v2 patch series was rebased on latest upstream code.

v1->v2:

Firstly the sequence of this patch series were adjusted addressing Andrew and
Tim's comments:

    - Put v1 patch 2 (new boot parameter for PML) to be patch 1.
    - Added a new patch to patch the doc change of new added boot parameter
      of controlling PML enabling, as patch 2.
    - Added a new patch to introduce paging_mark_gfn_dirty, and made
      paging_mark_dirty as a wrapper of it, as patch 3.
    - Merged v1 patch 1 (EPT A/D bits support) and v1 patch 10 (enabling PML in
      p2m-ept.c) to a single patch as they both operates on EPT A/D bits, with
      considering EPT A/D bits will be enabled only when PML is used as well.
      To me, looks a single patch is more straightforward.

The v2 patch series were orginized as following, with detail changes described
under each of them.

patch 1: Add new boot parameter to control PML enabling
    - adjusted the sequence of parsing function and boot parameter.

patch 2: new patch for adding description for new PML boot parameter
    - inspired by "iommu=" parameter.

patch 3: add new paging_mark_gfn_dirty
    - Introduced new paging_mark_gfn_dirty, which takes guest pfn as parameter,
      and made paging_mark_dirty a wrapper of paging_mark_gfn_dirty, commented
      by Tim.

patch 4: PML feature detection
    - disable opt_pml_enabled if PML is not present, commented by Andrew.

patch 5 ~ 9: Add PML support in VMX
    - changed vmx_*_{enable|disable}_pml, vmx_domain_flush_pml_buffer to be
      idempotent, commented by Tim. vmx_vcpu_flush_pml_buffer remains the same
      as it is also called in PML buffer full VMEXIT.
    - changed vmx_{vcpu|domain}_pml_enabled to return bool_t, with taking const
      pointer (of vcpu or domain) as parameter, commented by Andrew.
    - changed vmx_vcpu_flush_pml_buffer calling paging_mark_gfn_dirty instead of
      paging_mark_dirty, commented by Tim.
    - changed various coding style issues and did several code refinements
      commented by Andrew.

patch 10: refine log-dirty common code to support PML
    - removed PML buffer flush callback in log_dirty_domain in paging layer.
    - changed to call p2m_flush_hardware_cached_dirty directly in
      hap_track_dirty_vram, and paging_log_dirty_op.

patch 11: enable EPT A/D bits and add PML support in p2m-ept.c
    - Merged EPT A/D bits support with enabling PML in p2m-ept.c as it's more
      straightforward, with considering EPT A/D bits will only be enabled if PML
      is used, and both of them operates on EPT A/D bits.
    - Manually set or clear A/D bits in ept_p2m_type_to_flags, and
      ept_set_middle_entry, commented by Tim.

Several sanity tests of live migration were done, and all tests worked well.

I also tested specjbb performance under global log-dirty, by using the same hack
mentioned in v1. The result is consistent with v1 (~10% improvement in global
log-dirty), and PML is beneficial in reducing hypervisor overhead in log-dirty
mode.

- global log-dirty:

    WP              PML (v1)        PML (v2)
    72862           79511	        80007
    73466           81173	        81614
    72989           81177	        82047
    73138           81777	        81975
    72811           80257	        80139
    72486           80413	        81127

avg 72959           80718	        81151
    100%            110.63%         111.22%



Kai Huang (11):
  vmx: add new boot parameter to control PML enabling
  doc: add description for new PML boot parameter
  log-dirty: add new paging_mark_gfn_dirty
  vmx: add PML definition and feature detection.
  vmx: add new data structure member to support PML
  vmx: add help functions to support PML
  vmx: handle PML buffer full VMEXIT
  vmx: handle PML enabling in vmx_vcpu_initialise
  vmx: disable PML in vmx_vcpu_destroy
  log-dirty: refine common code to support PML
  p2m/ept: enable PML in p2m-ept for log-dirty

 docs/misc/xen-command-line.markdown |  14 +++
 xen/arch/x86/hvm/vmx/vmcs.c         | 231 ++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c          |  35 ++++++
 xen/arch/x86/mm/hap/hap.c           |  29 ++++-
 xen/arch/x86/mm/p2m-ept.c           |  79 ++++++++++--
 xen/arch/x86/mm/p2m.c               |  36 ++++++
 xen/arch/x86/mm/paging.c            |  41 +++++--
 xen/include/asm-x86/hvm/vmx/vmcs.h  |  25 +++-
 xen/include/asm-x86/hvm/vmx/vmx.h   |   4 +-
 xen/include/asm-x86/p2m.h           |  11 ++
 xen/include/asm-x86/paging.h        |   2 +
 11 files changed, 484 insertions(+), 23 deletions(-)

-- 
2.1.0

[v2 01/11] vmx: add new boot parameter to control PML enabling

[Kai Huang]

A top level EPT parameter "ept=<options>" and a sub boolean "opt_pml_enabled"
are added to control PML. Other booleans can be further added for any other EPT
related features.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index d614638..4fff46d 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -64,6 +64,37 @@ integer_param("ple_gap", ple_gap);
 static unsigned int __read_mostly ple_window = 4096;
 integer_param("ple_window", ple_window);
 
+static bool_t __read_mostly opt_pml_enabled = 0;
+
+/*
+ * The 'ept' parameter controls functionalities that depend on, or impact the
+ * EPT mechanism. Optional comma separated value may contain:
+ *
+ *  pml                 Enable PML
+ */
+static void __init parse_ept_param(char *s)
+{
+    char *ss;
+    int val;
+
+    do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "pml") )
+            opt_pml_enabled = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+
+custom_param("ept", parse_ept_param);
+
 /* Dynamic (run-time adjusted) execution control flags. */
 u32 vmx_pin_based_exec_control __read_mostly;
 u32 vmx_cpu_based_exec_control __read_mostly;
-- 
2.1.0

[v2 02/11] doc: add description for new PML boot parameter

[Kai Huang]

This patch adds doc description for new boot parameter 'ept=pml'.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 docs/misc/xen-command-line.markdown | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 1dda1f0..ae30d02 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -685,6 +685,20 @@ requirement can be relaxed.  This option is particularly useful for nested
 virtualization, to allow the L1 hypervisor to use EPT even if the L0 hypervisor
 does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
 
+### ept (Intel)
+> `= List of [ pml ]`
+
+> Sub-options:
+
+> `pml`
+
+> This sub-option is boolean type and can be prefixed with `no-` to effect the
+> inverse meaning.
+
+> Default: `false`
+
+>> Control the use of Page Modification Logging for log-dirty.
+
 ### gdb
 > `= <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]] | pci | amt ] `
 
-- 
2.1.0

[v2 03/11] log-dirty: add new paging_mark_gfn_dirty

[Kai Huang]

PML logs GPA in PML buffer. Original paging_mark_dirty takes MFN as parameter
but it gets guest pfn internally and use guest pfn to as index for looking up
radix log-dirty tree. In flushing PML buffer, calling paging_mark_dirty directly
introduces redundant p2m lookups (gfn->mfn->gfn), therefore we introduce
paging_mark_gfn_dirty which is bulk of paging_mark_dirty but takes guest pfn as
parameter, and in flushing PML buffer we call paging_mark_gfn_dirty directly.
Original paging_mark_dirty then simply is a wrapper of paging_mark_gfn_dirty.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/mm/paging.c     | 31 +++++++++++++++++++++----------
 xen/include/asm-x86/paging.h |  2 ++
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index b54d76a..77c929b 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -266,24 +266,17 @@ static int paging_log_dirty_disable(struct domain *d, bool_t resuming)
     return ret;
 }
 
-/* Mark a page as dirty */
-void paging_mark_dirty(struct domain *d, unsigned long guest_mfn)
+/* Mark a page as dirty, with taking guest pfn as parameter */
+void paging_mark_gfn_dirty(struct domain *d, unsigned long pfn)
 {
-    unsigned long pfn;
-    mfn_t gmfn;
     int changed;
     mfn_t mfn, *l4, *l3, *l2;
     unsigned long *l1;
     int i1, i2, i3, i4;
 
-    gmfn = _mfn(guest_mfn);
-
-    if ( !paging_mode_log_dirty(d) || !mfn_valid(gmfn) ||
-         page_get_owner(mfn_to_page(gmfn)) != d )
+    if ( !paging_mode_log_dirty(d) )
         return;
 
-    /* We /really/ mean PFN here, even for non-translated guests. */
-    pfn = get_gpfn_from_mfn(mfn_x(gmfn));
     /* Shared MFNs should NEVER be marked dirty */
     BUG_ON(SHARED_M2P(pfn));
 
@@ -351,6 +344,24 @@ out:
     return;
 }
 
+/* Mark a page as dirty */
+void paging_mark_dirty(struct domain *d, unsigned long guest_mfn)
+{
+    unsigned long pfn;
+    mfn_t gmfn;
+
+    gmfn = _mfn(guest_mfn);
+
+    if ( !paging_mode_log_dirty(d) || !mfn_valid(gmfn) ||
+         page_get_owner(mfn_to_page(gmfn)) != d )
+        return;
+
+    /* We /really/ mean PFN here, even for non-translated guests. */
+    pfn = get_gpfn_from_mfn(mfn_x(gmfn));
+
+    paging_mark_gfn_dirty(d, pfn);
+}
+
 
 /* Is this guest page dirty? */
 int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index 53de715..c99324c 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -156,6 +156,8 @@ void paging_log_dirty_init(struct domain *d,
 
 /* mark a page as dirty */
 void paging_mark_dirty(struct domain *d, unsigned long guest_mfn);
+/* mark a page as dirty with taking guest pfn as parameter */
+void paging_mark_gfn_dirty(struct domain *d, unsigned long pfn);
 
 /* is this guest page dirty? 
  * This is called from inside paging code, with the paging lock held. */
-- 
2.1.0

[v2 04/11] vmx: add PML definition and feature detection.

[Kai Huang]

The patch adds PML definition and feature detection. Note PML won't be detected
if PML is disabled from boot parameter. PML is also disabled in construct_vmcs,
as it will only be enabled when domain is switched to log dirty mode.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        | 22 ++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  6 ++++++
 xen/include/asm-x86/hvm/vmx/vmx.h  |  1 +
 3 files changed, 29 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 4fff46d..d120370 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -141,6 +141,7 @@ static void __init vmx_display_features(void)
     P(cpu_has_vmx_virtual_intr_delivery, "Virtual Interrupt Delivery");
     P(cpu_has_vmx_posted_intr_processing, "Posted Interrupt Processing");
     P(cpu_has_vmx_vmcs_shadowing, "VMCS shadowing");
+    P(cpu_has_vmx_pml, "Page Modification Logging");
 #undef P
 
     if ( !printed )
@@ -238,6 +239,8 @@ static int vmx_init_vmcs_config(void)
             opt |= SECONDARY_EXEC_ENABLE_VPID;
         if ( opt_unrestricted_guest_enabled )
             opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;
+        if ( opt_pml_enabled )
+            opt |= SECONDARY_EXEC_ENABLE_PML;
 
         /*
          * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
@@ -284,6 +287,14 @@ static int vmx_init_vmcs_config(void)
          */
         if ( !(_vmx_ept_vpid_cap & VMX_VPID_INVVPID_ALL_CONTEXT) )
             _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
+
+        /*
+         * PML cannot be supported if EPT A/D bits is not supported. Actually,
+         * PML should not be detected if EPT A/D bits is not supported, but for
+         * sure we do the check anyway.
+         */
+        if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT) )
+            _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
     }
 
     if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )
@@ -304,6 +315,14 @@ static int vmx_init_vmcs_config(void)
                   SECONDARY_EXEC_UNRESTRICTED_GUEST);
     }
 
+    /* PML cannot be supported if EPT is not used */
+    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) )
+        _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+
+    /* Turn off opt_pml_enabled if PML feature is not present */
+    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML) )
+        opt_pml_enabled = 0;
+
     if ( (_vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
           ple_gap == 0 )
     {
@@ -1039,6 +1058,9 @@ static int construct_vmcs(struct vcpu *v)
         __vmwrite(POSTED_INTR_NOTIFICATION_VECTOR, posted_intr_vector);
     }
 
+    /* Disable PML anyway here as it will only be enabled in log dirty mode */
+    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+
     /* Host data selectors. */
     __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
     __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 6fce6aa..f831a78 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -215,6 +215,7 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_ENABLE_INVPCID           0x00001000
 #define SECONDARY_EXEC_ENABLE_VMFUNC            0x00002000
 #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
+#define SECONDARY_EXEC_ENABLE_PML               0x00020000
 extern u32 vmx_secondary_exec_control;
 
 #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
@@ -226,6 +227,7 @@ extern u32 vmx_secondary_exec_control;
 #define VMX_EPT_INVEPT_INSTRUCTION              0x00100000
 #define VMX_EPT_INVEPT_SINGLE_CONTEXT           0x02000000
 #define VMX_EPT_INVEPT_ALL_CONTEXT              0x04000000
+#define VMX_EPT_AD_BIT                          0x00200000
 
 #define VMX_MISC_VMWRITE_ALL                    0x20000000
 
@@ -274,6 +276,8 @@ extern u32 vmx_secondary_exec_control;
     (vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
 #define cpu_has_vmx_vmcs_shadowing \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
+#define cpu_has_vmx_pml \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
 
 #define VMCS_RID_TYPE_MASK              0x80000000
 
@@ -318,6 +322,7 @@ enum vmcs_field {
     GUEST_LDTR_SELECTOR             = 0x0000080c,
     GUEST_TR_SELECTOR               = 0x0000080e,
     GUEST_INTR_STATUS               = 0x00000810,
+    GUEST_PML_INDEX                 = 0x00000812,
     HOST_ES_SELECTOR                = 0x00000c00,
     HOST_CS_SELECTOR                = 0x00000c02,
     HOST_SS_SELECTOR                = 0x00000c04,
@@ -331,6 +336,7 @@ enum vmcs_field {
     VM_EXIT_MSR_STORE_ADDR          = 0x00002006,
     VM_EXIT_MSR_LOAD_ADDR           = 0x00002008,
     VM_ENTRY_MSR_LOAD_ADDR          = 0x0000200a,
+    PML_ADDRESS                     = 0x0000200e,
     TSC_OFFSET                      = 0x00002010,
     VIRTUAL_APIC_PAGE_ADDR          = 0x00002012,
     APIC_ACCESS_ADDR                = 0x00002014,
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 91c5e18..50f1bfc 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -185,6 +185,7 @@ static inline unsigned long pi_get_pir(struct pi_desc *pi_desc, int group)
 #define EXIT_REASON_XSETBV              55
 #define EXIT_REASON_APIC_WRITE          56
 #define EXIT_REASON_INVPCID             58
+#define EXIT_REASON_PML_FULL            62
 
 /*
  * Interruption-information format
-- 
2.1.0

[v2 05/11] vmx: add new data structure member to support PML

[Kai Huang]

A new 4K page pointer is added to arch_vmx_struct as PML buffer for vcpu. And a
new 'status' field is added to vmx_domain to indicate whether PML is enabled for
the domain or not. The 'status' field also can be used for further similiar
purpose.

Note both new members don't have to be initialized to zero explicitly as both
vcpu and domain structure are zero-ed when they are created.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index f831a78..2c679ac 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -70,8 +70,12 @@ struct ept_data {
     cpumask_var_t synced_mask;
 };
 
+#define _VMX_DOMAIN_PML_ENABLED    0
+#define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
 struct vmx_domain {
     unsigned long apic_access_mfn;
+    /* VMX_DOMAIN_* */
+    unsigned long status;
 };
 
 struct pi_desc {
@@ -142,6 +146,9 @@ struct arch_vmx_struct {
     /* Bitmap to control vmexit policy for Non-root VMREAD/VMWRITE */
     struct page_info     *vmread_bitmap;
     struct page_info     *vmwrite_bitmap;
+
+#define NR_PML_ENTRIES   512
+    struct page_info     *pml_pg;
 };
 
 int vmx_create_vmcs(struct vcpu *v);
-- 
2.1.0

[v2 06/11] vmx: add help functions to support PML

[Kai Huang]

This patch adds help functions to enable/disable PML, and flush PML buffer for
single vcpu and particular domain for further use.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        | 178 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
 2 files changed, 187 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index d120370..d3cb50f 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1328,6 +1328,184 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector)
                 &v->arch.hvm_vmx.eoi_exitmap_changed);
 }
 
+bool_t vmx_vcpu_pml_enabled(const struct vcpu *v)
+{
+    return !!(v->arch.hvm_vmx.secondary_exec_control &
+              SECONDARY_EXEC_ENABLE_PML);
+}
+
+int vmx_vcpu_enable_pml(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    if ( vmx_vcpu_pml_enabled(v) )
+        return 0;
+
+    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
+    if ( !v->arch.hvm_vmx.pml_pg )
+        return -ENOMEM;
+
+    vmx_vmcs_enter(v);
+
+    __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) << PAGE_SHIFT);
+    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
+
+    v->arch.hvm_vmx.secondary_exec_control |= SECONDARY_EXEC_ENABLE_PML;
+
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+              v->arch.hvm_vmx.secondary_exec_control);
+
+    vmx_vmcs_exit(v);
+
+    return 0;
+}
+
+void vmx_vcpu_disable_pml(struct vcpu *v)
+{
+    if ( !vmx_vcpu_pml_enabled(v) )
+        return;
+
+    /* Make sure we don't lose any logged GPAs */
+    vmx_vcpu_flush_pml_buffer(v);
+
+    vmx_vmcs_enter(v);
+
+    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+              v->arch.hvm_vmx.secondary_exec_control);
+
+    vmx_vmcs_exit(v);
+
+    v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg);
+    v->arch.hvm_vmx.pml_pg = NULL;
+}
+
+void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
+{
+    uint64_t *pml_buf;
+    unsigned long pml_idx;
+
+    ASSERT(vmx_vcpu_pml_enabled(v));
+
+    vmx_vmcs_enter(v);
+
+    __vmread(GUEST_PML_INDEX, &pml_idx);
+
+    /* Do nothing if PML buffer is empty */
+    if ( pml_idx == (NR_PML_ENTRIES - 1) )
+        goto out;
+
+    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
+
+    /*
+     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is not
+     * full), and in latter case PML index always points to next available
+     * entity.
+     */
+    if (pml_idx >= NR_PML_ENTRIES)
+        pml_idx = 0;
+    else
+        pml_idx++;
+
+    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
+    {
+        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
+        /*
+         * Need to change type from log-dirty to normal memory for logged GFN.
+         * hap_track_dirty_vram depends on it to work. And we really only need
+         * to mark GFNs which hve been successfully changed from log-dirty to
+         * normal memory to be dirty.
+         */
+        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
+                    p2m_ram_rw) )
+            paging_mark_gfn_dirty(v->domain, gfn);
+    }
+
+    unmap_domain_page(pml_buf);
+
+    /* Reset PML index */
+    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
+
+out:
+    vmx_vmcs_exit(v);
+}
+
+bool_t vmx_domain_pml_enabled(const struct domain *d)
+{
+    return !!(d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED);
+}
+
+/*
+ * This function enables PML for particular domain. It should be called when
+ * domain is paused.
+ *
+ * PML needs to be enabled globally for all vcpus of the domain, as PML buffer
+ * and PML index are pre-vcpu, but EPT table is shared by vcpus, therefore
+ * enabling PML on partial vcpus won't work.
+ */
+int vmx_domain_enable_pml(struct domain *d)
+{
+    struct vcpu *v;
+    int rc;
+
+    ASSERT(atomic_read(&d->pause_count));
+
+    if ( vmx_domain_pml_enabled(d) )
+        return 0;
+
+    for_each_vcpu( d, v )
+        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
+            goto error;
+
+    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
+
+    return 0;
+
+error:
+    for_each_vcpu( d, v )
+        if ( vmx_vcpu_pml_enabled(v) )
+            vmx_vcpu_disable_pml(v);
+    return rc;
+}
+
+/*
+ * Disable PML for particular domain. Called when domain is paused.
+ *
+ * The same as enabling PML for domain, disabling PML should be done for all
+ * vcpus at once.
+ */
+void vmx_domain_disable_pml(struct domain *d)
+{
+    struct vcpu *v;
+
+    ASSERT(atomic_read(&d->pause_count));
+
+    if ( !vmx_domain_pml_enabled(d) )
+        return;
+
+    for_each_vcpu( d, v )
+        vmx_vcpu_disable_pml(v);
+
+    d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED;
+}
+
+/*
+ * Flush PML buffer of all vcpus, and update the logged dirty pages to log-dirty
+ * radix tree. Called when domain is paused.
+ */
+void vmx_domain_flush_pml_buffers(struct domain *d)
+{
+    struct vcpu *v;
+
+    ASSERT(atomic_read(&d->pause_count));
+
+    if ( !vmx_domain_pml_enabled(d) )
+        return;
+
+    for_each_vcpu( d, v )
+        vmx_vcpu_flush_pml_buffer(v);
+}
+
 int vmx_create_vmcs(struct vcpu *v)
 {
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 2c679ac..ceb09bf 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -498,6 +498,15 @@ static inline int vmx_add_host_load_msr(u32 msr)
 
 DECLARE_PER_CPU(bool_t, vmxon);
 
+bool_t vmx_vcpu_pml_enabled(const struct vcpu *v);
+int vmx_vcpu_enable_pml(struct vcpu *v);
+void vmx_vcpu_disable_pml(struct vcpu *v);
+void vmx_vcpu_flush_pml_buffer(struct vcpu *v);
+bool_t vmx_domain_pml_enabled(const struct domain *d);
+int vmx_domain_enable_pml(struct domain *d);
+void vmx_domain_disable_pml(struct domain *d);
+void vmx_domain_flush_pml_buffers(struct domain *d);
+
 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
 
 /*
-- 
2.1.0

[v2 07/11] vmx: handle PML buffer full VMEXIT

[Kai Huang]

We need to flush PML buffer when it's full.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2ac1492..279e745 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3177,6 +3177,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_handle_apic_write();
         break;
 
+    case EXIT_REASON_PML_FULL:
+        vmx_vcpu_flush_pml_buffer(v);
+        break;
+
     case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
     case EXIT_REASON_ACCESS_LDTR_OR_TR:
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
-- 
2.1.0

[v2 08/11] vmx: handle PML enabling in vmx_vcpu_initialise

[Kai Huang]

It's possible domain has already been in log-dirty mode when creating vcpu, in
which case we should enable PML for this vcpu if PML has been enabled for the
domain.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 279e745..ad9d7d4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -116,6 +116,29 @@ static int vmx_vcpu_initialise(struct vcpu *v)
         return rc;
     }
 
+    /*
+     * It's rare but still possible that domain has already been in log-dirty
+     * mode when vcpu is being created (commented by Tim), in which case we
+     * should enable PML for this vcpu if PML has been enabled for the domain,
+     * and failure to enable results in failure of creating this vcpu.
+     *
+     * Note even there's no vcpu created for the domain, vmx_domain_enable_pml
+     * will return successful in which case vmx_domain_pml_enabled will also
+     * return true. And even this is the first vcpu to be created with
+     * vmx_domain_pml_enabled being true, failure of enabling PML still results
+     * in failure of creating vcpu, to avoid complicated logic to revert PML
+     * style EPT table to non-PML style EPT table.
+     */
+    if ( vmx_domain_pml_enabled(v->domain) )
+    {
+        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
+        {
+            dprintk(XENLOG_ERR, "%pv: Failed to enable PML.\n", v);
+            vmx_destroy_vmcs(v);
+            return rc;
+        }
+    }
+
     vpmu_initialise(v);
 
     vmx_install_vlapic_mapping(v);
-- 
2.1.0

[v2 09/11] vmx: disable PML in vmx_vcpu_destroy

[Kai Huang]

It's possible domain still remains in log-dirty mode when it is about to be
destroyed, in which case we should manually disable PML for it.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index ad9d7d4..821e90b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -152,6 +152,14 @@ static int vmx_vcpu_initialise(struct vcpu *v)
 
 static void vmx_vcpu_destroy(struct vcpu *v)
 {
+    /*
+     * There are cases that domain still remains in log-dirty mode when it is
+     * about to be destroyed (ex, user types 'xl destroy <dom>'), in which case
+     * we should disable PML manually here. Note that vmx_vcpu_destroy is called
+     * prior to vmx_domain_destroy so we need to disable PML for each vcpu
+     * separately here.
+     */
+    vmx_vcpu_disable_pml(v);
     vmx_destroy_vmcs(v);
     vpmu_destroy(v);
     passive_domain_destroy(v);
-- 
2.1.0

[v2 10/11] log-dirty: refine common code to support PML

[Kai Huang]

Using PML, it's possible there are dirty GPAs logged in vcpus' PML buffers
when userspace peek/clear dirty pages, therefore we need to flush them befor
reporting dirty pages to userspace. This applies to both video ram tracking and
paging_log_dirty_op.

This patch adds new p2m layer functions to enable/disable PML and flush PML
buffers. The new functions are named to be generic to cover potential futher
PML-like features for other platforms.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/mm/hap/hap.c | 29 +++++++++++++++++++++++++----
 xen/arch/x86/mm/p2m.c     | 36 ++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/paging.c  | 10 ++++++++++
 xen/include/asm-x86/p2m.h | 11 +++++++++++
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 4ecb2e2..1099670 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -121,7 +121,10 @@ int hap_track_dirty_vram(struct domain *d,
                 p2m_change_type_range(d, ostart, oend,
                                       p2m_ram_logdirty, p2m_ram_rw);
 
-            /* set l1e entries of range within P2M table to be read-only. */
+            /*
+             * switch vram to log dirty mode, either by setting l1e entries of
+             * P2M table to be read-only, or via hardware-assisted log-dirty.
+             */
             p2m_change_type_range(d, begin_pfn, begin_pfn + nr,
                                   p2m_ram_rw, p2m_ram_logdirty);
 
@@ -135,6 +138,9 @@ int hap_track_dirty_vram(struct domain *d,
 
             domain_pause(d);
 
+            /* flush dirty GFNs potentially cached by hardware */
+            p2m_flush_hardware_cached_dirty(d);
+
             /* get the bitmap */
             paging_log_dirty_range(d, begin_pfn, nr, dirty_bitmap);
 
@@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
     d->arch.paging.mode |= PG_log_dirty;
     paging_unlock(d);
 
+    /* enable hardware-assisted log-dirty if it is supported */
+    p2m_enable_hardware_log_dirty(d);
+
     if ( log_global )
     {
-        /* set l1e entries of P2M table to be read-only. */
+        /*
+         * switch to log dirty mode, either by setting l1e entries of P2M table
+         * to be read-only, or via hardware-assisted log-dirty.
+         */
         p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
         flush_tlb_mask(d->domain_dirty_cpumask);
     }
@@ -205,14 +217,23 @@ static int hap_disable_log_dirty(struct domain *d)
     d->arch.paging.mode &= ~PG_log_dirty;
     paging_unlock(d);
 
-    /* set l1e entries of P2M table with normal mode */
+    /* disable hardware-assisted log-dirty if it is supported */
+    p2m_disable_hardware_log_dirty(d);
+
+    /*
+     * switch to normal mode, either by setting l1e entries of P2M table to
+     * normal mode, or via hardware-assisted log-dirty.
+     */
     p2m_change_entry_type_global(d, p2m_ram_logdirty, p2m_ram_rw);
     return 0;
 }
 
 static void hap_clean_dirty_bitmap(struct domain *d)
 {
-    /* set l1e entries of P2M table to be read-only. */
+    /*
+     * switch to log-dirty mode, either by setting l1e entries of P2M table to
+     * be read-only, or via hardware-assisted log-dirty.
+     */
     p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
     flush_tlb_mask(d->domain_dirty_cpumask);
 }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6a06e9f..291a275 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -239,6 +239,42 @@ void p2m_memory_type_changed(struct domain *d)
     }
 }
 
+void p2m_enable_hardware_log_dirty(struct domain *d)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    if ( p2m->enable_hardware_log_dirty )
+    {
+        p2m_lock(p2m);
+        p2m->enable_hardware_log_dirty(p2m);
+        p2m_unlock(p2m);
+    }
+}
+
+void p2m_disable_hardware_log_dirty(struct domain *d)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    if ( p2m->disable_hardware_log_dirty )
+    {
+        p2m_lock(p2m);
+        p2m->disable_hardware_log_dirty(p2m);
+        p2m_unlock(p2m);
+    }
+}
+
+void p2m_flush_hardware_cached_dirty(struct domain *d)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    if ( p2m->flush_hardware_cached_dirty )
+    {
+        p2m_lock(p2m);
+        p2m->flush_hardware_cached_dirty(p2m);
+        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 77c929b..59d4720 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -422,7 +422,17 @@ static int paging_log_dirty_op(struct domain *d,
     int i4, i3, i2;
 
     if ( !resuming )
+    {
         domain_pause(d);
+
+        /*
+         * Flush dirty GFNs potentially cached by hardware. Only need to flush
+         * when not resuming, as domain was paused in resuming case therefore
+         * it's not possible to have any new dirty pages.
+         */
+        p2m_flush_hardware_cached_dirty(d);
+    }
+
     paging_lock(d);
 
     if ( !d->arch.paging.preempt.dom )
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index e93c551..91c17a5 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -233,6 +233,9 @@ struct p2m_domain {
                                        p2m_access_t *p2ma,
                                        p2m_query_t q,
                                        unsigned int *page_order);
+    void               (*enable_hardware_log_dirty)(struct p2m_domain *p2m);
+    void               (*disable_hardware_log_dirty)(struct p2m_domain *p2m);
+    void               (*flush_hardware_cached_dirty)(struct p2m_domain *p2m);
     void               (*change_entry_type_global)(struct p2m_domain *p2m,
                                                    p2m_type_t ot,
                                                    p2m_type_t nt);
@@ -507,6 +510,14 @@ void guest_physmap_remove_page(struct domain *d,
 /* Set a p2m range as populate-on-demand */
 int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
                                           unsigned int order);
+/* Enable hardware-assisted log-dirty. */
+void p2m_enable_hardware_log_dirty(struct domain *d);
+
+/* Disable hardware-assisted log-dirty */
+void p2m_disable_hardware_log_dirty(struct domain *d);
+
+/* Flush hardware cached dirty GFNs */
+void p2m_flush_hardware_cached_dirty(struct domain *d);
 
 /* Change types across all p2m entries in a domain */
 void p2m_change_entry_type_global(struct domain *d, 
-- 
2.1.0

[v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Kai Huang]

This patch firstly enables EPT A/D bits if PML is used, as PML depends on EPT
A/D bits to work. A bit is set for all present leaf p2m types, D bit is set for
all writable types, except log-dirty type.

With PML, for 4K pages, instead of setting EPT entry to read-only, we just need
to clear D bit in order to log that GFN. For superpages, we still need to set it
to read-only as we need to split superpage to 4K pages in EPT violation.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 xen/arch/x86/mm/p2m-ept.c          | 79 ++++++++++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  3 +-
 xen/include/asm-x86/hvm/vmx/vmx.h  |  3 +-
 3 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 5e95a83..ff84c16 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -102,9 +102,20 @@ static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
     return rc;
 }
 
-static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_access_t access)
+static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
+                                  p2m_type_t type, p2m_access_t access)
 {
-    /* First apply type permissions */
+    /*
+     * First apply type permissions.
+     *
+     * A/D bits are also manually set to avoid overhead of MMU having to set
+     * them later. Both A/D bits are safe to be updated directly as they are
+     * ignored by processor if EPT A/D bits is not turned on.
+     *
+     * A bit is set for all present leaf types. D bit is set for all writable
+     * types and cleared for read-only types, as read-only types are apparently
+     * impossible to be dirty.
+     */
     switch(type)
     {
         case p2m_invalid:
@@ -118,27 +129,51 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces
             break;
         case p2m_ram_rw:
             entry->r = entry->w = entry->x = 1;
+            entry->a = entry->d = 1;
             break;
         case p2m_mmio_direct:
             entry->r = entry->x = 1;
             entry->w = !rangeset_contains_singleton(mmio_ro_ranges,
                                                     entry->mfn);
+            entry->a = 1;
+            entry->d = entry->w;
             break;
         case p2m_ram_logdirty:
+            entry->r = entry->x = 1;
+            /*
+             * In case of PML, we don't have to write protect 4K page, but
+             * only need to clear D-bit for it, but we still need to write
+             * protect super page in order to split it to 4K pages in EPT
+             * violation.
+             */
+            if ( vmx_domain_pml_enabled(p2m->domain)
+                 && !is_epte_superpage(entry) )
+                entry->w = 1;
+            else
+                entry->w = 0;
+            entry->a = 1;
+            /* For both PML or non-PML cases we clear D bit anyway */
+            entry->d = 0;
+            break;
         case p2m_ram_ro:
         case p2m_ram_shared:
             entry->r = entry->x = 1;
             entry->w = 0;
+            entry->a = 1;
+            entry->d = 0;
             break;
         case p2m_grant_map_rw:
         case p2m_map_foreign:
             entry->r = entry->w = 1;
             entry->x = 0;
+            entry->a = entry->d = 1;
             break;
         case p2m_grant_map_ro:
         case p2m_mmio_write_dm:
             entry->r = 1;
             entry->w = entry->x = 0;
+            entry->a = 1;
+            entry->d = 0;
             break;
     }
 
@@ -194,6 +229,8 @@ static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t *ept_entry)
     ept_entry->access = p2m->default_access;
 
     ept_entry->r = ept_entry->w = ept_entry->x = 1;
+    /* Manually set A bit to avoid overhead of MMU having to write it later. */
+    ept_entry->a = 1;
 
     return 1;
 }
@@ -244,10 +281,9 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
         epte->sp = (level > 1);
         epte->mfn += i * trunk;
         epte->snp = (iommu_enabled && iommu_snoop);
-        ASSERT(!epte->rsvd1);
         ASSERT(!epte->avail3);
 
-        ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);
+        ept_p2m_type_to_flags(p2m, epte, epte->sa_p2mt, epte->access);
 
         if ( (level - 1) == target )
             continue;
@@ -489,7 +525,7 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
                     {
                          e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + i, gfn + i)
                                      ? p2m_ram_logdirty : p2m_ram_rw;
-                         ept_p2m_type_to_flags(&e, e.sa_p2mt, e.access);
+                         ept_p2m_type_to_flags(p2m, &e, e.sa_p2mt, e.access);
                     }
                     e.recalc = 0;
                     wrc = atomic_write_ept_entry(&epte[i], e, level);
@@ -541,7 +577,7 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
                 e.ipat = ipat;
                 e.recalc = 0;
                 if ( recalc && p2m_is_changeable(e.sa_p2mt) )
-                    ept_p2m_type_to_flags(&e, e.sa_p2mt, e.access);
+                    ept_p2m_type_to_flags(p2m, &e, e.sa_p2mt, e.access);
                 wrc = atomic_write_ept_entry(&epte[i], e, level);
                 ASSERT(wrc == 0);
             }
@@ -752,7 +788,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         if ( ept_entry->mfn == new_entry.mfn )
              need_modify_vtd_table = 0;
 
-        ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
+        ept_p2m_type_to_flags(p2m, &new_entry, p2mt, p2ma);
     }
 
     rc = atomic_write_ept_entry(ept_entry, new_entry, target);
@@ -1053,6 +1089,26 @@ void ept_sync_domain(struct p2m_domain *p2m)
                      __ept_sync_domain, p2m, 1);
 }
 
+static void ept_enable_pml(struct p2m_domain *p2m)
+{
+    /*
+     * No need to check if vmx_domain_enable_pml has succeeded or not, as
+     * ept_p2m_type_to_flags will do the check, and write protection will be
+     * used if PML is not enabled.
+     */
+    vmx_domain_enable_pml(p2m->domain);
+}
+
+static void ept_disable_pml(struct p2m_domain *p2m)
+{
+    vmx_domain_disable_pml(p2m->domain);
+}
+
+static void ept_flush_pml_buffers(struct p2m_domain *p2m)
+{
+    vmx_domain_flush_pml_buffers(p2m->domain);
+}
+
 int ept_p2m_init(struct p2m_domain *p2m)
 {
     struct ept_data *ept = &p2m->ept;
@@ -1070,6 +1126,15 @@ int ept_p2m_init(struct p2m_domain *p2m)
     /* set EPT page-walk length, now it's actual walk length - 1, i.e. 3 */
     ept->ept_wl = 3;
 
+    if ( cpu_has_vmx_pml )
+    {
+        /* Enable EPT A/D bits if we are going to use PML */
+        ept->ept_ad = cpu_has_vmx_pml ? 1 : 0;
+        p2m->enable_hardware_log_dirty = ept_enable_pml;
+        p2m->disable_hardware_log_dirty = ept_disable_pml;
+        p2m->flush_hardware_cached_dirty = ept_flush_pml_buffers;
+    }
+
     if ( !zalloc_cpumask_var(&ept->synced_mask) )
         return -ENOMEM;
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index ceb09bf..afdaf6b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -62,7 +62,8 @@ struct ept_data {
     struct {
             u64 ept_mt :3,
                 ept_wl :3,
-                rsvd   :6,
+                ept_ad :1,  /* bit 6 - enable EPT A/D bits */
+                rsvd   :5,
                 asr    :52;
         };
         u64 eptp;
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 50f1bfc..35f804a 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 */
         recalc      :   1,  /* bit 10 - Software available 1 */
         snp         :   1,  /* bit 11 - VT-d snoop control in shared
                                EPT/VT-d usage */
-- 
2.1.0

Re: [v2 01/11] vmx: add new boot parameter to control PML enabling

[Andrew Cooper]

On 15/04/15 08:03, Kai Huang wrote:
> A top level EPT parameter "ept=<options>" and a sub boolean "opt_pml_enabled"
> are added to control PML. Other booleans can be further added for any other EPT
> related features.
>
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/vmx/vmcs.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d614638..4fff46d 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -64,6 +64,37 @@ integer_param("ple_gap", ple_gap);
>  static unsigned int __read_mostly ple_window = 4096;
>  integer_param("ple_window", ple_window);
>  
> +static bool_t __read_mostly opt_pml_enabled = 0;
> +
> +/*
> + * The 'ept' parameter controls functionalities that depend on, or impact the
> + * EPT mechanism. Optional comma separated value may contain:
> + *
> + *  pml                 Enable PML
> + */
> +static void __init parse_ept_param(char *s)
> +{
> +    char *ss;
> +    int val;
> +
> +    do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;
> +
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        if ( !strcmp(s, "pml") )
> +            opt_pml_enabled = val;
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +
> +custom_param("ept", parse_ept_param);
> +
>  /* Dynamic (run-time adjusted) execution control flags. */
>  u32 vmx_pin_based_exec_control __read_mostly;
>  u32 vmx_cpu_based_exec_control __read_mostly;

Re: [v2 02/11] doc: add description for new PML boot parameter

[Andrew Cooper]

On 15/04/15 08:03, Kai Huang wrote:
> This patch adds doc description for new boot parameter 'ept=pml'.
>
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>

Personally, I would fold this patch into the previous so the
documentation is in the same patch as introduces the parameter.

> ---
>  docs/misc/xen-command-line.markdown | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 1dda1f0..ae30d02 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -685,6 +685,20 @@ requirement can be relaxed.  This option is particularly useful for nested
>  virtualization, to allow the L1 hypervisor to use EPT even if the L0 hypervisor
>  does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
>  
> +### ept (Intel)
> +> `= List of [ pml ]`
> +
> +> Sub-options:
> +
> +> `pml`
> +
> +> This sub-option is boolean type and can be prefixed with `no-` to effect the
> +> inverse meaning.
> +
> +> Default: `false`
> +
> +>> Control the use of Page Modification Logging for log-dirty.

Can you follow the same style as the "psr" option?  Specifically, I
don't think you need to re-describe what a boolean parameter is.

~Andrew

> +
>  ### gdb
>  > `= <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]] | pci | amt ] `
>  

Re: [v2 02/11] doc: add description for new PML boot parameter

[Jan Beulich]

>>> On 15.04.15 at 12:15, <andrew.cooper3@citrix.com> wrote:
> On 15/04/15 08:03, Kai Huang wrote:
>> This patch adds doc description for new boot parameter 'ept=pml'.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> 
> Personally, I would fold this patch into the previous so the
> documentation is in the same patch as introduces the parameter.

+1

Jan

Re: [v2 01/11] vmx: add new boot parameter to control PML enabling

[Jan Beulich]

>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
> +static void __init parse_ept_param(char *s)
> +{
> +    char *ss;
> +    int val;

bool_t, and would better move ...

> +
> +    do {
> +        val = !!strncmp(s, "no-", 3);

... here (making the right side expression its intializer).

Jan

Re: [v2 01/11] vmx: add new boot parameter to control PML enabling

[Kai Huang]

On Wed, Apr 15, 2015 at 8:20 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> +static void __init parse_ept_param(char *s)
>> +{
>> +    char *ss;
>> +    int val;
>
> bool_t, and would better move ...
>
>> +
>> +    do {
>> +        val = !!strncmp(s, "no-", 3);
>
> ... here (making the right side expression its intializer).

Hi Jan,

Thanks for review. Do you mean below?

    do {
        bool_t val = !!strncmp(s,  "no-", 3);
        ...

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

Re: [v2 01/11] vmx: add new boot parameter to control PML enabling

[Jan Beulich]

>>> On 15.04.15 at 15:20, <kaih.linux@gmail.com> wrote:
> On Wed, Apr 15, 2015 at 8:20 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>> +static void __init parse_ept_param(char *s)
>>> +{
>>> +    char *ss;
>>> +    int val;
>>
>> bool_t, and would better move ...
>>
>>> +
>>> +    do {
>>> +        val = !!strncmp(s, "no-", 3);
>>
>> ... here (making the right side expression its intializer).
> 
> Thanks for review. Do you mean below?
> 
>     do {
>         bool_t val = !!strncmp(s,  "no-", 3);
>         ...

Yes.

Jan

Re: [v2 02/11] doc: add description for new PML boot parameter

[Kai Huang]

On 04/15/2015 06:15 PM, Andrew Cooper wrote:
> On 15/04/15 08:03, Kai Huang wrote:
>> This patch adds doc description for new boot parameter 'ept=pml'.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> Personally, I would fold this patch into the previous so the
> documentation is in the same patch as introduces the parameter.
Thanks for suggestion. Will do in the next version.

>
>> ---
>>   docs/misc/xen-command-line.markdown | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
>> index 1dda1f0..ae30d02 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -685,6 +685,20 @@ requirement can be relaxed.  This option is particularly useful for nested
>>   virtualization, to allow the L1 hypervisor to use EPT even if the L0 hypervisor
>>   does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
>>   
>> +### ept (Intel)
>> +> `= List of [ pml ]`
>> +
>> +> Sub-options:
>> +
>> +> `pml`
>> +
>> +> This sub-option is boolean type and can be prefixed with `no-` to effect the
>> +> inverse meaning.
>> +
>> +> Default: `false`
>> +
>> +>> Control the use of Page Modification Logging for log-dirty.
> Can you follow the same style as the "psr" option?  Specifically, I
> don't think you need to re-describe what a boolean parameter is.
Thanks for suggestion. I chose to follow the "iommu=" style parameter as 
to me the "ept=" parameter is more likely to the "iommu=" parameter -- 
both of them intend to have various sub-boolean types, within which some 
may or may not be independent to each other.

Referring to 'psr' parameter, does below change look good to you?

+### ept (Intel)
+> `= List of ( pml<boolean> )`
+
+> Default: `false`
+
+Controls EPT related features. Currently only Page Modification Logging 
(PML) is
+the controllable feature as boolean type.
+
+PML is a new hardware feature in Intel's Broadwell Server and further 
platforms
+which reduces hypervisor overhead of log-dirty mechanism by automatically
+recording GPAs (guest physical addresses) when guest memory gets dirty, and
+therefore significantly reducing number of EPT violation caused by write
+protection of guest memory, which is a necessity to implement log-dirty
+mechanism before PML.

Thanks,
-Kai
>
> ~Andrew
>
>> +
>>   ### gdb
>>   > `= <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]] | pci | amt ] `
>>   
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 00/11] PML (Paging Modification Logging) support

[Tim Deegan]

At 15:03 +0800 on 15 Apr (1429110222), Kai Huang wrote:
> This v2 patch series was rebased on latest upstream code.

Looks good to me.

Acked-by: Tim Deegan <tim@xen.org>

Re: [v2 02/11] doc: add description for new PML boot parameter

[Andrew Cooper]

On 16/04/15 05:47, Kai Huang wrote:
>
>
> On 04/15/2015 06:15 PM, Andrew Cooper wrote:
>> On 15/04/15 08:03, Kai Huang wrote:
>>> This patch adds doc description for new boot parameter 'ept=pml'.
>>>
>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> Personally, I would fold this patch into the previous so the
>> documentation is in the same patch as introduces the parameter.
> Thanks for suggestion. Will do in the next version.
>
>>
>>> ---
>>>   docs/misc/xen-command-line.markdown | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/docs/misc/xen-command-line.markdown
>>> b/docs/misc/xen-command-line.markdown
>>> index 1dda1f0..ae30d02 100644
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -685,6 +685,20 @@ requirement can be relaxed.  This option is
>>> particularly useful for nested
>>>   virtualization, to allow the L1 hypervisor to use EPT even if the
>>> L0 hypervisor
>>>   does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
>>>   +### ept (Intel)
>>> +> `= List of [ pml ]`
>>> +
>>> +> Sub-options:
>>> +
>>> +> `pml`
>>> +
>>> +> This sub-option is boolean type and can be prefixed with `no-` to
>>> effect the
>>> +> inverse meaning.
>>> +
>>> +> Default: `false`
>>> +
>>> +>> Control the use of Page Modification Logging for log-dirty.
>> Can you follow the same style as the "psr" option?  Specifically, I
>> don't think you need to re-describe what a boolean parameter is.
> Thanks for suggestion. I chose to follow the "iommu=" style parameter
> as to me the "ept=" parameter is more likely to the "iommu=" parameter
> -- both of them intend to have various sub-boolean types, within which
> some may or may not be independent to each other.
>
> Referring to 'psr' parameter, does below change look good to you?
>
> +### ept (Intel)
> +> `= List of ( pml<boolean> )`
> +
> +> Default: `false`
> +
> +Controls EPT related features. Currently only Page Modification
> Logging (PML) is
> +the controllable feature as boolean type.
> +
> +PML is a new hardware feature in Intel's Broadwell Server and further
> platforms
> +which reduces hypervisor overhead of log-dirty mechanism by
> automatically
> +recording GPAs (guest physical addresses) when guest memory gets
> dirty, and
> +therefore significantly reducing number of EPT violation caused by write
> +protection of guest memory, which is a necessity to implement log-dirty
> +mechanism before PML.
>
> Thanks,
> -Kai

Yep - that looks good.

~Andrew

Re: [v2 00/11] PML (Paging Modification Logging) support

[Kai Huang]

Thanks Tim!

I'll send out the v3 addressing minor comments from Andrew and Jan
regarding to patch 1 & 2.

Thanks,
-Kai

On Thu, Apr 16, 2015 at 10:41 PM, Tim Deegan <tim@xen.org> wrote:
> At 15:03 +0800 on 15 Apr (1429110222), Kai Huang wrote:
>> This v2 patch series was rebased on latest upstream code.
>
> Looks good to me.
>
> Acked-by: Tim Deegan <tim@xen.org>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

Re: [v2 05/11] vmx: add new data structure member to support PML

[Jan Beulich]

>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -70,8 +70,12 @@ struct ept_data {
>      cpumask_var_t synced_mask;
>  };
>  
> +#define _VMX_DOMAIN_PML_ENABLED    0
> +#define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
>  struct vmx_domain {
>      unsigned long apic_access_mfn;
> +    /* VMX_DOMAIN_* */
> +    unsigned long status;

Please us "unsigned int" until 32 bits wouldn't suffice anymore. This
will (on the average) produce slightly smaller code.

Jan

Re: [v2 06/11] vmx: add help functions to support PML

[Jan Beulich]

>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
> +int vmx_vcpu_enable_pml(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +
> +    if ( vmx_vcpu_pml_enabled(v) )
> +        return 0;
> +
> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);

So you latch v->domain into d for this invocation, ...

> +void vmx_vcpu_disable_pml(struct vcpu *v)
> +{
> +    if ( !vmx_vcpu_pml_enabled(v) )
> +        return;
> +
> +    /* Make sure we don't lose any logged GPAs */
> +    vmx_vcpu_flush_pml_buffer(v);
> +
> +    vmx_vmcs_enter(v);
> +
> +    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
> +              v->arch.hvm_vmx.secondary_exec_control);
> +
> +    vmx_vmcs_exit(v);
> +
> +    v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg);

... but not for this one. Please be consistent.

> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
> +{
> +    uint64_t *pml_buf;
> +    unsigned long pml_idx;
> +
> +    ASSERT(vmx_vcpu_pml_enabled(v));
> +
> +    vmx_vmcs_enter(v);
> +
> +    __vmread(GUEST_PML_INDEX, &pml_idx);

Don't you require the vCPU to be non-running or current when you
get here? If so, perhaps add a respective ASSERT()?

> +
> +    /* Do nothing if PML buffer is empty */
> +    if ( pml_idx == (NR_PML_ENTRIES - 1) )
> +        goto out;
> +
> +    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
> +
> +    /*
> +     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is not
> +     * full), and in latter case PML index always points to next available
> +     * entity.
> +     */
> +    if (pml_idx >= NR_PML_ENTRIES)
> +        pml_idx = 0;
> +    else
> +        pml_idx++;
> +
> +    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
> +    {
> +        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;

Blank line here please.

> +        /*
> +         * Need to change type from log-dirty to normal memory for logged GFN.
> +         * hap_track_dirty_vram depends on it to work. And we really only need
> +         * to mark GFNs which hve been successfully changed from log-dirty to
> +         * normal memory to be dirty.
> +         */
> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> +                    p2m_ram_rw) )

Indentation.

> +            paging_mark_gfn_dirty(v->domain, gfn);
> +    }
> +
> +    unmap_domain_page(pml_buf);
> +
> +    /* Reset PML index */
> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
> +
> +out:

Labels indented by at least one space please.

Jan

Re: [v2 10/11] log-dirty: refine common code to support PML

[Jan Beulich]

>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:

> @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>      d->arch.paging.mode |= PG_log_dirty;
>      paging_unlock(d);
>  
> +    /* enable hardware-assisted log-dirty if it is supported */
> +    p2m_enable_hardware_log_dirty(d);

I don't see that you would anywhere avoid setting up software
log-dirty handling - is that on purpose? If so, is there really a
win from adding PML?

>      if ( log_global )
>      {
> -        /* set l1e entries of P2M table to be read-only. */
> +        /*
> +         * switch to log dirty mode, either by setting l1e entries of P2M table
> +         * to be read-only, or via hardware-assisted log-dirty.
> +         */
>          p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);

Or did I miss you changing the behavior of this anywhere (as the
changed comment suggests)?

Jan

Re: [v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Jan Beulich]

>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
> This patch firstly enables EPT A/D bits if PML is used, as PML depends on EPT
> A/D bits to work. A bit is set for all present leaf p2m types, D bit is set for
> all writable types, except log-dirty type.

I think the tying of "leaf" to the A bit part of the description became
stale, as you're now also doing this for non-leaf ones.

Jan

Re: [v2 04/11] vmx: add PML definition and feature detection.

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> Sent: Wednesday, April 15, 2015 3:04 PM
> 
> The patch adds PML definition and feature detection. Note PML won't be
> detected
> if PML is disabled from boot parameter. PML is also disabled in
> construct_vmcs,
> as it will only be enabled when domain is switched to log dirty mode.
> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        | 22 ++++++++++++++++++++++
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  6 ++++++
>  xen/include/asm-x86/hvm/vmx/vmx.h  |  1 +
>  3 files changed, 29 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 4fff46d..d120370 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -141,6 +141,7 @@ static void __init vmx_display_features(void)
>      P(cpu_has_vmx_virtual_intr_delivery, "Virtual Interrupt Delivery");
>      P(cpu_has_vmx_posted_intr_processing, "Posted Interrupt Processing");
>      P(cpu_has_vmx_vmcs_shadowing, "VMCS shadowing");
> +    P(cpu_has_vmx_pml, "Page Modification Logging");
>  #undef P
> 
>      if ( !printed )
> @@ -238,6 +239,8 @@ static int vmx_init_vmcs_config(void)
>              opt |= SECONDARY_EXEC_ENABLE_VPID;
>          if ( opt_unrestricted_guest_enabled )
>              opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;
> +        if ( opt_pml_enabled )
> +            opt |= SECONDARY_EXEC_ENABLE_PML;
> 
>          /*
>           * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
> @@ -284,6 +287,14 @@ static int vmx_init_vmcs_config(void)
>           */
>          if ( !(_vmx_ept_vpid_cap & VMX_VPID_INVVPID_ALL_CONTEXT) )
>              _vmx_secondary_exec_control &=
> ~SECONDARY_EXEC_ENABLE_VPID;
> +
> +        /*
> +         * PML cannot be supported if EPT A/D bits is not supported.
> Actually,
> +         * PML should not be detected if EPT A/D bits is not supported, but
> for
> +         * sure we do the check anyway.
> +         */
> +        if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT) )
> +            _vmx_secondary_exec_control &=
> ~SECONDARY_EXEC_ENABLE_PML;
>      }

the comment is not very clear. I think you just want to say "EPT A/D bit is
required for PML" or no comment at all.

> 
>      if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )
> @@ -304,6 +315,14 @@ static int vmx_init_vmcs_config(void)
>                    SECONDARY_EXEC_UNRESTRICTED_GUEST);
>      }
> 
> +    /* PML cannot be supported if EPT is not used */
> +    if ( !(_vmx_secondary_exec_control &
> SECONDARY_EXEC_ENABLE_EPT) )
> +        _vmx_secondary_exec_control &=
> ~SECONDARY_EXEC_ENABLE_PML;
> +
> +    /* Turn off opt_pml_enabled if PML feature is not present */
> +    if ( !(_vmx_secondary_exec_control &
> SECONDARY_EXEC_ENABLE_PML) )
> +        opt_pml_enabled = 0;
> +
>      if ( (_vmx_secondary_exec_control &
> SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
>            ple_gap == 0 )
>      {
> @@ -1039,6 +1058,9 @@ static int construct_vmcs(struct vcpu *v)
>          __vmwrite(POSTED_INTR_NOTIFICATION_VECTOR,
> posted_intr_vector);
>      }
> 
> +    /* Disable PML anyway here as it will only be enabled in log dirty mode
> */
> +    v->arch.hvm_vmx.secondary_exec_control &=
> ~SECONDARY_EXEC_ENABLE_PML;
> +
>      /* Host data selectors. */
>      __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
>      __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 6fce6aa..f831a78 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -215,6 +215,7 @@ extern u32 vmx_vmentry_control;
>  #define SECONDARY_EXEC_ENABLE_INVPCID           0x00001000
>  #define SECONDARY_EXEC_ENABLE_VMFUNC            0x00002000
>  #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
> +#define SECONDARY_EXEC_ENABLE_PML               0x00020000
>  extern u32 vmx_secondary_exec_control;
> 
>  #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
> @@ -226,6 +227,7 @@ extern u32 vmx_secondary_exec_control;
>  #define VMX_EPT_INVEPT_INSTRUCTION              0x00100000
>  #define VMX_EPT_INVEPT_SINGLE_CONTEXT           0x02000000
>  #define VMX_EPT_INVEPT_ALL_CONTEXT              0x04000000
> +#define VMX_EPT_AD_BIT                          0x00200000
> 
>  #define VMX_MISC_VMWRITE_ALL                    0x20000000
> 
> @@ -274,6 +276,8 @@ extern u32 vmx_secondary_exec_control;
>      (vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
>  #define cpu_has_vmx_vmcs_shadowing \
>      (vmx_secondary_exec_control &
> SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
> +#define cpu_has_vmx_pml \
> +    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
> 
>  #define VMCS_RID_TYPE_MASK              0x80000000
> 
> @@ -318,6 +322,7 @@ enum vmcs_field {
>      GUEST_LDTR_SELECTOR             = 0x0000080c,
>      GUEST_TR_SELECTOR               = 0x0000080e,
>      GUEST_INTR_STATUS               = 0x00000810,
> +    GUEST_PML_INDEX                 = 0x00000812,
>      HOST_ES_SELECTOR                = 0x00000c00,
>      HOST_CS_SELECTOR                = 0x00000c02,
>      HOST_SS_SELECTOR                = 0x00000c04,
> @@ -331,6 +336,7 @@ enum vmcs_field {
>      VM_EXIT_MSR_STORE_ADDR          = 0x00002006,
>      VM_EXIT_MSR_LOAD_ADDR           = 0x00002008,
>      VM_ENTRY_MSR_LOAD_ADDR          = 0x0000200a,
> +    PML_ADDRESS                     = 0x0000200e,
>      TSC_OFFSET                      = 0x00002010,
>      VIRTUAL_APIC_PAGE_ADDR          = 0x00002012,
>      APIC_ACCESS_ADDR                = 0x00002014,
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
> b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 91c5e18..50f1bfc 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -185,6 +185,7 @@ static inline unsigned long pi_get_pir(struct pi_desc
> *pi_desc, int group)
>  #define EXIT_REASON_XSETBV              55
>  #define EXIT_REASON_APIC_WRITE          56
>  #define EXIT_REASON_INVPCID             58
> +#define EXIT_REASON_PML_FULL            62
> 
>  /*
>   * Interruption-information format
> --
> 2.1.0

Re: [v2 05/11] vmx: add new data structure member to support PML

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> Sent: Wednesday, April 15, 2015 3:04 PM
> 
> A new 4K page pointer is added to arch_vmx_struct as PML buffer for vcpu.
> And a
> new 'status' field is added to vmx_domain to indicate whether PML is enabled
> for
> the domain or not. The 'status' field also can be used for further similiar
> purpose.

not sure about the last sentence. what's the similar purpose to "whether PML
is enabled"? :-)

> 
> Note both new members don't have to be initialized to zero explicitly as both
> vcpu and domain structure are zero-ed when they are created.

no initialization in this patch, so why explaining it here?

> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
>  xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index f831a78..2c679ac 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -70,8 +70,12 @@ struct ept_data {
>      cpumask_var_t synced_mask;
>  };
> 
> +#define _VMX_DOMAIN_PML_ENABLED    0
> +#define VMX_DOMAIN_PML_ENABLED     (1ul <<
> _VMX_DOMAIN_PML_ENABLED)
>  struct vmx_domain {
>      unsigned long apic_access_mfn;
> +    /* VMX_DOMAIN_* */
> +    unsigned long status;
>  };
> 
>  struct pi_desc {
> @@ -142,6 +146,9 @@ struct arch_vmx_struct {
>      /* Bitmap to control vmexit policy for Non-root VMREAD/VMWRITE */
>      struct page_info     *vmread_bitmap;
>      struct page_info     *vmwrite_bitmap;
> +
> +#define NR_PML_ENTRIES   512
> +    struct page_info     *pml_pg;

move the macro out of the structure. and is pml_buffer or pml_buf more clear?

>  };
> 
>  int vmx_create_vmcs(struct vcpu *v);
> --
> 2.1.0

Re: [v2 06/11] vmx: add help functions to support PML

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> Sent: Wednesday, April 15, 2015 3:04 PM
> 
> This patch adds help functions to enable/disable PML, and flush PML buffer for
> single vcpu and particular domain for further use.
> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        | 178
> +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
>  2 files changed, 187 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d120370..d3cb50f 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1328,6 +1328,184 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v,
> u8 vector)
>                  &v->arch.hvm_vmx.eoi_exitmap_changed);
>  }
> 
> +bool_t vmx_vcpu_pml_enabled(const struct vcpu *v)
> +{
> +    return !!(v->arch.hvm_vmx.secondary_exec_control &
> +              SECONDARY_EXEC_ENABLE_PML);
> +}
> +
> +int vmx_vcpu_enable_pml(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +
> +    if ( vmx_vcpu_pml_enabled(v) )
> +        return 0;
> +
> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
> +    if ( !v->arch.hvm_vmx.pml_pg )
> +        return -ENOMEM;
> +
> +    vmx_vmcs_enter(v);
> +
> +    __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) <<
> PAGE_SHIFT);
> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
> +
> +    v->arch.hvm_vmx.secondary_exec_control |=
> SECONDARY_EXEC_ENABLE_PML;
> +
> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
> +              v->arch.hvm_vmx.secondary_exec_control);
> +
> +    vmx_vmcs_exit(v);
> +
> +    return 0;
> +}
> +
> +void vmx_vcpu_disable_pml(struct vcpu *v)
> +{
> +    if ( !vmx_vcpu_pml_enabled(v) )
> +        return;
> +
> +    /* Make sure we don't lose any logged GPAs */
> +    vmx_vcpu_flush_pml_buffer(v);
> +
> +    vmx_vmcs_enter(v);
> +
> +    v->arch.hvm_vmx.secondary_exec_control &=
> ~SECONDARY_EXEC_ENABLE_PML;
> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
> +              v->arch.hvm_vmx.secondary_exec_control);
> +
> +    vmx_vmcs_exit(v);
> +
> +    v->domain->arch.paging.free_page(v->domain,
> v->arch.hvm_vmx.pml_pg);
> +    v->arch.hvm_vmx.pml_pg = NULL;
> +}
> +
> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
> +{
> +    uint64_t *pml_buf;
> +    unsigned long pml_idx;
> +
> +    ASSERT(vmx_vcpu_pml_enabled(v));
> +
> +    vmx_vmcs_enter(v);
> +
> +    __vmread(GUEST_PML_INDEX, &pml_idx);
> +
> +    /* Do nothing if PML buffer is empty */
> +    if ( pml_idx == (NR_PML_ENTRIES - 1) )
> +        goto out;
> +
> +    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
> +
> +    /*
> +     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is
> not

0~NR_PML_ENTRIES-1

> +     * full), and in latter case PML index always points to next available
> +     * entity.
> +     */
> +    if (pml_idx >= NR_PML_ENTRIES)
> +        pml_idx = 0;
> +    else
> +        pml_idx++;
> +
> +    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
> +    {
> +        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
> +        /*
> +         * Need to change type from log-dirty to normal memory for
> logged GFN.
> +         * hap_track_dirty_vram depends on it to work. And we really only
> need
> +         * to mark GFNs which hve been successfully changed from
> log-dirty to
> +         * normal memory to be dirty.
> +         */
> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> +                    p2m_ram_rw) )
> +            paging_mark_gfn_dirty(v->domain, gfn);

Should we handle error from p2m_change_type_one and consequently
making this flush function non-void?

> +    }
> +
> +    unmap_domain_page(pml_buf);
> +
> +    /* Reset PML index */
> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);

Like Jan pointed out an assertion of vcpu status is required here otherwise
blindly reset PML index may race with new logged entries from a running
vcpu.

> +
> +out:
> +    vmx_vmcs_exit(v);
> +}
> +
> +bool_t vmx_domain_pml_enabled(const struct domain *d)
> +{
> +    return !!(d->arch.hvm_domain.vmx.status &
> VMX_DOMAIN_PML_ENABLED);
> +}
> +
> +/*
> + * This function enables PML for particular domain. It should be called when
> + * domain is paused.
> + *
> + * PML needs to be enabled globally for all vcpus of the domain, as PML
> buffer
> + * and PML index are pre-vcpu, but EPT table is shared by vcpus, therefore
> + * enabling PML on partial vcpus won't work.
> + */
> +int vmx_domain_enable_pml(struct domain *d)
> +{
> +    struct vcpu *v;
> +    int rc;
> +
> +    ASSERT(atomic_read(&d->pause_count));
> +
> +    if ( vmx_domain_pml_enabled(d) )
> +        return 0;
> +
> +    for_each_vcpu( d, v )
> +        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
> +            goto error;
> +
> +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;

I didn't see how this domain-wise flag is useful. Or if we really
want to go this way, you also need to clear this flag if vcpu pml
enable is failed in vcpu hotplug phase, since the flag itself means
all vcpus of the domain so we must keep this intention in all
places.

> +
> +    return 0;
> +
> +error:
> +    for_each_vcpu( d, v )
> +        if ( vmx_vcpu_pml_enabled(v) )
> +            vmx_vcpu_disable_pml(v);
> +    return rc;
> +}
> +
> +/*
> + * Disable PML for particular domain. Called when domain is paused.
> + *
> + * The same as enabling PML for domain, disabling PML should be done for
> all
> + * vcpus at once.
> + */
> +void vmx_domain_disable_pml(struct domain *d)
> +{
> +    struct vcpu *v;
> +
> +    ASSERT(atomic_read(&d->pause_count));
> +
> +    if ( !vmx_domain_pml_enabled(d) )
> +        return;
> +
> +    for_each_vcpu( d, v )
> +        vmx_vcpu_disable_pml(v);
> +
> +    d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED;
> +}
> +
> +/*
> + * Flush PML buffer of all vcpus, and update the logged dirty pages to
> log-dirty
> + * radix tree. Called when domain is paused.
> + */
> +void vmx_domain_flush_pml_buffers(struct domain *d)
> +{
> +    struct vcpu *v;
> +
> +    ASSERT(atomic_read(&d->pause_count));
> +
> +    if ( !vmx_domain_pml_enabled(d) )
> +        return;
> +
> +    for_each_vcpu( d, v )
> +        vmx_vcpu_flush_pml_buffer(v);
> +}
> +
>  int vmx_create_vmcs(struct vcpu *v)
>  {
>      struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 2c679ac..ceb09bf 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -498,6 +498,15 @@ static inline int vmx_add_host_load_msr(u32 msr)
> 
>  DECLARE_PER_CPU(bool_t, vmxon);
> 
> +bool_t vmx_vcpu_pml_enabled(const struct vcpu *v);
> +int vmx_vcpu_enable_pml(struct vcpu *v);
> +void vmx_vcpu_disable_pml(struct vcpu *v);
> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v);
> +bool_t vmx_domain_pml_enabled(const struct domain *d);
> +int vmx_domain_enable_pml(struct domain *d);
> +void vmx_domain_disable_pml(struct domain *d);
> +void vmx_domain_flush_pml_buffers(struct domain *d);
> +
>  #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
> 
>  /*
> --
> 2.1.0

Re: [v2 06/11] vmx: add help functions to support PML

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Friday, April 17, 2015 6:57 AM
> > +int vmx_domain_enable_pml(struct domain *d)
> > +{
> > +    struct vcpu *v;
> > +    int rc;
> > +
> > +    ASSERT(atomic_read(&d->pause_count));
> > +
> > +    if ( vmx_domain_pml_enabled(d) )
> > +        return 0;
> > +
> > +    for_each_vcpu( d, v )
> > +        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
> > +            goto error;
> > +
> > +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
> 
> I didn't see how this domain-wise flag is useful. Or if we really
> want to go this way, you also need to clear this flag if vcpu pml
> enable is failed in vcpu hotplug phase, since the flag itself means
> all vcpus of the domain so we must keep this intention in all
> places.
> 

forget this comment. Your [08/11] did it. :-)

Thanks
Kevin

Re: [v2 10/11] log-dirty: refine common code to support PML

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, April 16, 2015 11:52 PM
> 
> >>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
> 
> > @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d,
> bool_t log_global)
> >      d->arch.paging.mode |= PG_log_dirty;
> >      paging_unlock(d);
> >
> > +    /* enable hardware-assisted log-dirty if it is supported */
> > +    p2m_enable_hardware_log_dirty(d);
> 
> I don't see that you would anywhere avoid setting up software
> log-dirty handling - is that on purpose? If so, is there really a
> win from adding PML?
> 
> >      if ( log_global )
> >      {
> > -        /* set l1e entries of P2M table to be read-only. */
> > +        /*
> > +         * switch to log dirty mode, either by setting l1e entries of P2M
> table
> > +         * to be read-only, or via hardware-assisted log-dirty.
> > +         */
> >          p2m_change_entry_type_global(d, p2m_ram_rw,
> p2m_ram_logdirty);
> 
> Or did I miss you changing the behavior of this anywhere (as the
> changed comment suggests)?
> 

just found behavior is changed in [11/11]. :-)

Thanks
Kevin

Re: [v2 06/11] vmx: add help functions to support PML

[Tim Deegan]

At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:

> > From: Kai Huang [mailto:kai.huang@linux.intel.com]
> > +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> > +                    p2m_ram_rw) )
> > +            paging_mark_gfn_dirty(v->domain, gfn);
> 
> Should we handle error from p2m_change_type_one and consequently
> making this flush function non-void?

I don't think we need to return an error, but we should probably 
call mark_dirty here for anything except -EBUSY.

> > +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
> 
> I didn't see how this domain-wise flag is useful. Or if we really
> want to go this way, you also need to clear this flag if vcpu pml
> enable is failed in vcpu hotplug phase, since the flag itself means
> all vcpus of the domain so we must keep this intention in all
> places.

IIUC we need this flag so that we know whether to enable PML on any
new vcpus.  If we fail to enable PML on hotplug, we fail the hotplug
(like for other vcpu bringup errors) so the invariant still holds.

Cheers,

Tim.

Re: [v2 05/11] vmx: add new data structure member to support PML

[Kai Huang]

On 04/16/2015 11:33 PM, Jan Beulich wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -70,8 +70,12 @@ struct ept_data {
>>       cpumask_var_t synced_mask;
>>   };
>>   
>> +#define _VMX_DOMAIN_PML_ENABLED    0
>> +#define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
>>   struct vmx_domain {
>>       unsigned long apic_access_mfn;
>> +    /* VMX_DOMAIN_* */
>> +    unsigned long status;
> Please us "unsigned int" until 32 bits wouldn't suffice anymore. This
> will (on the average) produce slightly smaller code.
Sure.

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 04/11] vmx: add PML definition and feature detection.

[Kai Huang]

On 04/17/2015 06:35 AM, Tian, Kevin wrote:
>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>> Sent: Wednesday, April 15, 2015 3:04 PM
>>
>> The patch adds PML definition and feature detection. Note PML won't be
>> detected
>> if PML is disabled from boot parameter. PML is also disabled in
>> construct_vmcs,
>> as it will only be enabled when domain is switched to log dirty mode.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> ---
>>   xen/arch/x86/hvm/vmx/vmcs.c        | 22 ++++++++++++++++++++++
>>   xen/include/asm-x86/hvm/vmx/vmcs.h |  6 ++++++
>>   xen/include/asm-x86/hvm/vmx/vmx.h  |  1 +
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 4fff46d..d120370 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -141,6 +141,7 @@ static void __init vmx_display_features(void)
>>       P(cpu_has_vmx_virtual_intr_delivery, "Virtual Interrupt Delivery");
>>       P(cpu_has_vmx_posted_intr_processing, "Posted Interrupt Processing");
>>       P(cpu_has_vmx_vmcs_shadowing, "VMCS shadowing");
>> +    P(cpu_has_vmx_pml, "Page Modification Logging");
>>   #undef P
>>
>>       if ( !printed )
>> @@ -238,6 +239,8 @@ static int vmx_init_vmcs_config(void)
>>               opt |= SECONDARY_EXEC_ENABLE_VPID;
>>           if ( opt_unrestricted_guest_enabled )
>>               opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;
>> +        if ( opt_pml_enabled )
>> +            opt |= SECONDARY_EXEC_ENABLE_PML;
>>
>>           /*
>>            * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
>> @@ -284,6 +287,14 @@ static int vmx_init_vmcs_config(void)
>>            */
>>           if ( !(_vmx_ept_vpid_cap & VMX_VPID_INVVPID_ALL_CONTEXT) )
>>               _vmx_secondary_exec_control &=
>> ~SECONDARY_EXEC_ENABLE_VPID;
>> +
>> +        /*
>> +         * PML cannot be supported if EPT A/D bits is not supported.
>> Actually,
>> +         * PML should not be detected if EPT A/D bits is not supported, but
>> for
>> +         * sure we do the check anyway.
>> +         */
>> +        if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT) )
>> +            _vmx_secondary_exec_control &=
>> ~SECONDARY_EXEC_ENABLE_PML;
>>       }
> the comment is not very clear. I think you just want to say "EPT A/D bit is
> required for PML" or no comment at all.
Sure, I'll change it to "EPT A/D bit is required for PML".

Thanks,
-Kai
>
>>       if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )
>> @@ -304,6 +315,14 @@ static int vmx_init_vmcs_config(void)
>>                     SECONDARY_EXEC_UNRESTRICTED_GUEST);
>>       }
>>
>> +    /* PML cannot be supported if EPT is not used */
>> +    if ( !(_vmx_secondary_exec_control &
>> SECONDARY_EXEC_ENABLE_EPT) )
>> +        _vmx_secondary_exec_control &=
>> ~SECONDARY_EXEC_ENABLE_PML;
>> +
>> +    /* Turn off opt_pml_enabled if PML feature is not present */
>> +    if ( !(_vmx_secondary_exec_control &
>> SECONDARY_EXEC_ENABLE_PML) )
>> +        opt_pml_enabled = 0;
>> +
>>       if ( (_vmx_secondary_exec_control &
>> SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
>>             ple_gap == 0 )
>>       {
>> @@ -1039,6 +1058,9 @@ static int construct_vmcs(struct vcpu *v)
>>           __vmwrite(POSTED_INTR_NOTIFICATION_VECTOR,
>> posted_intr_vector);
>>       }
>>
>> +    /* Disable PML anyway here as it will only be enabled in log dirty mode
>> */
>> +    v->arch.hvm_vmx.secondary_exec_control &=
>> ~SECONDARY_EXEC_ENABLE_PML;
>> +
>>       /* Host data selectors. */
>>       __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
>>       __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index 6fce6aa..f831a78 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -215,6 +215,7 @@ extern u32 vmx_vmentry_control;
>>   #define SECONDARY_EXEC_ENABLE_INVPCID           0x00001000
>>   #define SECONDARY_EXEC_ENABLE_VMFUNC            0x00002000
>>   #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
>> +#define SECONDARY_EXEC_ENABLE_PML               0x00020000
>>   extern u32 vmx_secondary_exec_control;
>>
>>   #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
>> @@ -226,6 +227,7 @@ extern u32 vmx_secondary_exec_control;
>>   #define VMX_EPT_INVEPT_INSTRUCTION              0x00100000
>>   #define VMX_EPT_INVEPT_SINGLE_CONTEXT           0x02000000
>>   #define VMX_EPT_INVEPT_ALL_CONTEXT              0x04000000
>> +#define VMX_EPT_AD_BIT                          0x00200000
>>
>>   #define VMX_MISC_VMWRITE_ALL                    0x20000000
>>
>> @@ -274,6 +276,8 @@ extern u32 vmx_secondary_exec_control;
>>       (vmx_pin_based_exec_control & PIN_BASED_POSTED_INTERRUPT)
>>   #define cpu_has_vmx_vmcs_shadowing \
>>       (vmx_secondary_exec_control &
>> SECONDARY_EXEC_ENABLE_VMCS_SHADOWING)
>> +#define cpu_has_vmx_pml \
>> +    (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
>>
>>   #define VMCS_RID_TYPE_MASK              0x80000000
>>
>> @@ -318,6 +322,7 @@ enum vmcs_field {
>>       GUEST_LDTR_SELECTOR             = 0x0000080c,
>>       GUEST_TR_SELECTOR               = 0x0000080e,
>>       GUEST_INTR_STATUS               = 0x00000810,
>> +    GUEST_PML_INDEX                 = 0x00000812,
>>       HOST_ES_SELECTOR                = 0x00000c00,
>>       HOST_CS_SELECTOR                = 0x00000c02,
>>       HOST_SS_SELECTOR                = 0x00000c04,
>> @@ -331,6 +336,7 @@ enum vmcs_field {
>>       VM_EXIT_MSR_STORE_ADDR          = 0x00002006,
>>       VM_EXIT_MSR_LOAD_ADDR           = 0x00002008,
>>       VM_ENTRY_MSR_LOAD_ADDR          = 0x0000200a,
>> +    PML_ADDRESS                     = 0x0000200e,
>>       TSC_OFFSET                      = 0x00002010,
>>       VIRTUAL_APIC_PAGE_ADDR          = 0x00002012,
>>       APIC_ACCESS_ADDR                = 0x00002014,
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h
>> b/xen/include/asm-x86/hvm/vmx/vmx.h
>> index 91c5e18..50f1bfc 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -185,6 +185,7 @@ static inline unsigned long pi_get_pir(struct pi_desc
>> *pi_desc, int group)
>>   #define EXIT_REASON_XSETBV              55
>>   #define EXIT_REASON_APIC_WRITE          56
>>   #define EXIT_REASON_INVPCID             58
>> +#define EXIT_REASON_PML_FULL            62
>>
>>   /*
>>    * Interruption-information format
>> --
>> 2.1.0
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 05/11] vmx: add new data structure member to support PML

[Kai Huang]

On 04/17/2015 06:39 AM, Tian, Kevin wrote:
>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>> Sent: Wednesday, April 15, 2015 3:04 PM
>>
>> A new 4K page pointer is added to arch_vmx_struct as PML buffer for vcpu.
>> And a
>> new 'status' field is added to vmx_domain to indicate whether PML is enabled
>> for
>> the domain or not. The 'status' field also can be used for further similiar
>> purpose.
> not sure about the last sentence. what's the similar purpose to "whether PML
> is enabled"? :-)
I mean potentially there might be such feature in the future, and I 
can't give you an example right now. If you are just commenting the 
description here but fine with the current code, I can remove that last 
sentence if you like. Or do you suggest to just use a "bool_t 
pml_enabled"? I am fine with both, but looks there's no objection from 
others so I intend to keep it as 'unsigned int status', if you agree.

>
>> Note both new members don't have to be initialized to zero explicitly as both
>> vcpu and domain structure are zero-ed when they are created.
> no initialization in this patch, so why explaining it here?
OK. Looks it's a common sense to all of you so I'll just remove this 
sentence.

>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> ---
>>   xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index f831a78..2c679ac 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -70,8 +70,12 @@ struct ept_data {
>>       cpumask_var_t synced_mask;
>>   };
>>
>> +#define _VMX_DOMAIN_PML_ENABLED    0
>> +#define VMX_DOMAIN_PML_ENABLED     (1ul <<
>> _VMX_DOMAIN_PML_ENABLED)
>>   struct vmx_domain {
>>       unsigned long apic_access_mfn;
>> +    /* VMX_DOMAIN_* */
>> +    unsigned long status;
>>   };
>>
>>   struct pi_desc {
>> @@ -142,6 +146,9 @@ struct arch_vmx_struct {
>>       /* Bitmap to control vmexit policy for Non-root VMREAD/VMWRITE */
>>       struct page_info     *vmread_bitmap;
>>       struct page_info     *vmwrite_bitmap;
>> +
>> +#define NR_PML_ENTRIES   512
>> +    struct page_info     *pml_pg;
> move the macro out of the structure.
OK. I will move it just above the declaration of struct arch_vmx_struct.

> and is pml_buffer or pml_buf more clear?

To me pml_buffer or pml_buf is more likely a virtual address you can 
access the buffer directly, while pml_pg indicates it's a pointer of 
struct page_info. If you you look at patch 6, you can find statements like:

     uint64_t *pml_buf;

     pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);

So I intend to keep it.

Thanks,
-Kai
>
>>   };
>>
>>   int vmx_create_vmcs(struct vcpu *v);
>> --
>> 2.1.0
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Kai Huang]

On 04/16/2015 11:54 PM, Jan Beulich wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> This patch firstly enables EPT A/D bits if PML is used, as PML depends on EPT
>> A/D bits to work. A bit is set for all present leaf p2m types, D bit is set for
>> all writable types, except log-dirty type.
> I think the tying of "leaf" to the A bit part of the description became
> stale, as you're now also doing this for non-leaf ones.
You are right. How about just "A bit is set for all present p2m types, ..."?

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 10/11] log-dirty: refine common code to support PML

[Kai Huang]

On 04/16/2015 11:51 PM, Jan Beulich wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>>       d->arch.paging.mode |= PG_log_dirty;
>>       paging_unlock(d);
>>   
>> +    /* enable hardware-assisted log-dirty if it is supported */
>> +    p2m_enable_hardware_log_dirty(d);
> I don't see that you would anywhere avoid setting up software
> log-dirty handling - is that on purpose? If so, is there really a
> win from adding PML?
>
>>       if ( log_global )
>>       {
>> -        /* set l1e entries of P2M table to be read-only. */
>> +        /*
>> +         * switch to log dirty mode, either by setting l1e entries of P2M table
>> +         * to be read-only, or via hardware-assisted log-dirty.
>> +         */
>>           p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
> Or did I miss you changing the behavior of this anywhere (as the
> changed comment suggests)?
Both of your comments are done in patch 11.

And I think even without patch 11, there's no harm with this patch as 
p2m_enable_hardware_log_dirty will essentially do nothing and write 
protection will be used for log-dirty :)

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 10/11] log-dirty: refine common code to support PML

[Kai Huang]

On 04/17/2015 07:07 AM, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, April 16, 2015 11:52 PM
>>
>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>> @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d,
>> bool_t log_global)
>>>       d->arch.paging.mode |= PG_log_dirty;
>>>       paging_unlock(d);
>>>
>>> +    /* enable hardware-assisted log-dirty if it is supported */
>>> +    p2m_enable_hardware_log_dirty(d);
>> I don't see that you would anywhere avoid setting up software
>> log-dirty handling - is that on purpose? If so, is there really a
>> win from adding PML?
>>
>>>       if ( log_global )
>>>       {
>>> -        /* set l1e entries of P2M table to be read-only. */
>>> +        /*
>>> +         * switch to log dirty mode, either by setting l1e entries of P2M
>> table
>>> +         * to be read-only, or via hardware-assisted log-dirty.
>>> +         */
>>>           p2m_change_entry_type_global(d, p2m_ram_rw,
>> p2m_ram_logdirty);
>>
>> Or did I miss you changing the behavior of this anywhere (as the
>> changed comment suggests)?
>>
> just found behavior is changed in [11/11]. :-)
Yes.

Thanks,
-Kai
>
> Thanks
> Kevin
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> +int vmx_vcpu_enable_pml(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +
>> +    if ( vmx_vcpu_pml_enabled(v) )
>> +        return 0;
>> +
>> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
> So you latch v->domain into d for this invocation, ...
>
>> +void vmx_vcpu_disable_pml(struct vcpu *v)
>> +{
>> +    if ( !vmx_vcpu_pml_enabled(v) )
>> +        return;
>> +
>> +    /* Make sure we don't lose any logged GPAs */
>> +    vmx_vcpu_flush_pml_buffer(v);
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>> +              v->arch.hvm_vmx.secondary_exec_control);
>> +
>> +    vmx_vmcs_exit(v);
>> +
>> +    v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg);
> ... but not for this one. Please be consistent.
Hmm. My bad. I'll use v->domain in both function.

>
>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>> +{
>> +    uint64_t *pml_buf;
>> +    unsigned long pml_idx;
>> +
>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
> Don't you require the vCPU to be non-running or current when you
> get here? If so, perhaps add a respective ASSERT()?
Yes an ASSERT would be better.

v->pause_count will be increased if vcpu is kicked out by domain_pause 
explicitly, but looks the same thing won't be done if vcpu is kicked out 
by PML buffer full VMEXIT. So should the ASSERT be done like below?

ASSERT(atomic_read(&v->pause_count) || (v == current));

>
>> +
>> +    /* Do nothing if PML buffer is empty */
>> +    if ( pml_idx == (NR_PML_ENTRIES - 1) )
>> +        goto out;
>> +
>> +    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
>> +
>> +    /*
>> +     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is not
>> +     * full), and in latter case PML index always points to next available
>> +     * entity.
>> +     */
>> +    if (pml_idx >= NR_PML_ENTRIES)
>> +        pml_idx = 0;
>> +    else
>> +        pml_idx++;
>> +
>> +    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
>> +    {
>> +        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
> Blank line here please.
Will do.

>
>> +        /*
>> +         * Need to change type from log-dirty to normal memory for logged GFN.
>> +         * hap_track_dirty_vram depends on it to work. And we really only need
>> +         * to mark GFNs which hve been successfully changed from log-dirty to
>> +         * normal memory to be dirty.
>> +         */
>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>> +                    p2m_ram_rw) )
> Indentation.
To be where exactly? Sorry I didn't find an example to refer in such case.

>
>> +            paging_mark_gfn_dirty(v->domain, gfn);
>> +    }
>> +
>> +    unmap_domain_page(pml_buf);
>> +
>> +    /* Reset PML index */
>> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
>> +
>> +out:
> Labels indented by at least one space please.
OK. I'll put one space before the "out:" label.

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 06:57 AM, Tian, Kevin wrote:
>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>> Sent: Wednesday, April 15, 2015 3:04 PM
>>
>> This patch adds help functions to enable/disable PML, and flush PML buffer for
>> single vcpu and particular domain for further use.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> ---
>>   xen/arch/x86/hvm/vmx/vmcs.c        | 178
>> +++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
>>   2 files changed, 187 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index d120370..d3cb50f 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1328,6 +1328,184 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v,
>> u8 vector)
>>                   &v->arch.hvm_vmx.eoi_exitmap_changed);
>>   }
>>
>> +bool_t vmx_vcpu_pml_enabled(const struct vcpu *v)
>> +{
>> +    return !!(v->arch.hvm_vmx.secondary_exec_control &
>> +              SECONDARY_EXEC_ENABLE_PML);
>> +}
>> +
>> +int vmx_vcpu_enable_pml(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +
>> +    if ( vmx_vcpu_pml_enabled(v) )
>> +        return 0;
>> +
>> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
>> +    if ( !v->arch.hvm_vmx.pml_pg )
>> +        return -ENOMEM;
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) <<
>> PAGE_SHIFT);
>> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
>> +
>> +    v->arch.hvm_vmx.secondary_exec_control |=
>> SECONDARY_EXEC_ENABLE_PML;
>> +
>> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>> +              v->arch.hvm_vmx.secondary_exec_control);
>> +
>> +    vmx_vmcs_exit(v);
>> +
>> +    return 0;
>> +}
>> +
>> +void vmx_vcpu_disable_pml(struct vcpu *v)
>> +{
>> +    if ( !vmx_vcpu_pml_enabled(v) )
>> +        return;
>> +
>> +    /* Make sure we don't lose any logged GPAs */
>> +    vmx_vcpu_flush_pml_buffer(v);
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    v->arch.hvm_vmx.secondary_exec_control &=
>> ~SECONDARY_EXEC_ENABLE_PML;
>> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>> +              v->arch.hvm_vmx.secondary_exec_control);
>> +
>> +    vmx_vmcs_exit(v);
>> +
>> +    v->domain->arch.paging.free_page(v->domain,
>> v->arch.hvm_vmx.pml_pg);
>> +    v->arch.hvm_vmx.pml_pg = NULL;
>> +}
>> +
>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>> +{
>> +    uint64_t *pml_buf;
>> +    unsigned long pml_idx;
>> +
>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>> +
>> +    /* Do nothing if PML buffer is empty */
>> +    if ( pml_idx == (NR_PML_ENTRIES - 1) )
>> +        goto out;
>> +
>> +    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
>> +
>> +    /*
>> +     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is
>> not
> 0~NR_PML_ENTRIES-1
Will do.

>
>> +     * full), and in latter case PML index always points to next available
>> +     * entity.
>> +     */
>> +    if (pml_idx >= NR_PML_ENTRIES)
>> +        pml_idx = 0;
>> +    else
>> +        pml_idx++;
>> +
>> +    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
>> +    {
>> +        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
>> +        /*
>> +         * Need to change type from log-dirty to normal memory for
>> logged GFN.
>> +         * hap_track_dirty_vram depends on it to work. And we really only
>> need
>> +         * to mark GFNs which hve been successfully changed from
>> log-dirty to
>> +         * normal memory to be dirty.
>> +         */
>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>> +                    p2m_ram_rw) )
>> +            paging_mark_gfn_dirty(v->domain, gfn);
> Should we handle error from p2m_change_type_one and consequently
> making this flush function non-void?
>
>> +    }
>> +
>> +    unmap_domain_page(pml_buf);
>> +
>> +    /* Reset PML index */
>> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
> Like Jan pointed out an assertion of vcpu status is required here otherwise
> blindly reset PML index may race with new logged entries from a running
> vcpu.
Yeah will do.

Thanks,
-Kai
>
>> +
>> +out:
>> +    vmx_vmcs_exit(v);
>> +}
>> +
>> +bool_t vmx_domain_pml_enabled(const struct domain *d)
>> +{
>> +    return !!(d->arch.hvm_domain.vmx.status &
>> VMX_DOMAIN_PML_ENABLED);
>> +}
>> +
>> +/*
>> + * This function enables PML for particular domain. It should be called when
>> + * domain is paused.
>> + *
>> + * PML needs to be enabled globally for all vcpus of the domain, as PML
>> buffer
>> + * and PML index are pre-vcpu, but EPT table is shared by vcpus, therefore
>> + * enabling PML on partial vcpus won't work.
>> + */
>> +int vmx_domain_enable_pml(struct domain *d)
>> +{
>> +    struct vcpu *v;
>> +    int rc;
>> +
>> +    ASSERT(atomic_read(&d->pause_count));
>> +
>> +    if ( vmx_domain_pml_enabled(d) )
>> +        return 0;
>> +
>> +    for_each_vcpu( d, v )
>> +        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
>> +            goto error;
>> +
>> +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
> I didn't see how this domain-wise flag is useful. Or if we really
> want to go this way, you also need to clear this flag if vcpu pml
> enable is failed in vcpu hotplug phase, since the flag itself means
> all vcpus of the domain so we must keep this intention in all
> places.
>
>> +
>> +    return 0;
>> +
>> +error:
>> +    for_each_vcpu( d, v )
>> +        if ( vmx_vcpu_pml_enabled(v) )
>> +            vmx_vcpu_disable_pml(v);
>> +    return rc;
>> +}
>> +
>> +/*
>> + * Disable PML for particular domain. Called when domain is paused.
>> + *
>> + * The same as enabling PML for domain, disabling PML should be done for
>> all
>> + * vcpus at once.
>> + */
>> +void vmx_domain_disable_pml(struct domain *d)
>> +{
>> +    struct vcpu *v;
>> +
>> +    ASSERT(atomic_read(&d->pause_count));
>> +
>> +    if ( !vmx_domain_pml_enabled(d) )
>> +        return;
>> +
>> +    for_each_vcpu( d, v )
>> +        vmx_vcpu_disable_pml(v);
>> +
>> +    d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED;
>> +}
>> +
>> +/*
>> + * Flush PML buffer of all vcpus, and update the logged dirty pages to
>> log-dirty
>> + * radix tree. Called when domain is paused.
>> + */
>> +void vmx_domain_flush_pml_buffers(struct domain *d)
>> +{
>> +    struct vcpu *v;
>> +
>> +    ASSERT(atomic_read(&d->pause_count));
>> +
>> +    if ( !vmx_domain_pml_enabled(d) )
>> +        return;
>> +
>> +    for_each_vcpu( d, v )
>> +        vmx_vcpu_flush_pml_buffer(v);
>> +}
>> +
>>   int vmx_create_vmcs(struct vcpu *v)
>>   {
>>       struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index 2c679ac..ceb09bf 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -498,6 +498,15 @@ static inline int vmx_add_host_load_msr(u32 msr)
>>
>>   DECLARE_PER_CPU(bool_t, vmxon);
>>
>> +bool_t vmx_vcpu_pml_enabled(const struct vcpu *v);
>> +int vmx_vcpu_enable_pml(struct vcpu *v);
>> +void vmx_vcpu_disable_pml(struct vcpu *v);
>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v);
>> +bool_t vmx_domain_pml_enabled(const struct domain *d);
>> +int vmx_domain_enable_pml(struct domain *d);
>> +void vmx_domain_disable_pml(struct domain *d);
>> +void vmx_domain_flush_pml_buffers(struct domain *d);
>> +
>>   #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
>>
>>   /*
>> --
>> 2.1.0
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 08:10 AM, Tim Deegan wrote:
> At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
>
>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>>> +                    p2m_ram_rw) )
>>> +            paging_mark_gfn_dirty(v->domain, gfn);
>> Should we handle error from p2m_change_type_one and consequently
>> making this flush function non-void?
> I don't think we need to return an error, but we should probably
> call mark_dirty here for anything except -EBUSY.
Hi Kevin, Tim,

My intention here is to rule out the GFN with original type that is not 
p2m_ram_logdirty, though with patch 11 it's not likely have such GFN 
logged.

Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty, 
so I think it might be OK to do as Tim suggested.

But given the same thing has already been done in hap_track_dirty_vram 
(hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in 
bitmap with !p2m_change_type_one is true), and in EPT violation 
(p2m_change_type_one is called unconditionally without checking return 
value), I think it should be safe to do the current code here.

What's your comments?

Thanks,
-Kai

>
>>> +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
>> I didn't see how this domain-wise flag is useful. Or if we really
>> want to go this way, you also need to clear this flag if vcpu pml
>> enable is failed in vcpu hotplug phase, since the flag itself means
>> all vcpus of the domain so we must keep this intention in all
>> places.
> IIUC we need this flag so that we know whether to enable PML on any
> new vcpus.  If we fail to enable PML on hotplug, we fail the hotplug
> (like for other vcpu bringup errors) so the invariant still holds.
>
> Cheers,
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Jan Beulich]

>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>> +{
>>> +    uint64_t *pml_buf;
>>> +    unsigned long pml_idx;
>>> +
>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>> +
>>> +    vmx_vmcs_enter(v);
>>> +
>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>> Don't you require the vCPU to be non-running or current when you
>> get here? If so, perhaps add a respective ASSERT()?
> Yes an ASSERT would be better.
> 
> v->pause_count will be increased if vcpu is kicked out by domain_pause 
> explicitly, but looks the same thing won't be done if vcpu is kicked out 
> by PML buffer full VMEXIT. So should the ASSERT be done like below?
> 
> ASSERT(atomic_read(&v->pause_count) || (v == current));

For one I'd reverse the two parts. And then I think pause count
being non-zero is not a sufficient condition - if a non-synchronous
pause was issued against the vCPU it may still be running. I'd
suggest !vcpu_runnable(v) && !v->is_running, possibly with the
pause count check instead of the runnable one if the only
permitted case where v != current requires the vCPU to be
paused.

>>> +        /*
>>> +         * Need to change type from log-dirty to normal memory for logged GFN.
>>> +         * hap_track_dirty_vram depends on it to work. And we really only need
>>> +         * to mark GFNs which hve been successfully changed from log-dirty to
>>> +         * normal memory to be dirty.
>>> +         */
>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>>> +                    p2m_ram_rw) )
>> Indentation.
> To be where exactly? Sorry I didn't find an example to refer in such case.

p2m_ram_rw should align with the v in v->domain.

Jan

Re: [v2 10/11] log-dirty: refine common code to support PML

[Jan Beulich]

>>> On 17.04.15 at 04:46, <kai.huang@linux.intel.com> wrote:
> On 04/16/2015 11:51 PM, Jan Beulich wrote:
>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>> @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>>>       d->arch.paging.mode |= PG_log_dirty;
>>>       paging_unlock(d);
>>>   
>>> +    /* enable hardware-assisted log-dirty if it is supported */
>>> +    p2m_enable_hardware_log_dirty(d);
>> I don't see that you would anywhere avoid setting up software
>> log-dirty handling - is that on purpose? If so, is there really a
>> win from adding PML?
>>
>>>       if ( log_global )
>>>       {
>>> -        /* set l1e entries of P2M table to be read-only. */
>>> +        /*
>>> +         * switch to log dirty mode, either by setting l1e entries of P2M table
>>> +         * to be read-only, or via hardware-assisted log-dirty.
>>> +         */
>>>           p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
>> Or did I miss you changing the behavior of this anywhere (as the
>> changed comment suggests)?
> Both of your comments are done in patch 11.

Partly - the new behavior indeed gets added there, but the misconfig
VM exits still seem to be a necessary part of the logic, so the question
stands: Is there really a win from adding PML? Or wait, I think now I
recall - the benefit comes from avoiding the protection violation exits,
not the misconfig ones. Sorry for the noise then.

Jan

Re: [v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Jan Beulich]

>>> On 17.04.15 at 04:40, <kai.huang@linux.intel.com> wrote:
> On 04/16/2015 11:54 PM, Jan Beulich wrote:
>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>> This patch firstly enables EPT A/D bits if PML is used, as PML depends on 
> EPT
>>> A/D bits to work. A bit is set for all present leaf p2m types, D bit is set 
> for
>>> all writable types, except log-dirty type.
>> I think the tying of "leaf" to the A bit part of the description became
>> stale, as you're now also doing this for non-leaf ones.
> You are right. How about just "A bit is set for all present p2m types, ..."?

Almost - adding "leaf" to the D bit part would still be desirable for
clarity.

Jan

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>> +{
>>>> +    uint64_t *pml_buf;
>>>> +    unsigned long pml_idx;
>>>> +
>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>> +
>>>> +    vmx_vmcs_enter(v);
>>>> +
>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>> Don't you require the vCPU to be non-running or current when you
>>> get here? If so, perhaps add a respective ASSERT()?
>> Yes an ASSERT would be better.
>>
>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>
>> ASSERT(atomic_read(&v->pause_count) || (v == current));
> For one I'd reverse the two parts. And then I think pause count
> being non-zero is not a sufficient condition - if a non-synchronous
> pause was issued against the vCPU it may still be running. I'd
> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
> pause count check instead of the runnable one if the only
> permitted case where v != current requires the vCPU to be
> paused.
The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:

     - When PML full VMEXIT happens
     - In paging_log_dirty_op & hap_track_dirty_vram, before reporting 
dirty pages to userspace.
     - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when 
log-dirty mode is disabled.

In the latter two cases, domain_pause is guaranteed to be called before 
vmx_vcpu_flush_pml_buffer is called, therefore looks there's no 
possibility of non-synchronous pause of the vcpu.

Or are you suggesting we should suppose this function can be called from 
any caller, and meanwhile is able to act reasonably?

>
>>>> +        /*
>>>> +         * Need to change type from log-dirty to normal memory for logged GFN.
>>>> +         * hap_track_dirty_vram depends on it to work. And we really only need
>>>> +         * to mark GFNs which hve been successfully changed from log-dirty to
>>>> +         * normal memory to be dirty.
>>>> +         */
>>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>>>> +                    p2m_ram_rw) )
>>> Indentation.
>> To be where exactly? Sorry I didn't find an example to refer in such case.
> p2m_ram_rw should align with the v in v->domain.
Understood. Will do.

Thanks,
-Kai
>
> Jan
>

Re: [v2 10/11] log-dirty: refine common code to support PML

[Kai Huang]

On 04/17/2015 02:28 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 04:46, <kai.huang@linux.intel.com> wrote:
>> On 04/16/2015 11:51 PM, Jan Beulich wrote:
>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>> @@ -190,9 +196,15 @@ static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>>>>        d->arch.paging.mode |= PG_log_dirty;
>>>>        paging_unlock(d);
>>>>    
>>>> +    /* enable hardware-assisted log-dirty if it is supported */
>>>> +    p2m_enable_hardware_log_dirty(d);
>>> I don't see that you would anywhere avoid setting up software
>>> log-dirty handling - is that on purpose? If so, is there really a
>>> win from adding PML?
>>>
>>>>        if ( log_global )
>>>>        {
>>>> -        /* set l1e entries of P2M table to be read-only. */
>>>> +        /*
>>>> +         * switch to log dirty mode, either by setting l1e entries of P2M table
>>>> +         * to be read-only, or via hardware-assisted log-dirty.
>>>> +         */
>>>>            p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
>>> Or did I miss you changing the behavior of this anywhere (as the
>>> changed comment suggests)?
>> Both of your comments are done in patch 11.
> Partly - the new behavior indeed gets added there, but the misconfig
> VM exits still seem to be a necessary part of the logic, so the question
> stands: Is there really a win from adding PML? Or wait, I think now I
> recall - the benefit comes from avoiding the protection violation exits,
> not the misconfig ones. Sorry for the noise then.
Yes PML is targeted to significantly reduce number of EPT violation 
caused by write protection of guest memory, and thus reduce hypervisor 
overhead of log-dirty mechanism.

Thanks,
-Kai
>
> Jan
>

Re: [v2 06/11] vmx: add help functions to support PML

[Jan Beulich]

>>> On 17.04.15 at 08:51, <kai.huang@linux.intel.com> wrote:

> 
> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>> +{
>>>>> +    uint64_t *pml_buf;
>>>>> +    unsigned long pml_idx;
>>>>> +
>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>> +
>>>>> +    vmx_vmcs_enter(v);
>>>>> +
>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>> Don't you require the vCPU to be non-running or current when you
>>>> get here? If so, perhaps add a respective ASSERT()?
>>> Yes an ASSERT would be better.
>>>
>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>
>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>> For one I'd reverse the two parts. And then I think pause count
>> being non-zero is not a sufficient condition - if a non-synchronous
>> pause was issued against the vCPU it may still be running. I'd
>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>> pause count check instead of the runnable one if the only
>> permitted case where v != current requires the vCPU to be
>> paused.
> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
> 
>      - When PML full VMEXIT happens
>      - In paging_log_dirty_op & hap_track_dirty_vram, before reporting 
> dirty pages to userspace.
>      - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when 
> log-dirty mode is disabled.
> 
> In the latter two cases, domain_pause is guaranteed to be called before 
> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no 
> possibility of non-synchronous pause of the vcpu.
> 
> Or are you suggesting we should suppose this function can be called from 
> any caller, and meanwhile is able to act reasonably?

No. All I'm saying is in order to protect against eventual undue
future callers, it should assert that its preconditions are met. I.e.
if the vCPU is expected to be paused, check that the pause
count in non-zero _and_ that the pause actually took effect.

Jan

Re: [v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Kai Huang]

On 04/17/2015 02:28 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 04:40, <kai.huang@linux.intel.com> wrote:
>> On 04/16/2015 11:54 PM, Jan Beulich wrote:
>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>> This patch firstly enables EPT A/D bits if PML is used, as PML depends on
>> EPT
>>>> A/D bits to work. A bit is set for all present leaf p2m types, D bit is set
>> for
>>>> all writable types, except log-dirty type.
>>> I think the tying of "leaf" to the A bit part of the description became
>>> stale, as you're now also doing this for non-leaf ones.
>> You are right. How about just "A bit is set for all present p2m types, ..."?
> Almost - adding "leaf" to the D bit part would still be desirable for
> clarity.
That will be more accurate. Thanks for suggestion. Is below good to you?

"A bit is set for all present p2m types in middle and leaf EPT entries, 
and D bit is set for all writable types in the leaf EPT entry, except 
log-dirty type."

And I will update comments at the beginning of ept_p2m_type_to_flags 
accordingly.

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 02:58 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 08:51, <kai.huang@linux.intel.com> wrote:
>> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>>> +{
>>>>>> +    uint64_t *pml_buf;
>>>>>> +    unsigned long pml_idx;
>>>>>> +
>>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>>> +
>>>>>> +    vmx_vmcs_enter(v);
>>>>>> +
>>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>>> Don't you require the vCPU to be non-running or current when you
>>>>> get here? If so, perhaps add a respective ASSERT()?
>>>> Yes an ASSERT would be better.
>>>>
>>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>>
>>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>>> For one I'd reverse the two parts. And then I think pause count
>>> being non-zero is not a sufficient condition - if a non-synchronous
>>> pause was issued against the vCPU it may still be running. I'd
>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>>> pause count check instead of the runnable one if the only
>>> permitted case where v != current requires the vCPU to be
>>> paused.
>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
>>
>>       - When PML full VMEXIT happens
>>       - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
>> dirty pages to userspace.
>>       - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
>> log-dirty mode is disabled.
>>
>> In the latter two cases, domain_pause is guaranteed to be called before
>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
>> possibility of non-synchronous pause of the vcpu.
>>
>> Or are you suggesting we should suppose this function can be called from
>> any caller, and meanwhile is able to act reasonably?
> No. All I'm saying is in order to protect against eventual undue
> future callers, it should assert that its preconditions are met. I.e.
> if the vCPU is expected to be paused, check that the pause
> count in non-zero _and_ that the pause actually took effect.
I see. I will do as you suggested:

ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));

And v != current should be the only case requires the vcpu to be paused.

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 11/11] p2m/ept: enable PML in p2m-ept for log-dirty

[Jan Beulich]

>>> On 17.04.15 at 09:10, <kai.huang@linux.intel.com> wrote:
> On 04/17/2015 02:28 PM, Jan Beulich wrote:
>>>>> On 17.04.15 at 04:40, <kai.huang@linux.intel.com> wrote:
>>> On 04/16/2015 11:54 PM, Jan Beulich wrote:
>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>> This patch firstly enables EPT A/D bits if PML is used, as PML depends on
>>> EPT
>>>>> A/D bits to work. A bit is set for all present leaf p2m types, D bit is set
>>> for
>>>>> all writable types, except log-dirty type.
>>>> I think the tying of "leaf" to the A bit part of the description became
>>>> stale, as you're now also doing this for non-leaf ones.
>>> You are right. How about just "A bit is set for all present p2m types, ..."?
>> Almost - adding "leaf" to the D bit part would still be desirable for
>> clarity.
> That will be more accurate. Thanks for suggestion. Is below good to you?

Yes.

> "A bit is set for all present p2m types in middle and leaf EPT entries, 
> and D bit is set for all writable types in the leaf EPT entry, except 
> log-dirty type."
> 
> And I will update comments at the beginning of ept_p2m_type_to_flags 
> accordingly.

Thanks.

Jan

Re: [v2 06/11] vmx: add help functions to support PML

[Jan Beulich]

>>> On 17.04.15 at 09:23, <kai.huang@linux.intel.com> wrote:

> 
> On 04/17/2015 02:58 PM, Jan Beulich wrote:
>>>>> On 17.04.15 at 08:51, <kai.huang@linux.intel.com> wrote:
>>> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>>>> +{
>>>>>>> +    uint64_t *pml_buf;
>>>>>>> +    unsigned long pml_idx;
>>>>>>> +
>>>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>>>> +
>>>>>>> +    vmx_vmcs_enter(v);
>>>>>>> +
>>>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>>>> Don't you require the vCPU to be non-running or current when you
>>>>>> get here? If so, perhaps add a respective ASSERT()?
>>>>> Yes an ASSERT would be better.
>>>>>
>>>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>>>
>>>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>>>> For one I'd reverse the two parts. And then I think pause count
>>>> being non-zero is not a sufficient condition - if a non-synchronous
>>>> pause was issued against the vCPU it may still be running. I'd
>>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>>>> pause count check instead of the runnable one if the only
>>>> permitted case where v != current requires the vCPU to be
>>>> paused.
>>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
>>>
>>>       - When PML full VMEXIT happens
>>>       - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
>>> dirty pages to userspace.
>>>       - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
>>> log-dirty mode is disabled.
>>>
>>> In the latter two cases, domain_pause is guaranteed to be called before
>>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
>>> possibility of non-synchronous pause of the vcpu.
>>>
>>> Or are you suggesting we should suppose this function can be called from
>>> any caller, and meanwhile is able to act reasonably?
>> No. All I'm saying is in order to protect against eventual undue
>> future callers, it should assert that its preconditions are met. I.e.
>> if the vCPU is expected to be paused, check that the pause
>> count in non-zero _and_ that the pause actually took effect.
> I see. I will do as you suggested:
> 
> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
> 
> And v != current should be the only case requires the vcpu to be paused.

But if you require (or at least expect) the vCPU to be paused, this
isn't what I suggested. Afaict

ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));

would then be the right check (and, while not be a full guarantee that
things wouldn't change behind your back, would at least increase
chances that the vCPU's runnable state won't change, as the vCPU
could have been non-runnable for reasons other than having been
paused).

Jan

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 03:37 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 09:23, <kai.huang@linux.intel.com> wrote:
>> On 04/17/2015 02:58 PM, Jan Beulich wrote:
>>>>>> On 17.04.15 at 08:51, <kai.huang@linux.intel.com> wrote:
>>>> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>>>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>>>>> +{
>>>>>>>> +    uint64_t *pml_buf;
>>>>>>>> +    unsigned long pml_idx;
>>>>>>>> +
>>>>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>>>>> +
>>>>>>>> +    vmx_vmcs_enter(v);
>>>>>>>> +
>>>>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>>>>> Don't you require the vCPU to be non-running or current when you
>>>>>>> get here? If so, perhaps add a respective ASSERT()?
>>>>>> Yes an ASSERT would be better.
>>>>>>
>>>>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>>>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>>>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>>>>
>>>>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>>>>> For one I'd reverse the two parts. And then I think pause count
>>>>> being non-zero is not a sufficient condition - if a non-synchronous
>>>>> pause was issued against the vCPU it may still be running. I'd
>>>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>>>>> pause count check instead of the runnable one if the only
>>>>> permitted case where v != current requires the vCPU to be
>>>>> paused.
>>>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
>>>>
>>>>        - When PML full VMEXIT happens
>>>>        - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
>>>> dirty pages to userspace.
>>>>        - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
>>>> log-dirty mode is disabled.
>>>>
>>>> In the latter two cases, domain_pause is guaranteed to be called before
>>>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
>>>> possibility of non-synchronous pause of the vcpu.
>>>>
>>>> Or are you suggesting we should suppose this function can be called from
>>>> any caller, and meanwhile is able to act reasonably?
>>> No. All I'm saying is in order to protect against eventual undue
>>> future callers, it should assert that its preconditions are met. I.e.
>>> if the vCPU is expected to be paused, check that the pause
>>> count in non-zero _and_ that the pause actually took effect.
>> I see. I will do as you suggested:
>>
>> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
>>
>> And v != current should be the only case requires the vcpu to be paused.
> But if you require (or at least expect) the vCPU to be paused, this
> isn't what I suggested. Afaict
>
> ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));
>
> would then be the right check (and, while not be a full guarantee that
> things wouldn't change behind your back, would at least increase
> chances that the vCPU's runnable state won't change, as the vCPU
> could have been non-runnable for reasons other than having been
> paused).
You are right. Your last sentence convinced me. I didn't go that far. I 
will use atomic_read(&v->pause_count) instead of !vcpu_runnable(v).

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Tim Deegan]

At 11:32 +0800 on 17 Apr (1429270332), Kai Huang wrote:
> 
> 
> On 04/17/2015 08:10 AM, Tim Deegan wrote:
> > At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
> >
> >>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> >>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> >>> +                    p2m_ram_rw) )
> >>> +            paging_mark_gfn_dirty(v->domain, gfn);
> >> Should we handle error from p2m_change_type_one and consequently
> >> making this flush function non-void?
> > I don't think we need to return an error, but we should probably
> > call mark_dirty here for anything except -EBUSY.
> Hi Kevin, Tim,
> 
> My intention here is to rule out the GFN with original type that is not 
> p2m_ram_logdirty, though with patch 11 it's not likely have such GFN 
> logged.
> 
> Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty, 
> so I think it might be OK to do as Tim suggested.
> 
> But given the same thing has already been done in hap_track_dirty_vram 
> (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in 
> bitmap with !p2m_change_type_one is true), and in EPT violation 
> (p2m_change_type_one is called unconditionally without checking return 
> value), I think it should be safe to do the current code here.

The paging_log_dirty_range case is doing something quite different:
it is making pages read-only so they can be tracked, and it needs to
mark any page that couldn't be made read-only (because the guest can
write to them).  Its three cases are:
 - change succeeded: no mark, we will trap any new writes
 - EBUSY: mark, since we can't be sure we'll trap new writes
 - other error: mark, since we can't be sure we'll trap new writes

In this case we _know_ the guest has written to the page (because it's
in the PML log), so our only reason for not calling mark_dirty() is
if we see that someone else has changed the p2m (EBUSY) and that
someone else ought to already have DTRT.

Now that I've written it out, and since we expect these races to be
very rare, I've changed my mind: we should _always_ call mark_dirty
here.  The extra cost should be negligible, and a missing mark is
extremely difficult to debug.

Cheers,

Tim.

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 04:36 PM, Tim Deegan wrote:
> At 11:32 +0800 on 17 Apr (1429270332), Kai Huang wrote:
>>
>> On 04/17/2015 08:10 AM, Tim Deegan wrote:
>>> At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
>>>
>>>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>>>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>>>>> +                    p2m_ram_rw) )
>>>>> +            paging_mark_gfn_dirty(v->domain, gfn);
>>>> Should we handle error from p2m_change_type_one and consequently
>>>> making this flush function non-void?
>>> I don't think we need to return an error, but we should probably
>>> call mark_dirty here for anything except -EBUSY.
>> Hi Kevin, Tim,
>>
>> My intention here is to rule out the GFN with original type that is not
>> p2m_ram_logdirty, though with patch 11 it's not likely have such GFN
>> logged.
>>
>> Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty,
>> so I think it might be OK to do as Tim suggested.
>>
>> But given the same thing has already been done in hap_track_dirty_vram
>> (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in
>> bitmap with !p2m_change_type_one is true), and in EPT violation
>> (p2m_change_type_one is called unconditionally without checking return
>> value), I think it should be safe to do the current code here.
> The paging_log_dirty_range case is doing something quite different:
> it is making pages read-only so they can be tracked, and it needs to
> mark any page that couldn't be made read-only (because the guest can
> write to them).
Thanks for comprehensive reply. However, looks I can't agree on some points.

paging_log_dirty_range currently is only used by hap_track_dirty_vram 
for video ram tracking, and it doesn't call paging_mark_dirty in any 
case. Basically, paging_log_dirty_range only does below thing but 
nothing else:

     for ( i = 0, pfn = begin_pfn; pfn < begin_pfn + nr; i++, pfn++ )
         if ( !p2m_change_type_one(d, pfn, p2m_ram_rw, p2m_ram_logdirty) )
             dirty_bitmap[i >> 3] |= (1 << (i & 7));

 From which we can see the purpose of this function (and let us put PML 
away for now):

     - change GFN's type from p2m_ram_rw back to p2m_ram_logdirty, in 
order to be able to log the GFN again (more precisely, to track the GFN 
again in EPT violation), with the fact that the dirty page's p2m type 
has been changed from p2m_log_dirty to p2m_ram_rw, in EPT violation.
     - mark the dirty GFN to the bitmap only when above changing 
p2m_ram_logdirty to p2m_ram_rw is done successfully. It is reasonable, 
as only a successful changing from p2m_ram_rw to p2m_ram_dirty means the 
dirty page has been changed from p2m_ram_logdirty to p2m_ram_rw in EPT 
violation.

Btw, from which we can also note that currently video ram tracking is 
not via log-dirty radix tree but depends on p2m type change, this is the 
reason we must call p2m_type_change_one in vmx_vcpu_flush_pml_buffer.

> Its three cases are:
>   - change succeeded: no mark, we will trap any new writes
>   - EBUSY: mark, since we can't be sure we'll trap new writes
>   - other error: mark, since we can't be sure we'll trap new writes
Regarding to the above three cases, I assume you are referring to 
changing p2m_ram_rw back to p2m_ram_logdirty in paging_log_dirty_range , 
in which case the paging_mark_dirty is not called at all, as I mentioned 
above.

>
> In this case we _know_ the guest has written to the page (because it's
> in the PML log), so our only reason for not calling mark_dirty() is
> if we see that someone else has changed the p2m (EBUSY) and that
> someone else ought to already have DTRT.
I agree on this, given you said we can't be sure for the unsuccessful 
p2m type change.

>
> Now that I've written it out, and since we expect these races to be
> very rare, I've changed my mind: we should _always_ call mark_dirty
> here.  The extra cost should be negligible, and a missing mark is
> extremely difficult to debug.
Which is also good to me, and in this case we should also call 
p2m_change_type_one(.., p2m_ram_logdirty, p2m_ram_rw) unconditionally, 
as this is required for video ram tracking.

Thanks,
-Kai
>
> Cheers,
>
> Tim.

Re: [v2 06/11] vmx: add help functions to support PML

[Tim Deegan]

At 17:29 +0800 on 17 Apr (1429291763), Kai Huang wrote:
> 
> 
> On 04/17/2015 04:36 PM, Tim Deegan wrote:
> > At 11:32 +0800 on 17 Apr (1429270332), Kai Huang wrote:
> >>
> >> On 04/17/2015 08:10 AM, Tim Deegan wrote:
> >>> At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
> >>>
> >>>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> >>>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> >>>>> +                    p2m_ram_rw) )
> >>>>> +            paging_mark_gfn_dirty(v->domain, gfn);
> >>>> Should we handle error from p2m_change_type_one and consequently
> >>>> making this flush function non-void?
> >>> I don't think we need to return an error, but we should probably
> >>> call mark_dirty here for anything except -EBUSY.
> >> Hi Kevin, Tim,
> >>
> >> My intention here is to rule out the GFN with original type that is not
> >> p2m_ram_logdirty, though with patch 11 it's not likely have such GFN
> >> logged.
> >>
> >> Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty,
> >> so I think it might be OK to do as Tim suggested.
> >>
> >> But given the same thing has already been done in hap_track_dirty_vram
> >> (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in
> >> bitmap with !p2m_change_type_one is true), and in EPT violation
> >> (p2m_change_type_one is called unconditionally without checking return
> >> value), I think it should be safe to do the current code here.
> > The paging_log_dirty_range case is doing something quite different:
> > it is making pages read-only so they can be tracked, and it needs to
> > mark any page that couldn't be made read-only (because the guest can
> > write to them).
> Thanks for comprehensive reply. However, looks I can't agree on some points.
> 
> paging_log_dirty_range currently is only used by hap_track_dirty_vram 
> for video ram tracking, and it doesn't call paging_mark_dirty in any 
> case.

Sure, it doesn't call paging_mark_dirty(), but instead it puts the
marks into a bitmap directly.

> Basically, paging_log_dirty_range only does below thing but 
> nothing else:
> 
>      for ( i = 0, pfn = begin_pfn; pfn < begin_pfn + nr; i++, pfn++ )
>          if ( !p2m_change_type_one(d, pfn, p2m_ram_rw, p2m_ram_logdirty) )
>              dirty_bitmap[i >> 3] |= (1 << (i & 7));
> 
>  From which we can see the purpose of this function (and let us put PML 
> away for now):
> 
>      - change GFN's type from p2m_ram_rw back to p2m_ram_logdirty, in 
> order to be able to log the GFN again (more precisely, to track the GFN 
> again in EPT violation), with the fact that the dirty page's p2m type 
> has been changed from p2m_log_dirty to p2m_ram_rw, in EPT violation.
>      - mark the dirty GFN to the bitmap only when above changing 
> p2m_ram_logdirty to p2m_ram_rw is done successfully. It is reasonable, 
> as only a successful changing from p2m_ram_rw to p2m_ram_dirty means the 
> dirty page has been changed from p2m_ram_logdirty to p2m_ram_rw in EPT 
> violation.

Right; so this code should probably also be setting the mark if
p2m_change_type_one() returns anything except EBUSY.

But in this vram code we can't just set the mark in all cases, because
we need to detect the case where the type is still p2m_ram_logdirty --
i.e. the page hasn't been written to.

> Btw, from which we can also note that currently video ram tracking is 
> not via log-dirty radix tree but depends on p2m type change, this is the 
> reason we must call p2m_type_change_one in vmx_vcpu_flush_pml_buffer.

I think we need to do that anyway, to make sure that next time we clear
the bitmap, the change back from _rw to _logdirty clears the D bit.

But it does suggest that we might want to flush the PML buffers in the
vram function.

> > Now that I've written it out, and since we expect these races to be
> > very rare, I've changed my mind: we should _always_ call mark_dirty
> > here.  The extra cost should be negligible, and a missing mark is
> > extremely difficult to debug.
> Which is also good to me, and in this case we should also call 
> p2m_change_type_one(.., p2m_ram_logdirty, p2m_ram_rw) unconditionally, 
> as this is required for video ram tracking.

Yep. 

Tim.

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On Mon, Apr 20, 2015 at 4:29 PM, Tim Deegan <tim@xen.org> wrote:
> At 17:29 +0800 on 17 Apr (1429291763), Kai Huang wrote:
>>
>>
>> On 04/17/2015 04:36 PM, Tim Deegan wrote:
>> > At 11:32 +0800 on 17 Apr (1429270332), Kai Huang wrote:
>> >>
>> >> On 04/17/2015 08:10 AM, Tim Deegan wrote:
>> >>> At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
>> >>>
>> >>>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>> >>>>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>> >>>>> +                    p2m_ram_rw) )
>> >>>>> +            paging_mark_gfn_dirty(v->domain, gfn);
>> >>>> Should we handle error from p2m_change_type_one and consequently
>> >>>> making this flush function non-void?
>> >>> I don't think we need to return an error, but we should probably
>> >>> call mark_dirty here for anything except -EBUSY.
>> >> Hi Kevin, Tim,
>> >>
>> >> My intention here is to rule out the GFN with original type that is not
>> >> p2m_ram_logdirty, though with patch 11 it's not likely have such GFN
>> >> logged.
>> >>
>> >> Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty,
>> >> so I think it might be OK to do as Tim suggested.
>> >>
>> >> But given the same thing has already been done in hap_track_dirty_vram
>> >> (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in
>> >> bitmap with !p2m_change_type_one is true), and in EPT violation
>> >> (p2m_change_type_one is called unconditionally without checking return
>> >> value), I think it should be safe to do the current code here.
>> > The paging_log_dirty_range case is doing something quite different:
>> > it is making pages read-only so they can be tracked, and it needs to
>> > mark any page that couldn't be made read-only (because the guest can
>> > write to them).
>> Thanks for comprehensive reply. However, looks I can't agree on some points.
>>
>> paging_log_dirty_range currently is only used by hap_track_dirty_vram
>> for video ram tracking, and it doesn't call paging_mark_dirty in any
>> case.
>
> Sure, it doesn't call paging_mark_dirty(), but instead it puts the
> marks into a bitmap directly.
>
>> Basically, paging_log_dirty_range only does below thing but
>> nothing else:
>>
>>      for ( i = 0, pfn = begin_pfn; pfn < begin_pfn + nr; i++, pfn++ )
>>          if ( !p2m_change_type_one(d, pfn, p2m_ram_rw, p2m_ram_logdirty) )
>>              dirty_bitmap[i >> 3] |= (1 << (i & 7));
>>
>>  From which we can see the purpose of this function (and let us put PML
>> away for now):
>>
>>      - change GFN's type from p2m_ram_rw back to p2m_ram_logdirty, in
>> order to be able to log the GFN again (more precisely, to track the GFN
>> again in EPT violation), with the fact that the dirty page's p2m type
>> has been changed from p2m_log_dirty to p2m_ram_rw, in EPT violation.
>>      - mark the dirty GFN to the bitmap only when above changing
>> p2m_ram_logdirty to p2m_ram_rw is done successfully. It is reasonable,
>> as only a successful changing from p2m_ram_rw to p2m_ram_dirty means the
>> dirty page has been changed from p2m_ram_logdirty to p2m_ram_rw in EPT
>> violation.
>
> Right; so this code should probably also be setting the mark if
> p2m_change_type_one() returns anything except EBUSY.

Reasonable. But as so far there's nothing wrong behavior observed with
current code, I intend to leave it unchanged. Is it OK to you?

>
> But in this vram code we can't just set the mark in all cases, because
> we need to detect the case where the type is still p2m_ram_logdirty --
> i.e. the page hasn't been written to.

Agreed.

>
>> Btw, from which we can also note that currently video ram tracking is
>> not via log-dirty radix tree but depends on p2m type change, this is the
>> reason we must call p2m_type_change_one in vmx_vcpu_flush_pml_buffer.
>
> I think we need to do that anyway, to make sure that next time we clear
> the bitmap, the change back from _rw to _logdirty clears the D bit.
>
> But it does suggest that we might want to flush the PML buffers in the
> vram function.

True.

>
>> > Now that I've written it out, and since we expect these races to be
>> > very rare, I've changed my mind: we should _always_ call mark_dirty
>> > here.  The extra cost should be negligible, and a missing mark is
>> > extremely difficult to debug.
>> Which is also good to me, and in this case we should also call
>> p2m_change_type_one(.., p2m_ram_logdirty, p2m_ram_rw) unconditionally,
>> as this is required for video ram tracking.
>
> Yep.

Agreed. So this time for the PML patches, I'll always call both
mark_dirty and p2m_change_type_one (and ignore return value) for all
logged GPA.

But I intend not to change current video ram tracking code
(paging_log_dirty_range), as explained above. Is this good to you?

Thanks,
-Kai

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



-- 
Thanks,
-Kai

Re: [v2 06/11] vmx: add help functions to support PML

[Tim Deegan]

Hi,

At 18:08 +0800 on 20 Apr (1429553282), Kai Huang wrote:
> Agreed. So this time for the PML patches, I'll always call both
> mark_dirty and p2m_change_type_one (and ignore return value) for all
> logged GPA.
> 
> But I intend not to change current video ram tracking code
> (paging_log_dirty_range), as explained above. Is this good to you?

Yes, that's fine.

Cheers,

Tim.

Re: [v2 05/11] vmx: add new data structure member to support PML

[Kai Huang]

On 04/17/2015 10:31 AM, Kai Huang wrote:
>
>
> On 04/17/2015 06:39 AM, Tian, Kevin wrote:
>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>>> Sent: Wednesday, April 15, 2015 3:04 PM
>>>
>>> A new 4K page pointer is added to arch_vmx_struct as PML buffer for 
>>> vcpu.
>>> And a
>>> new 'status' field is added to vmx_domain to indicate whether PML is 
>>> enabled
>>> for
>>> the domain or not. The 'status' field also can be used for further 
>>> similiar
>>> purpose.
>> not sure about the last sentence. what's the similar purpose to 
>> "whether PML
>> is enabled"? :-)
> I mean potentially there might be such feature in the future, and I 
> can't give you an example right now. If you are just commenting the 
> description here but fine with the current code, I can remove that 
> last sentence if you like. Or do you suggest to just use a "bool_t 
> pml_enabled"? I am fine with both, but looks there's no objection from 
> others so I intend to keep it as 'unsigned int status', if you agree.
Hi Kevin,

What's your opinion here? Is 'unsigned int status' OK to you?

>
>>
>>> Note both new members don't have to be initialized to zero 
>>> explicitly as both
>>> vcpu and domain structure are zero-ed when they are created.
>> no initialization in this patch, so why explaining it here?
> OK. Looks it's a common sense to all of you so I'll just remove this 
> sentence.
>
>>
>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>>> ---
>>>   xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> index f831a78..2c679ac 100644
>>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> @@ -70,8 +70,12 @@ struct ept_data {
>>>       cpumask_var_t synced_mask;
>>>   };
>>>
>>> +#define _VMX_DOMAIN_PML_ENABLED    0
>>> +#define VMX_DOMAIN_PML_ENABLED     (1ul <<
>>> _VMX_DOMAIN_PML_ENABLED)
>>>   struct vmx_domain {
>>>       unsigned long apic_access_mfn;
>>> +    /* VMX_DOMAIN_* */
>>> +    unsigned long status;
>>>   };
>>>
>>>   struct pi_desc {
>>> @@ -142,6 +146,9 @@ struct arch_vmx_struct {
>>>       /* Bitmap to control vmexit policy for Non-root VMREAD/VMWRITE */
>>>       struct page_info     *vmread_bitmap;
>>>       struct page_info     *vmwrite_bitmap;
>>> +
>>> +#define NR_PML_ENTRIES   512
>>> +    struct page_info     *pml_pg;
>> move the macro out of the structure.
> OK. I will move it just above the declaration of struct arch_vmx_struct.
>
>> and is pml_buffer or pml_buf more clear?
>
> To me pml_buffer or pml_buf is more likely a virtual address you can 
> access the buffer directly, while pml_pg indicates it's a pointer of 
> struct page_info. If you you look at patch 6, you can find statements 
> like:
>
>     uint64_t *pml_buf;
>
>     pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
>
> So I intend to keep it.
And this one? Are you OK with 'pml_pg'?

Thanks,
-Kai
>
> Thanks,
> -Kai
>>
>>>   };
>>>
>>>   int vmx_create_vmcs(struct vcpu *v);
>>> -- 
>>> 2.1.0
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 05/11] vmx: add new data structure member to support PML

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> Sent: Tuesday, April 21, 2015 2:05 PM
> 
> 
> On 04/17/2015 10:31 AM, Kai Huang wrote:
> >
> >
> > On 04/17/2015 06:39 AM, Tian, Kevin wrote:
> >>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> >>> Sent: Wednesday, April 15, 2015 3:04 PM
> >>>
> >>> A new 4K page pointer is added to arch_vmx_struct as PML buffer for
> >>> vcpu.
> >>> And a
> >>> new 'status' field is added to vmx_domain to indicate whether PML is
> >>> enabled
> >>> for
> >>> the domain or not. The 'status' field also can be used for further
> >>> similiar
> >>> purpose.
> >> not sure about the last sentence. what's the similar purpose to
> >> "whether PML
> >> is enabled"? :-)
> > I mean potentially there might be such feature in the future, and I
> > can't give you an example right now. If you are just commenting the
> > description here but fine with the current code, I can remove that
> > last sentence if you like. Or do you suggest to just use a "bool_t
> > pml_enabled"? I am fine with both, but looks there's no objection from
> > others so I intend to keep it as 'unsigned int status', if you agree.
> Hi Kevin,
> 
> What's your opinion here? Is 'unsigned int status' OK to you?

yes

> 
> >
> >>
> >>> Note both new members don't have to be initialized to zero
> >>> explicitly as both
> >>> vcpu and domain structure are zero-ed when they are created.
> >> no initialization in this patch, so why explaining it here?
> > OK. Looks it's a common sense to all of you so I'll just remove this
> > sentence.
> >
> >>
> >>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> >>> ---
> >>>   xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
> >>>   1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> >>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> >>> index f831a78..2c679ac 100644
> >>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> >>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> >>> @@ -70,8 +70,12 @@ struct ept_data {
> >>>       cpumask_var_t synced_mask;
> >>>   };
> >>>
> >>> +#define _VMX_DOMAIN_PML_ENABLED    0
> >>> +#define VMX_DOMAIN_PML_ENABLED     (1ul <<
> >>> _VMX_DOMAIN_PML_ENABLED)
> >>>   struct vmx_domain {
> >>>       unsigned long apic_access_mfn;
> >>> +    /* VMX_DOMAIN_* */
> >>> +    unsigned long status;
> >>>   };
> >>>
> >>>   struct pi_desc {
> >>> @@ -142,6 +146,9 @@ struct arch_vmx_struct {
> >>>       /* Bitmap to control vmexit policy for Non-root
> VMREAD/VMWRITE */
> >>>       struct page_info     *vmread_bitmap;
> >>>       struct page_info     *vmwrite_bitmap;
> >>> +
> >>> +#define NR_PML_ENTRIES   512
> >>> +    struct page_info     *pml_pg;
> >> move the macro out of the structure.
> > OK. I will move it just above the declaration of struct arch_vmx_struct.
> >
> >> and is pml_buffer or pml_buf more clear?
> >
> > To me pml_buffer or pml_buf is more likely a virtual address you can
> > access the buffer directly, while pml_pg indicates it's a pointer of
> > struct page_info. If you you look at patch 6, you can find statements
> > like:
> >
> >     uint64_t *pml_buf;
> >
> >     pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
> >
> > So I intend to keep it.
> And this one? Are you OK with 'pml_pg'?
> 

good to me too.

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/17/2015 03:37 PM, Jan Beulich wrote:
>>>> On 17.04.15 at 09:23, <kai.huang@linux.intel.com> wrote:
>> On 04/17/2015 02:58 PM, Jan Beulich wrote:
>>>>>> On 17.04.15 at 08:51, <kai.huang@linux.intel.com> wrote:
>>>> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>>>>> On 17.04.15 at 05:10, <kai.huang@linux.intel.com> wrote:
>>>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>>>>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>>>>> +{
>>>>>>>> +    uint64_t *pml_buf;
>>>>>>>> +    unsigned long pml_idx;
>>>>>>>> +
>>>>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>>>>> +
>>>>>>>> +    vmx_vmcs_enter(v);
>>>>>>>> +
>>>>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>>>>> Don't you require the vCPU to be non-running or current when you
>>>>>>> get here? If so, perhaps add a respective ASSERT()?
>>>>>> Yes an ASSERT would be better.
>>>>>>
>>>>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>>>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>>>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>>>>
>>>>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>>>>> For one I'd reverse the two parts. And then I think pause count
>>>>> being non-zero is not a sufficient condition - if a non-synchronous
>>>>> pause was issued against the vCPU it may still be running. I'd
>>>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>>>>> pause count check instead of the runnable one if the only
>>>>> permitted case where v != current requires the vCPU to be
>>>>> paused.
>>>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
>>>>
>>>>        - When PML full VMEXIT happens
>>>>        - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
>>>> dirty pages to userspace.
>>>>        - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
>>>> log-dirty mode is disabled.
>>>>
>>>> In the latter two cases, domain_pause is guaranteed to be called before
>>>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
>>>> possibility of non-synchronous pause of the vcpu.
>>>>
>>>> Or are you suggesting we should suppose this function can be called from
>>>> any caller, and meanwhile is able to act reasonably?
>>> No. All I'm saying is in order to protect against eventual undue
>>> future callers, it should assert that its preconditions are met. I.e.
>>> if the vCPU is expected to be paused, check that the pause
>>> count in non-zero _and_ that the pause actually took effect.
>> I see. I will do as you suggested:
>>
>> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
>>
>> And v != current should be the only case requires the vcpu to be paused.
> But if you require (or at least expect) the vCPU to be paused, this
> isn't what I suggested. Afaict
>
> ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));
>
> would then be the right check (and, while not be a full guarantee that
> things wouldn't change behind your back, would at least increase
> chances that the vCPU's runnable state won't change, as the vCPU
> could have been non-runnable for reasons other than having been
> paused).
Hi Jan,

I tried the ASSERT with atomic_read(&v->pause_count), and it turns out 
the ASSERT would fail and panic Xen. The reason is domain_pause only 
increases d->pause_count, but it doesn't internally increase 
v->pause_count for all vcpus.

vmx_vcpu_flush_pml_buffer is only supposed to be called from PML buffer 
full VMEXIT, and vmx_domain_flush_pml_buffer, before which domain_pause 
should be called.

Sorry that obviously I had some misunderstanding regarding to "require 
(or at least expect) vCPU to be paused", and looks !vcpu_runnable(v) is 
the right choice.

What's your opinion?

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v2 06/11] vmx: add help functions to support PML

[Jan Beulich]

>>> On 24.04.15 at 08:32, <kai.huang@linux.intel.com> wrote:
> On 04/17/2015 03:37 PM, Jan Beulich wrote:
>>>>> On 17.04.15 at 09:23, <kai.huang@linux.intel.com> wrote:
>>> I see. I will do as you suggested:
>>>
>>> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
>>>
>>> And v != current should be the only case requires the vcpu to be paused.
>> But if you require (or at least expect) the vCPU to be paused, this
>> isn't what I suggested. Afaict
>>
>> ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));
>>
>> would then be the right check (and, while not be a full guarantee that
>> things wouldn't change behind your back, would at least increase
>> chances that the vCPU's runnable state won't change, as the vCPU
>> could have been non-runnable for reasons other than having been
>> paused).
> Hi Jan,
> 
> I tried the ASSERT with atomic_read(&v->pause_count), and it turns out 
> the ASSERT would fail and panic Xen. The reason is domain_pause only 
> increases d->pause_count, but it doesn't internally increase 
> v->pause_count for all vcpus.
> 
> vmx_vcpu_flush_pml_buffer is only supposed to be called from PML buffer 
> full VMEXIT, and vmx_domain_flush_pml_buffer, before which domain_pause 
> should be called.
> 
> Sorry that obviously I had some misunderstanding regarding to "require 
> (or at least expect) vCPU to be paused", and looks !vcpu_runnable(v) is 
> the right choice.
> 
> What's your opinion?

Then either go with the slightly weaker original (still quoted at the
top) or (preferred by me) OR together both pause counts in the
condition.

Jan

Re: [v2 06/11] vmx: add help functions to support PML

[Kai Huang]

On 04/24/2015 03:30 PM, Jan Beulich wrote:
>>>> On 24.04.15 at 08:32, <kai.huang@linux.intel.com> wrote:
>> On 04/17/2015 03:37 PM, Jan Beulich wrote:
>>>>>> On 17.04.15 at 09:23, <kai.huang@linux.intel.com> wrote:
>>>> I see. I will do as you suggested:
>>>>
>>>> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
>>>>
>>>> And v != current should be the only case requires the vcpu to be paused.
>>> But if you require (or at least expect) the vCPU to be paused, this
>>> isn't what I suggested. Afaict
>>>
>>> ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));
>>>
>>> would then be the right check (and, while not be a full guarantee that
>>> things wouldn't change behind your back, would at least increase
>>> chances that the vCPU's runnable state won't change, as the vCPU
>>> could have been non-runnable for reasons other than having been
>>> paused).
>> Hi Jan,
>>
>> I tried the ASSERT with atomic_read(&v->pause_count), and it turns out
>> the ASSERT would fail and panic Xen. The reason is domain_pause only
>> increases d->pause_count, but it doesn't internally increase
>> v->pause_count for all vcpus.
>>
>> vmx_vcpu_flush_pml_buffer is only supposed to be called from PML buffer
>> full VMEXIT, and vmx_domain_flush_pml_buffer, before which domain_pause
>> should be called.
>>
>> Sorry that obviously I had some misunderstanding regarding to "require
>> (or at least expect) vCPU to be paused", and looks !vcpu_runnable(v) is
>> the right choice.
>>
>> What's your opinion?
> Then either go with the slightly weaker original (still quoted at the
> top) or (preferred by me) OR together both pause counts in the
> condition.
Thanks. Then I'd like to use original weaker one (below) instead of 
explicitly doing OR 'd->pause_count' and 'v->pause_count', as you are 
not objecting the former :)

ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel