PML (Paging Modification Logging) support

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 <[email protected]>

---
 xen/arch/x86/hvm/vmx/vmcs.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

xen/arch/x86/hvm/vmx/vmcs.c +31 -0

index d614638..4fff46d 100644
@@ -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;

xen/arch/x86/hvm/vmx/vmcs.c integer_param("ple_gap", ple_gap); · line 78

Quoted 5 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +4 -0

+static void __init parse_ept_param(char *s)
+{
+    char *ss;
+    int val;

bool_t, and would better move ...

xen/arch/x86/hvm/vmx/vmcs.c integer_param("ple_gap", ple_gap); · line 81

Quoted 3 lines

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

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

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 15.04.15 at 15:20, <[email protected]> wrote:

Quoted 12 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c integer_param("ple_gap", ple_gap); · line 78

Quoted 5 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +4 -0

+static void __init parse_ept_param(char *s)
+{
+    char *ss;
+    int val;

bool_t, and would better move ...

Quoted 3 lines

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 1 line

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

Thanks for review. Do you mean below?

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

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

Signed-off-by Kai Huang <[email protected]>

---
 docs/misc/xen-command-line.markdown | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

docs/misc/xen-command-line.markdown +3 -0

index 1dda1f0..ae30d02 100644
@@ -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 ]`
+

docs/misc/xen-command-line.markdown +1 -0

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

Quoted 1 line

Quoted 1 line · On 15.04.15 at 12:15, <[email protected]> wrote:

Quoted 3 lines · Kai Huang

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

Signed-off-by Kai Huang <[email protected]>

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

Quoted 3 lines · Kai Huang

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

Signed-off-by Kai Huang <[email protected]>

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

Quoted 13 lines

---
  docs/misc/xen-command-line.markdown | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

docs/misc/xen-command-line.markdown +0 -0

index 1dda1f0..ae30d02 100644
@@ -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.
 

docs/misc/xen-command-line.markdown requirement can be relaxed. This option is particularly useful for nested · line 700

Quoted 13 lines

docs/misc/xen-command-line.markdown +13 -0

+### 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

docs/misc/xen-command-line.markdown requirement can be relaxed. This option is particularly useful for nested · line 704

Quoted 4 lines

docs/misc/xen-command-line.markdown +1 -0

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 6 lines · Andrew Cooper

Quoted 3 lines · Kai Huang

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

Signed-off-by Kai Huang <[email protected]>

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.

Quoted 23 lines

Quoted 23 lines

---
  docs/misc/xen-command-line.markdown | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

docs/misc/xen-command-line.markdown +0 -0

index 1dda1f0..ae30d02 100644
@@ -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 ]

docs/misc/xen-command-line.markdown +6 -0

+
+> Sub-options:
+
+> `pml`
+
+> This sub-option is boolean type and can be prefixed with `no-` to

Quoted 8 lines

docs/misc/xen-command-line.markdown requirement can be relaxed. This option is particularly useful for nested · line 700

Quoted 6 lines

effect the
+> inverse meaning.

docs/misc/xen-command-line.markdown +4 -0

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

docs/misc/xen-command-line.markdown +6 -0

+### ept (Intel)
+> `= List of ( pml<boolean> )`
+
+> Default: `false`
+
+Controls EPT related features. Currently only Page Modification

Logging (PML) is

(quoted diff) +3 -0

+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

(quoted diff) +3 -0

+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

From: Kai Huang [mailto:[email protected]]
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 <[email protected]>

---
 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(+)

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

index 4fff46d..d120370 100644
@@ -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

xen/arch/x86/hvm/vmx/vmcs.c +4 -0

+         * 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 )

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

@@ -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 )
     {

xen/arch/x86/hvm/vmx/vmcs.c +3 -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);

xen/include/asm-x86/hvm/vmx/vmcs.h +6 -0

index 6fce6aa..f831a78 100644
@@ -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,

xen/include/asm-x86/hvm/vmx/vmx.h +1 -0

index 91c5e18..50f1bfc 100644
@@ -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

Quoted 48 lines

From: Kai Huang [mailto:[email protected]]
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 <[email protected]>

---
  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(+)

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

index 4fff46d..d120370 100644
@@ -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

Quoted 6 lines

xen/arch/x86/hvm/vmx/vmcs.c +4 -0

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

Quoted 94 lines

if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

@@ -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 )
      {

xen/arch/x86/hvm/vmx/vmcs.c +3 -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);

xen/include/asm-x86/hvm/vmx/vmcs.h +6 -0

index 6fce6aa..f831a78 100644
@@ -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,

xen/include/asm-x86/hvm/vmx/vmx.h +1 -0

index 91c5e18..50f1bfc 100644
@@ -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
 
  /*

Quoted 3 lines

* Interruption-information format
--
2.1.0

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 7 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/include/asm-x86/hvm/vmx/vmcs.h +0 -0

@@ -70,8 +70,12 @@ struct ept_data {
      cpumask_var_t synced_mask;
  };
 

xen/include/asm-x86/hvm/vmx/vmcs.h struct ept_data { · line 78

Quoted 6 lines

xen/include/asm-x86/hvm/vmx/vmcs.h +4 -0

+#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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

From: Kai Huang [mailto:[email protected]]
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.

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 <[email protected]>

---
 xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
 1 file changed, 7 insertions(+)

xen/include/asm-x86/hvm/vmx/vmcs.h +4 -0

index f831a78..2c679ac 100644
@@ -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 {

xen/include/asm-x86/hvm/vmx/vmcs.h +0 -1

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

2.1.0

Quoted 9 lines

From: Kai Huang [mailto:[email protected]]
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"? :-)

Quoted 3 lines

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?

Quoted 24 lines

Signed-off-by Kai Huang <[email protected]>

---
  xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
  1 file changed, 7 insertions(+)

xen/include/asm-x86/hvm/vmx/vmcs.h +4 -0

index f831a78..2c679ac 100644
@@ -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;
  };
 

xen/include/asm-x86/hvm/vmx/vmcs.h struct arch_vmx_struct { · line 151

Quoted 8 lines

  struct pi_desc {

xen/include/asm-x86/hvm/vmx/vmcs.h +3 -0

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

Quoted 6 lines

xen/include/asm-x86/hvm/vmx/vmcs.h +0 -1

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

2.1.0

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 9 lines · Kevin

Quoted 9 lines

From: Kai Huang [mailto:[email protected]]
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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 24 lines

Quoted 24 lines

Signed-off-by Kai Huang <[email protected]>

---
  xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
  1 file changed, 7 insertions(+)

xen/include/asm-x86/hvm/vmx/vmcs.h +4 -0

index f831a78..2c679ac 100644
@@ -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;
  };
 

Quoted 9 lines

xen/include/asm-x86/hvm/vmx/vmcs.h struct arch_vmx_struct { · line 151

Quoted 8 lines

  struct pi_desc {

xen/include/asm-x86/hvm/vmx/vmcs.h +3 -0

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

Quoted 1 line

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

Quoted 11 lines

Quoted 6 lines

xen/include/asm-x86/hvm/vmx/vmcs.h +0 -0

  };
 
  int vmx_create_vmcs(struct vcpu *v);

Signature · 1 line

2.1.0

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

From: Kai Huang [mailto:[email protected]]
Sent: Tuesday, April 21, 2015 2:05 PM

Quoted 18 lines · Kai Huang

Quoted 12 lines · Kevin

Quoted 12 lines

From: Kai Huang [mailto:[email protected]]
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.

Quoted 3 lines

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

Quoted 6 lines

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?

Quoted 34 lines

Quoted 7 lines

Quoted 6 lines

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.

Quoted 24 lines

Quoted 24 lines

Signed-off-by Kai Huang <[email protected]>

---
  xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
  1 file changed, 7 insertions(+)

xen/include/asm-x86/hvm/vmx/vmcs.h +4 -0

index f831a78..2c679ac 100644
@@ -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;
  };
 

Quoted 3 lines

Quoted 3 lines

xen/include/asm-x86/hvm/vmx/vmcs.h struct arch_vmx_struct { · line 146

Quoted 3 lines

  struct pi_desc {

xen/include/asm-x86/hvm/vmx/vmcs.h +0 -0

@@ -142,6 +146,9 @@ struct arch_vmx_struct {
      /* Bitmap to control vmexit policy for Non-root

VMREAD/VMWRITE */

Quoted 15 lines

Quoted 6 lines

xen/include/asm-x86/hvm/vmx/vmcs.h struct arch_vmx_struct { · line 151

Quoted 5 lines

xen/include/asm-x86/hvm/vmx/vmcs.h +3 -0

      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.

Quoted 1 line

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:

Quoted 5 lines

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

xen/arch/x86/hvm/vmx/vmcs.c +7 -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);

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

+
+    /* 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

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1344

Quoted 9 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

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

Quoted 10 lines

xen/arch/x86/hvm/vmx/vmcs.c +10 -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);
+

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1379

Quoted 7 lines

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

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

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1392

Quoted 11 lines

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

+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()?

Quoted 14 lines

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+
+    /* 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)

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1412

Quoted 7 lines

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

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

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1420

Quoted 9 lines

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

+        /*
+         * 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.

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1429

Quoted 10 lines

xen/arch/x86/hvm/vmx/vmcs.c +9 -0

+            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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1392

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

+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));

Quoted 9 lines

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1420

Quoted 8 lines

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

+        /*
+         * 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.

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1392

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

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

Quoted 1 line

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.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

+        /*
+         * 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) )

Quoted 2 lines

Quoted 1 line

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

Quoted 1 line

Quoted 1 line · On 17.04.15 at 08:51, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1392

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

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

Quoted 8 lines

Quoted 1 line

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?

Quoted 43 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 08:51, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1392

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 2 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 17.04.15 at 09:23, <[email protected]> wrote:

Quoted 43 lines · Jan Beulich

Quoted 43 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 08:51, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

Review

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

(quoted diff) +10 -0

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 51 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 09:23, <[email protected]> wrote:

Quoted 43 lines · Jan Beulich

Quoted 43 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 08:51, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

Review

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

(quoted diff) +10 -0

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 51 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 09:23, <[email protected]> wrote:

Quoted 43 lines · Jan Beulich

Quoted 43 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 08:51, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 21 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 05:10, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

Review

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 15.04.15 at 09:03, <[email protected]> wrote:

(quoted diff) +10 -0

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 24.04.15 at 08:32, <[email protected]> wrote:

Quoted 9 lines · Jan Beulich

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 09:23, <[email protected]> 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

Quoted 7 lines

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?

Quoted 29 lines

Quoted 1 line

Quoted 1 line · On 24.04.15 at 08:32, <[email protected]> wrote:

Quoted 9 lines · Jan Beulich

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 17.04.15 at 09:23, <[email protected]> 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

Quoted 7 lines

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.

Quoted 5 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

From: Kai Huang [mailto:[email protected]]
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 <[email protected]>

---
 xen/arch/x86/hvm/vmx/vmcs.c        | 178
+++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
 2 files changed, 187 insertions(+)

xen/arch/x86/hvm/vmx/vmcs.c +43 -0

index d120370..d3cb50f 100644
@@ -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;

xen/arch/x86/hvm/vmx/vmcs.c +6 -0

+    __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);

xen/arch/x86/hvm/vmx/vmcs.c +22 -0

+    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

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+     * 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

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

+
+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);

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

+}
+
+/*
+ * 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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +5 -0

+int vmx_domain_enable_pml(struct domain *d)
+{
+    struct vcpu *v;
+    int rc;
+

xen/arch/x86/hvm/vmx/vmcs.c +14 -0

+
+    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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +17 -0

+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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+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;

xen/include/asm-x86/hvm/vmx/vmcs.h +9 -1

index 2c679ac..ceb09bf 100644
@@ -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/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 1 line

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

Quoted 6 lines · Kevin

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 7 lines

Quoted 2 lines

+ 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
[email protected]
http://lists.xen.org/xen-devel

Quoted 10 lines · Tim Deegan

Quoted 6 lines · Kevin

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 18 lines · Kai Huang

Quoted 10 lines · Tim Deegan

Quoted 6 lines · Kevin

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 8 lines

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.

Quoted 27 lines · Tim Deegan

Quoted 18 lines · Kai Huang

Quoted 10 lines · Tim Deegan

Quoted 6 lines · Kevin

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

xen/arch/x86/hvm/vmx/vmcs.c +3 -0

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

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 34 lines · Kai Huang

Quoted 27 lines · Tim Deegan

Quoted 18 lines · Kai Huang

Quoted 10 lines · Tim Deegan

Quoted 6 lines · Kevin

Review

Quoted 4 lines

From: Kai Huang [mailto:[email protected]]

(quoted diff) +3 -0

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

Quoted 8 lines

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.

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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):

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

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?

Quoted 91 lines

From: Kai Huang [mailto:[email protected]]
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 <[email protected]>

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 178
+++++++++++++++++++++++++++++++++++++
  xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
  2 files changed, 187 insertions(+)

xen/arch/x86/hvm/vmx/vmcs.c +43 -0

index d120370..d3cb50f 100644
@@ -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;

xen/arch/x86/hvm/vmx/vmcs.c +6 -0

+    __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);

xen/arch/x86/hvm/vmx/vmcs.c +16 -0

+    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) )

Quoted 7 lines

xen/arch/x86/hvm/vmx/vmcs.c +6 -0

+        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

Quoted 19 lines

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+     * 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

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1421

Quoted 5 lines

xen/arch/x86/hvm/vmx/vmcs.c +5 -0

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

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1427

Quoted 6 lines

xen/arch/x86/hvm/vmx/vmcs.c +6 -0

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

Quoted 26 lines

xen/arch/x86/hvm/vmx/vmcs.c +8 -0

+
+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);

xen/arch/x86/hvm/vmx/vmcs.c +7 -0

+}
+
+/*
+ * 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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +5 -0

+int vmx_domain_enable_pml(struct domain *d)
+{
+    struct vcpu *v;
+    int rc;
+

xen/arch/x86/hvm/vmx/vmcs.c void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1460

Quoted 10 lines

xen/arch/x86/hvm/vmx/vmcs.c +10 -0

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

Quoted 75 lines

xen/arch/x86/hvm/vmx/vmcs.c +14 -0

+
+    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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +17 -0

+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.
+ */

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+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;

xen/include/asm-x86/hvm/vmx/vmcs.h +9 -1

index 2c679ac..ceb09bf 100644
@@ -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__ */
 
  /*
--

Quoted 1 line

2.1.0

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

From: Tian, Kevin
Sent: Friday, April 17, 2015 6:57 AM

Quoted 13 lines

xen/arch/x86/hvm/vmx/vmcs.c +13 -0

+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;

Review

Quoted 2 lines

(quoted diff) +2 -0

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