PML (Paging Modification Logging) support

PML requires A/D bit support so enable it for further use.

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

---
 xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
 xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
 xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
 xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
 4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
     P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
     P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
     P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
     P(cpu_has_vmx_vnmi, "Virtual NMI");
     P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
     P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644

xen/arch/x86/mm/p2m-ept.c +2 -1

+
     table = map_domain_page(new_ept.mfn);
     trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
 

@@ -244,7 +247,7 @@ 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);
+        /* A/D bits are inherited from superpage */
         ASSERT(!epte->avail3);
 
         ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);

xen/arch/x86/mm/p2m-ept.c +1 -0

+
     if ( !zalloc_cpumask_var(&ept->synced_mask) )
         return -ENOMEM;
 

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

index 6fce6aa..4528346 100644

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

         };
         u64 eptp;

@@ -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_SUPPORT                  0x00200000
 
 #define VMX_MISC_VMWRITE_ALL                    0x20000000
 

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

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

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

 #define EPT_2MB_SHIFT     16
 #define EPT_1GB_SHIFT     17

Quoted 30 lines · Kai Huang

PML requires A/D bit support so enable it for further use.

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

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
  xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
  xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
  xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
  4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
      P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
      P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
      P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
      P(cpu_has_vmx_vnmi, "Virtual NMI");
      P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
      P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644
@@ -233,6 +233,9 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &new_ept) )
          return 0;
 

xen/arch/x86/mm/p2m-ept.c static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry, · line 237

Quoted 2 lines

xen/arch/x86/mm/p2m-ept.c +2 -0

+    /* It's better to copy A bit of Middle entry from original entry */
+    new_ept.a = ept_entry->a;

Surely d needs to be propagated as well?

Would it make sense to extend
ept_set_middle_entry() to do all of new_ept setup in one location?

Quoted 18 lines

xen/arch/x86/mm/p2m-ept.c +2 -1

+
      table = map_domain_page(new_ept.mfn);
      trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
 

@@ -244,7 +247,7 @@ 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);
+        /* A/D bits are inherited from superpage */
          ASSERT(!epte->avail3);
 
          ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);

@@ -1071,6 +1074,9 @@ 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;
 

xen/arch/x86/mm/p2m-ept.c int ept_p2m_init(struct p2m_domain *p2m) · line 1078

Quoted 2 lines

xen/arch/x86/mm/p2m-ept.c +2 -0

+    /* Enable EPT A/D bit if it's supported by hardware */
+    ept->ept_ad = cpu_has_vmx_ept_ad_bit ? 1 : 0;

This will incur overhead on all EPT operations. It should only be enabled if pml is going to be in use. (I think you need reverse patches 1 and 2 in the series, and gate on pml_enable here)

Quoted 5 lines

xen/arch/x86/mm/p2m-ept.c +1 -0

+
      if ( !zalloc_cpumask_var(&ept->synced_mask) )
          return -ENOMEM;
 

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

Quoted 12 lines

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

index 6fce6aa..4528346 100644
@@ -62,7 +62,8 @@ struct ept_data {
      struct {
              u64 ept_mt :3,
                  ept_wl :3,
-                rsvd   :6,
+                ept_ad :1,
+                rsvd   :5,
                  asr    :52;

While you are making this change, can you add comments similar to ept_entry_t describing the bits?

Quoted 25 lines

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

          };
          u64 eptp;

@@ -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_SUPPORT                  0x00200000
 
  #define VMX_MISC_VMWRITE_ALL                    0x20000000
 

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

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

xen/include/asm-x86/hvm/vmx/vmx.h extern uint8_t posted_intr_vector; · line 266

Quoted 6 lines

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

@@ -261,6 +262,8 @@ extern uint8_t posted_intr_vector;
      (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_2MB)
  #define cpu_has_vmx_ept_invept_single_context   \
      (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
+#define cpu_has_vmx_ept_ad_bit                  \
+    (vmx_ept_vpid_cap & VMX_EPT_AD_BIT_SUPPORT)

I think cpu_has_vmx_ept_ad is sufficient, without the _bit suffix making it longer.

~Andrew

xen/include/asm-x86/hvm/vmx/vmx.h extern uint8_t posted_intr_vector; · line 269

Quoted 3 lines

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

  #define EPT_2MB_SHIFT     16
  #define EPT_1GB_SHIFT     17

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

Quoted 27 lines · Andrew Cooper

Quoted 26 lines · Kai Huang

PML requires A/D bit support so enable it for further use.

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

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
  xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
  xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
  xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
  4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
      P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
      P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
      P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
      P(cpu_has_vmx_vnmi, "Virtual NMI");
      P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
      P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644

Quoted 8 lines

Quoted 7 lines

xen/arch/x86/mm/p2m-ept.c +0 -0

@@ -233,6 +233,9 @@ static int ept_split_super_page(struct
 p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &new_ept) )
          return 0;
  +    /* It's better to copy A bit of Middle entry from original

entry */
+ new_ept.a = ept_entry->a;

Surely d needs to be propagated as well?

No it's not necessary. D-bit is not defined in middle level EPT table. Only leaf table entry has D-bit definition.

Quoted 2 lines

Would it make sense to extend
ept_set_middle_entry() to do all of new_ept setup in one location?

Yes it certainly makes sense to move A-bit propagation into ept_set_middle_entry, but this also requires adding additional original EPT entry pointer to ept_set_middle_entry as parameter. And ept_set_middle_entry is also called by ept_next_level, therefore changing it requires more code change, something like below. While I am fine with both, which solution do you prefer?

+++ b/xen/arch/x86/mm/p2m-ept.c

Quoted 25 lines · Kai Huang

Quoted 22 lines · Andrew Cooper

Quoted 21 lines · Kai Huang

PML requires A/D bit support so enable it for further use.

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

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
  xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
  xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
  xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
  4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
      P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
      P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
      P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
      P(cpu_has_vmx_vnmi, "Virtual NMI");
      P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");

Quoted 8 lines

Quoted 8 lines

xen/arch/x86/mm/p2m-ept.c static int ept_split_super_page(struct · line 234

Quoted 8 lines

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

      P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644
@@ -233,6 +233,9 @@ static int ept_split_super_page(struct
 p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &neThanks,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

xen/arch/x86/mm/p2m-ept.c +0 -0

          return 0;
  +    /* It's better to copy A bit of Middle entry from original

entry */
+ new_ept.a = ept_entry->a;

Quoted 3 lines

Quoted 1 line

Surely d needs to be propagated as well?

No it's not necessary. D-bit is not defined in middle level EPT table. Only leaf table entry has D-bit definition.

Ok - so the middle doesn't have a D.

What about the superpage having D set? Surely that needs propagated down to the new shattered leaves?

Quoted 12 lines

Quoted 3 lines

Would it make sense to extend
ept_set_middle_entry() to do all of new_ept setup in one location?

Yes it certainly makes sense to move A-bit propagation into ept_set_middle_entry, but this also requires adding additional original EPT entry pointer to ept_set_middle_entry as parameter. And ept_set_middle_entry is also called by ept_next_level, therefore changing it requires more code change, something like below. While I am fine with both, which solution do you prefer?

+++ b/xen/arch/x86/mm/p2m-ept.c

Review static void ept_p2m_type_to_flags(struct · line 215

Quoted 9 lines

(quoted diff) +2 -1

@@ -208,7 +208,8 @@ static void ept_p2m_type_to_flags(struct
 p2m_domain *p2m, ept_entry_t *entry,
 #define GUEST_TABLE_POD_PAGE    3
 
 /* Fill in middle levels of ept table */
-static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t
 *ept_entry)
+static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t
 *new_entry,
+        ept_entry_t *ori_entry)

const ept_entry_t *old_entry (for consistency with other similar functions, or even just 'new' and 'old' as you are already changing the names)

This looks fine. Being a static function with only two callsites, it is very likely to be inlined by the compiler.

~Andrew

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

Quoted 25 lines · Andrew Cooper

Quoted 24 lines · Kai Huang

Quoted 22 lines · Andrew Cooper

Quoted 21 lines · Kai Huang

PML requires A/D bit support so enable it for further use.

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

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
  xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
  xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
  xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
  4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
      P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
      P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
      P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
      P(cpu_has_vmx_vnmi, "Virtual NMI");
      P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

xen/arch/x86/mm/p2m-ept.c static int ept_split_super_page(struct · line 234

Quoted 8 lines

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

      P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644
@@ -233,6 +233,9 @@ static int ept_split_super_page(struct
 p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &neThanks,

-Kaiw_ept) )

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

xen/arch/x86/mm/p2m-ept.c +0 -0

          return 0;
  +    /* It's better to copy A bit of Middle entry from original

entry */
+ new_ept.a = ept_entry->a;

Surely d needs to be propagated as well?

No it's not necessary. D-bit is not defined in middle level EPT table. Only leaf table entry has D-bit definition.

Ok - so the middle doesn't have a D.

What about the superpage having D set? Surely that needs propagated down to the new shattered leaves?

Yes shattered leaves will inherit both A and D bits from the original superpage entry in " *epte = *ept_entry; " statement in below code in ept_split_super_page.

    for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
    {
        ept_entry_t *epte = table + i;

        *epte = *ept_entry;
        ......

Quoted 27 lines · Kai Huang

PML requires A/D bit support so enable it for further use.

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

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
  xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
  xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
  xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
  4 files changed, 15 insertions(+), 3 deletions(-)

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

index d614638..2f645fe 100644
@@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
      P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
      P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
      P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
+    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
      P(cpu_has_vmx_vnmi, "Virtual NMI");
      P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
      P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");

xen/arch/x86/mm/p2m-ept.c +0 -0

index c2d7720..8650092 100644

xen/arch/x86/mm/p2m-ept.c static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry, · line 237

Quoted 5 lines

xen/arch/x86/mm/p2m-ept.c +2 -0

@@ -233,6 +233,9 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &new_ept) )
          return 0;
 
+    /* It's better to copy A bit of Middle entry from original entry */
+    new_ept.a = ept_entry->a;

Surely d needs to be propagated as well? Would it make sense to extend ept_set_middle_entry() to do all of new_ept setup in one location?

Quoted 17 lines

xen/arch/x86/mm/p2m-ept.c +2 -1

+
      table = map_domain_page(new_ept.mfn);
      trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
 

@@ -244,7 +247,7 @@ 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);
+        /* A/D bits are inherited from superpage */
          ASSERT(!epte->avail3);
 
          ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);

@@ -1071,6 +1074,9 @@ 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;
 

xen/arch/x86/mm/p2m-ept.c int ept_p2m_init(struct p2m_domain *p2m) · line 1078

Quoted 2 lines

xen/arch/x86/mm/p2m-ept.c +2 -0

+    /* Enable EPT A/D bit if it's supported by hardware */
+    ept->ept_ad = cpu_has_vmx_ept_ad_bit ? 1 : 0;

This will incur overhead on all EPT operations. It should only be enabled if pml is going to be in use. (I think you need reverse patches 1 and 2 in the series, and gate on pml_enable here)

Quoted 9 lines

xen/arch/x86/mm/p2m-ept.c +1 -0

+
      if ( !zalloc_cpumask_var(&ept->synced_mask) )
          return -ENOMEM;
 

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

index 6fce6aa..4528346 100644

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

Quoted 7 lines

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

@@ -62,7 +62,8 @@ struct ept_data {
      struct {
              u64 ept_mt :3,
                  ept_wl :3,
-                rsvd   :6,
+                ept_ad :1,
+                rsvd   :5,
                  asr    :52;

While you are making this change, can you add comments similar to ept_entry_t describing the bits?

Quoted 25 lines

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

          };
          u64 eptp;

@@ -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_SUPPORT                  0x00200000
 
  #define VMX_MISC_VMWRITE_ALL                    0x20000000
 

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

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

xen/include/asm-x86/hvm/vmx/vmx.h extern uint8_t posted_intr_vector; · line 266

Quoted 5 lines

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

@@ -261,6 +262,8 @@ extern uint8_t posted_intr_vector;
      (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_2MB)
  #define cpu_has_vmx_ept_invept_single_context   \
      (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
+#define cpu_has_vmx_ept_ad_bit                  \
+    (vmx_ept_vpid_cap & VMX_EPT_AD_BIT_SUPPORT)

I think cpu_has_vmx_ept_ad is sufficient, without the _bit suffix making it longer.

~Andrew

xen/include/asm-x86/hvm/vmx/vmx.h extern uint8_t posted_intr_vector; · line 269

Quoted 3 lines

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

  #define EPT_2MB_SHIFT     16
  #define EPT_1GB_SHIFT     17

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

Quoted 14 lines

Quoted 14 lines

+

      table = map_domain_page(new_ept.mfn);
      trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
  @@ -244,7 +247,7 @@ 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);

xen/arch/x86/mm/p2m-ept.c +1 -1

-        ASSERT(!epte->rsvd1);
+        /* A/D bits are inherited from superpage */
          ASSERT(!epte->avail3);
            ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);

@@ -1071,6 +1074,9 @@ int ept_p2m_init(struct p2m_domain *p2m)

Quoted 8 lines

Quoted 5 lines

(quoted diff) +0 -0

      /* set EPT page-walk length, now it's actual walk length - 1,

i.e. 3 */

      ept->ept_wl = 3;

+ /* Enable EPT A/D bit if it's supported by hardware */
+ ept->ept_ad = cpu_has_vmx_ept_ad_bit ? 1 : 0;

This will incur overhead on all EPT operations. It should only be enabled if pml is going to be in use. (I think you need reverse patches 1 and 2 in the series, and gate on pml_enable here)

Hi Andrew,

I'd like to also put patch 3 (PML feature detection) before this patch, as it's better to use cpu_has_vmx_pml to gate A/D bit enabling here. Theoretically simple "pml_enable = 1" here doesn't guarantee cpu_has_vmx_pml to be true, as PML may be turned off during vmx_init_vmcs_config.

And in this case I also want to delete below code, as if PML is not enabled, below code will print but actually EPT A/D bits is not enabled in hardware.

   P(cpu_has_vmx_ept_ad, "EPT A/D bit");

xen/arch/x86/mm/p2m-ept.c static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry, · line 237

Quoted 6 lines · Kai Huang

xen/arch/x86/mm/p2m-ept.c +2 -0

@@ -233,6 +233,9 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
      if ( !ept_set_middle_entry(p2m, &new_ept) )
          return 0;
 
+    /* It's better to copy A bit of Middle entry from original entry */
+    new_ept.a = ept_entry->a;

Shouldn't this A bit always be set to 1? After all we're not using it for anything, and it doesn't mean the same thing as the A bit in the superpage entry did.

In fact, thinking about that, we should probably be setting _all_ the A bits to 1, even in leaf entries, to avoid the overhead of the MMU having to write them later.

Cheers,

Tim.

Quoted 3 lines

In fact, thinking about that, we should probably be setting _all_ the A bits to 1, even in leaf entries, to avoid the overhead of the MMU having to write them later.

I suppose you mean A bits of leaf entries of new shattered 4K pages? Yes we can, but looks it's more reasonable to inherit according to original A bit of super page.

Quoted 6 lines · Kai Huang

Quoted 3 lines

In fact, thinking about that, we should probably be setting _all_ the A bits to 1, even in leaf entries, to avoid the overhead of the MMU having to write them later.

I suppose you mean A bits of leaf entries of new shattered 4K pages? Yes we can, but looks it's more reasonable to inherit according to original A bit of super page.

Yes, inheriting is best, but we should have already set the A bit of the superpage to 1 as well, for the same reasons. IOW we should set every A bit in the whole system to 1 all the time.

BTW, this is what we do with A/D bits in normal pagetables in Xen too -- the default is to set them all the time, so the MMU doesn't have to fix them up later.

Cheers,

Tim.

Quoted 7 lines · Tim Deegan

Quoted 6 lines · Kai Huang

Quoted 3 lines

In fact, thinking about that, we should probably be setting _all_ the A bits to 1, even in leaf entries, to avoid the overhead of the MMU having to write them later.

I suppose you mean A bits of leaf entries of new shattered 4K pages? Yes we can, but looks it's more reasonable to inherit according to original A bit of super page.

Quoted 7 lines

Yes, inheriting is best, but we should have already set the A bit of the superpage to 1 as well, for the same reasons. IOW we should set every A bit in the whole system to 1 all the time.

BTW, this is what we do with A/D bits in normal pagetables in Xen too -- the default is to set them all the time, so the MMU doesn't have to fix them up later.

Sure then we can set all A bit to 1, which can be done by setting A bit properly in ept_p2m_type_to_flags I think.

What does normal pagetables mean here? I don't think you mean shadow page table, as shadow page table needs to sync A/D bits with guest table, right?

Quoted 20 lines · Kai Huang

Quoted 7 lines · Tim Deegan

Quoted 6 lines · Kai Huang

Quoted 3 lines

In fact, thinking about that, we should probably be setting _all_ the A bits to 1, even in leaf entries, to avoid the overhead of the MMU having to write them later.

I suppose you mean A bits of leaf entries of new shattered 4K pages? Yes we can, but looks it's more reasonable to inherit according to original A bit of super page.

Quoted 7 lines

Yes, inheriting is best, but we should have already set the A bit of the superpage to 1 as well, for the same reasons. IOW we should set every A bit in the whole system to 1 all the time.

BTW, this is what we do with A/D bits in normal pagetables in Xen too -- the default is to set them all the time, so the MMU doesn't have to fix them up later.

Sure then we can set all A bit to 1, which can be done by setting A bit properly in ept_p2m_type_to_flags I think.

Quoted 3 lines

What does normal pagetables mean here? I don't think you mean shadow page table, as shadow page table needs to sync A/D bits with guest table, right?

I meant xen's own pagetables, that it maps its own text and data with. But IIRC we also set the Accessed bit in all shadow pagetable entries -- fixing up the A/D bits in the guest PTEs happens in the pagefault handler.

Tim.

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

A top level EPT parameter "ept=<options>" and a sub boolean "pml_enable" 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 | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

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

index 2f645fe..9b20a4b 100644
@@ -50,6 +50,16 @@ boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled);
 static bool_t __read_mostly opt_apicv_enabled = 1;

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

+{
+    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") )
+            pml_enable = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+
 static void __init vmx_display_features(void)
 {
     int printed = 0;

Quoted 5 lines · Kai Huang

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

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

Please patch docs/misc/xen-command-line.markdown as well. See the existing "psr" option as a similar example.

Also, as indicated in patch 1, I think patches 1 and 2 need swapping in the series.

Quoted 13 lines

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

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

index 2f645fe..9b20a4b 100644
@@ -50,6 +50,16 @@ boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled);
  static bool_t __read_mostly opt_apicv_enabled = 1;
  boolean_param("apicv", opt_apicv_enabled);
 

xen/arch/x86/hvm/vmx/vmcs.c boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled); · line 60

Quoted 8 lines

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

+static void parse_ept_param(char *s);
+/*
+ * The 'ept' parameter controls functionalities that depend on, or impact the
+ * EPT mechanism. Optional comma separated value may contain:
+ *
+ *  pml                 Enable PML
+ */
+custom_param("ept", parse_ept_param);

It is common to put the custom_param() call below parse_ept_param() so you don't need to forward-declare the function. The comment can happily live at the top of parse_ept_param().

Quoted 10 lines

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

+static bool_t __read_mostly pml_enable = 0;
+
  /*
   * These two parameters are used to config the controls for Pause-Loop Exiting:
   * ple_gap:    upper bound on the amount of time between two successive

@@ -92,6 +102,28 @@ DEFINE_PER_CPU(bool_t, vmxon);
  static u32 vmcs_revision_id __read_mostly;
  u64 __read_mostly vmx_basic_msr;
 

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 105

Quoted 1 line

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

+/* Copied from parse_iommu_param */

Not a useful comment, as it is likely to diverge in the future.

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 106

Quoted 2 lines

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

+static void parse_ept_param(char *s)

__init

~Andrew

Quoted 13 lines

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

+{
+    char *ss;
+    int val;
+
+    do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 129

Quoted 10 lines

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

+        if ( !strcmp(s, "pml") )
+            pml_enable = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+
  static void __init vmx_display_features(void)
  {
      int printed = 0;

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

Quoted 5 lines · Kai Huang

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

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

Please patch docs/misc/xen-command-line.markdown as well. See the existing "psr" option as a similar example.

Also, as indicated in patch 1, I think patches 1 and 2 need swapping in the series.

Quoted 12 lines

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

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

index 2f645fe..9b20a4b 100644
@@ -50,6 +50,16 @@ boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled);
  static bool_t __read_mostly opt_apicv_enabled = 1;
  boolean_param("apicv", opt_apicv_enabled);
 

xen/arch/x86/hvm/vmx/vmcs.c boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled); · line 60

Quoted 8 lines

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

+static void parse_ept_param(char *s);
+/*
+ * The 'ept' parameter controls functionalities that depend on, or impact the
+ * EPT mechanism. Optional comma separated value may contain:
+ *
+ *  pml                 Enable PML
+ */
+custom_param("ept", parse_ept_param);

It is common to put the custom_param() call below parse_ept_param() so you don't need to forward-declare the function. The comment can happily live at the top of parse_ept_param().

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 105

Quoted 11 lines

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

+static bool_t __read_mostly pml_enable = 0;
+
  /*
   * These two parameters are used to config the controls for Pause-Loop Exiting:
   * ple_gap:    upper bound on the amount of time between two successive

@@ -92,6 +102,28 @@ DEFINE_PER_CPU(bool_t, vmxon);
  static u32 vmcs_revision_id __read_mostly;
  u64 __read_mostly vmx_basic_msr;
 
+/* Copied from parse_iommu_param */

Not a useful comment, as it is likely to diverge in the future.

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 106

Quoted 1 line

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

+static void parse_ept_param(char *s)

__init

~Andrew

Quoted 13 lines

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

+{
+    char *ss;
+    int val;
+
+    do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+

xen/arch/x86/hvm/vmx/vmcs.c DEFINE_PER_CPU(bool_t, vmxon); · line 129

Quoted 10 lines

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

+        if ( !strcmp(s, "pml") )
+            pml_enable = val;
+
+        s = ss + 1;
+    } while ( ss );
+}
+
  static void __init vmx_display_features(void)
  {
      int printed = 0;

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

Quoted 30 lines · Andrew Cooper

Quoted 7 lines · Kai Huang

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

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

Please patch docs/misc/xen-command-line.markdown as well. See the existing "psr" option as a similar example.

Also, as indicated in patch 1, I think patches 1 and 2 need swapping in the series.

Quoted 16 lines

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

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

index 2f645fe..9b20a4b 100644
@@ -50,6 +50,16 @@ boolean_param("unrestricted_guest",
 opt_unrestricted_guest_enabled);
  static bool_t __read_mostly opt_apicv_enabled = 1;
  boolean_param("apicv", opt_apicv_enabled);
  +static void parse_ept_param(char *s);
+/*
+ * The 'ept' parameter controls functionalities that depend on, or

impact the

Quoted 8 lines

Quoted 5 lines

+ * EPT mechanism. Optional comma separated value may contain:
+ *
+ * pml Enable PML
+ */
+custom_param("ept", parse_ept_param);

It is common to put the custom_param() call below parse_ept_param() so you don't need to forward-declare the function. The comment can happily live at the top of parse_ept_param().

Hi Andrew,

Looks it's better to keep parse_ept_param() below custom_param(), as simply moving parse_ept_param() above custom_param() results in below error (I also changed pml_enable to opt_pml_enabled), as it references opt_pml_enabled variable, which is defined below custom_param(). Actually for "iommu=<options>" parameter, parse_iommu_param() was also placed below custom_param().

What do you think?

vmcs.c: In function ‘parse_ept_param’:
vmcs.c:74:13: error: ‘opt_pml_enabled’ undeclared (first use in this function)

             opt_pml_enabled = val;
             ^

vmcs.c:74:13: note: each undeclared identifier is reported only once for each function it appears in
vmcs.c: At top level:
vmcs.c:81:29: error: ‘opt_pml_enabled’ defined but not used [-Werror=unused-variable]
static bool_t __read_mostly opt_pml_enabled = 0;

Quoted 55 lines · Kai Huang

Quoted 30 lines · Andrew Cooper

Quoted 7 lines · Kai Huang

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

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

Please patch docs/misc/xen-command-line.markdown as well. See the existing "psr" option as a similar example.

Also, as indicated in patch 1, I think patches 1 and 2 need swapping in the series.

Quoted 16 lines

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

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

index 2f645fe..9b20a4b 100644
@@ -50,6 +50,16 @@ boolean_param("unrestricted_guest",
 opt_unrestricted_guest_enabled);
  static bool_t __read_mostly opt_apicv_enabled = 1;
  boolean_param("apicv", opt_apicv_enabled);
  +static void parse_ept_param(char *s);
+/*
+ * The 'ept' parameter controls functionalities that depend on, or

impact the

Quoted 8 lines

Quoted 5 lines

+ * EPT mechanism. Optional comma separated value may contain:
+ *
+ * pml Enable PML
+ */
+custom_param("ept", parse_ept_param);

It is common to put the custom_param() call below parse_ept_param() so you don't need to forward-declare the function. The comment can happily live at the top of parse_ept_param().

Hi Andrew,

Looks it's better to keep parse_ept_param() below custom_param(), as simply moving parse_ept_param() above custom_param() results in below error (I also changed pml_enable to opt_pml_enabled), as it references opt_pml_enabled variable, which is defined below custom_param(). Actually for "iommu=<options>" parameter, parse_iommu_param() was also placed below custom_param().

What do you think?

vmcs.c: In function ‘parse_ept_param’:
vmcs.c:74:13: error: ‘opt_pml_enabled’ undeclared (first use in this function)

Quoted 8 lines

             opt_pml_enabled = val;
             ^

vmcs.c:74:13: note: each undeclared identifier is reported only once for each function it appears in
vmcs.c: At top level:
vmcs.c:81:29: error: ‘opt_pml_enabled’ defined but not used [-Werror=unused-variable]
static bool_t __read_mostly opt_pml_enabled = 0;

The most concise way of doing this is:

static bool_t __read_mostly opt_pml_enabled = 0;

static void parse_ept_param(char *s)
{
...
}
custom_param("ept", parse_ept_param);

~Andrew

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

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        | 18 ++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  5 +++++
 xen/include/asm-x86/hvm/vmx/vmx.h  |  1 +
 3 files changed, 24 insertions(+)

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

index 9b20a4b..2798b0b 100644
@@ -143,6 +143,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 )

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

+            opt |= SECONDARY_EXEC_ENABLE_PML;
 
         /*
          * "APIC Register Virtualization" and "Virtual Interrupt Delivery"

@@ -286,6 +289,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 it anyway.
+	 */
+	if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT_SUPPORT) )
+		_vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
     }
 
     if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )

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

     if ( (_vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
           ple_gap == 0 )
     {

@@ -1041,6 +1056,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 +5 -0

index 4528346..47b4df2 100644
@@ -216,6 +216,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

@@ -276,6 +277,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
 

@@ -320,6 +323,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,

@@ -333,6 +337,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 9afd351..c0e352d 100644
@@ -186,6 +186,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

Quoted 22 lines · 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 <[email protected]>

---
  xen/arch/x86/hvm/vmx/vmcs.c        | 18 ++++++++++++++++++
  xen/include/asm-x86/hvm/vmx/vmcs.h |  5 +++++
  xen/include/asm-x86/hvm/vmx/vmx.h  |  1 +
  3 files changed, 24 insertions(+)

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

index 9b20a4b..2798b0b 100644
@@ -143,6 +143,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
 

xen/arch/x86/hvm/vmx/vmcs.c static int vmx_init_vmcs_config(void) · line 244

Quoted 6 lines

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

      if ( !printed )

@@ -240,6 +241,8 @@ static int vmx_init_vmcs_config(void)
              opt |= SECONDARY_EXEC_ENABLE_VPID;
          if ( opt_unrestricted_guest_enabled )
              opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;
+        if ( pml_enable )

This should be named opt_pml_enable in patch 1 or 2 to identify that it is a command line option.

Quoted 24 lines

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

+            opt |= SECONDARY_EXEC_ENABLE_PML;
 
          /*
           * "APIC Register Virtualization" and "Virtual Interrupt Delivery"

@@ -286,6 +289,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 it anyway.
+	 */
+	if ( !(_vmx_ept_vpid_cap & VMX_EPT_AD_BIT_SUPPORT) )
+		_vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
      }
 
      if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT )

@@ -306,6 +317,10 @@ static int vmx_init_vmcs_config(void)
                    SECONDARY_EXEC_UNRESTRICTED_GUEST);
      }
 

xen/arch/x86/hvm/vmx/vmcs.c static int vmx_init_vmcs_config(void) · line 323

Quoted 4 lines

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

+    /* PML cannot be supported if we don't use EPT */
+    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) )
+        _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+

Somewhere in here you should clear pml_enable if hardware doesn't support it.

~Andrew

Quoted 55 lines

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

      if ( (_vmx_secondary_exec_control & SECONDARY_EXEC_PAUSE_LOOP_EXITING) &&
            ple_gap == 0 )
      {

@@ -1041,6 +1056,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 +5 -0

index 4528346..47b4df2 100644
@@ -216,6 +216,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

@@ -276,6 +277,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
 

@@ -320,6 +323,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,

@@ -333,6 +337,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 +0 -0

index 9afd351..c0e352d 100644

xen/include/asm-x86/hvm/vmx/vmx.h static inline unsigned long pi_get_pir(struct pi_desc *pi_desc, int group) · line 192

Quoted 7 lines

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

@@ -186,6 +186,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

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

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 47b4df2..8cc1122 100644
@@ -71,8 +71,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 -0

 };
 
 int vmx_create_vmcs(struct vcpu *v);

Quoted 29 lines · 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 <[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 47b4df2..8cc1122 100644
@@ -71,8 +71,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 7 lines

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

  struct pi_desc {

@@ -143,6 +147,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 PML_ENTITY_NUM      512

This is the number of pml entries, not entities. NR_PML_ENTRIES perhaps?

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

Quoted 2 lines

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

+    struct page_info	*pml_pg;

Please align the fields vertically like vmwrite_bitmap.

~Andrew

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

Quoted 3 lines

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

  };
 
  int vmx_create_vmcs(struct vcpu *v);

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

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        | 190 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h |   9 ++
 2 files changed, 199 insertions(+)

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

index 2798b0b..17cbef4 100644

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

+}
+
+int vmx_vcpu_enable_pml(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    ASSERT(!vmx_vcpu_pml_enabled(v));
+
+    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
+    if ( !v->arch.hvm_vmx.pml_pg )
+        return -ENOMEM;

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

+
+    vmx_vmcs_exit(v);
+
+    return 0;
+}
+
+void vmx_vcpu_disable_pml(struct vcpu *v)
+{
+    ASSERT(vmx_vcpu_pml_enabled(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));

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

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

+        unsigned long gfn;
+        mfn_t mfn;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;

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

+            continue;
+        }
+
+        /*
+         * 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_dirty(v->domain, mfn_x(mfn));
+    }
+

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

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

+    }
+
+    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 -EINVAL;
+}
+
+/*
+ * 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(vmx_domain_pml_enabled(d));
+
+    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(vmx_domain_pml_enabled(d));
+
+    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 -0

index 8cc1122..939d097 100644
@@ -499,6 +499,15 @@ static inline int vmx_add_host_load_msr(u32 msr)
 
 DECLARE_PER_CPU(bool_t, vmxon);
 
+int vmx_vcpu_pml_enabled(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);
+int vmx_domain_pml_enabled(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 17 lines · 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 <[email protected]>

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

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

index 2798b0b..17cbef4 100644
@@ -1326,6 +1326,196 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector)
                  &v->arch.hvm_vmx.eoi_exitmap_changed);
  }
 

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

Quoted 1 line

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

+int vmx_vcpu_pml_enabled(struct vcpu *v)

bool_t vmx_vcpu_pml_enabled(const struct vcpu *v)

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

Quoted 4 lines

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

+{
+    return (v->arch.hvm_vmx.secondary_exec_control &
+            SECONDARY_EXEC_ENABLE_PML) ? 1 : 0;

This would be slightly shorter as !!(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)

Quoted 15 lines

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

+}
+
+int vmx_vcpu_enable_pml(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    ASSERT(!vmx_vcpu_pml_enabled(v));
+
+    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);
+

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

Quoted 7 lines

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

+    __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) << PAGE_SHIFT);
+    __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
+
+    v->arch.hvm_vmx.secondary_exec_control |= SECONDARY_EXEC_ENABLE_PML;
+
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+            v->arch.hvm_vmx.secondary_exec_control);

Alignment.

Quoted 32 lines

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

+
+    vmx_vmcs_exit(v);
+
+    return 0;
+}
+
+void vmx_vcpu_disable_pml(struct vcpu *v)
+{
+    ASSERT(vmx_vcpu_pml_enabled(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);
+

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

Quoted 7 lines

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

+    __vmread(GUEST_PML_INDEX, &pml_idx);
+
+    /* Do nothing if PML buffer is empty */
+    if ( pml_idx == (PML_ENTITY_NUM - 1) )
+        goto out;
+
+    pml_buf = map_domain_page(page_to_mfn(v->arch.hvm_vmx.pml_pg));

__map_domain_page() is a wrapper which takes a struct page_info

Quoted 5 lines

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

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

Quoted 10 lines

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

+     * entity.
+     */
+    if (pml_idx >= PML_ENTITY_NUM)
+        pml_idx = 0;
+    else
+        pml_idx++;
+
+    for ( ; pml_idx < PML_ENTITY_NUM; pml_idx++ )
+    {
+        struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);

This p2m_get_host_p2m() call should be hoisted out of the loop.

Quoted 7 lines

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

+        unsigned long gfn;
+        mfn_t mfn;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;

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

Quoted 10 lines

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

+        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+        if ( mfn_x(mfn) == INVALID_MFN )
+        {
+            /*
+             * Either EPT table entry for mapping the GFN has been destroyed, or
+             * there's something wrong with hardware behavior, in both cases we
+             * should report a warning.
+             */
+            dprintk(XENLOG_WARNING, "PML: vcpu %d: invalid GPA 0x%lx logged\n",
+                    v->vcpu_id, pml_buf[pml_idx]);

It would be shorter to log gfn rather than gpa.

Quoted 18 lines

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

+            continue;
+        }
+
+        /*
+         * 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_dirty(v->domain, mfn_x(mfn));
+    }
+
+    unmap_domain_page(pml_buf);
+
+    /* Reset PML index */

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

Quoted 7 lines

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

+    __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
+
+out:
+    vmx_vmcs_exit(v);
+}
+
+int vmx_domain_pml_enabled(struct domain *d)

bool_t and const as per vcpu variant.

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

Quoted 8 lines

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

+{
+    return (d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED) ? 1 : 0;
+}
+
+/*
+ * This function enables PML for particular domain. It should be called when
+ * domain is paused.

In which case assert that the domain is paused, or call domain_pause() yourself to take an extra pause refcount.

Quoted 6 lines

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

+ *
+ * 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 void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) · line 1467

Quoted 10 lines

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

+int vmx_domain_enable_pml(struct domain *d)
+{
+    struct vcpu *v;
+
+    ASSERT(!vmx_domain_pml_enabled(d));
+
+    for_each_vcpu( d, v )
+    {
+        if ( vmx_vcpu_enable_pml(v) )
+            goto error;

Please catch the actual rc from vmx_vcpu_enable_pml() and propagate out of this function, rather than clobbering -ENOMEM with -EINVAL.

Also, per Xen style, you can drop the braces.

~Andrew

Quoted 64 lines

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

+    }
+
+    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 -EINVAL;
+}
+
+/*
+ * 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(vmx_domain_pml_enabled(d));
+
+    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(vmx_domain_pml_enabled(d));
+
+    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 +2 -0

index 8cc1122..939d097 100644
@@ -499,6 +499,15 @@ static inline int vmx_add_host_load_msr(u32 msr)
 
  DECLARE_PER_CPU(bool_t, vmxon);
 
+int vmx_vcpu_pml_enabled(struct vcpu *v);
+int vmx_vcpu_enable_pml(struct vcpu *v);

xen/include/asm-x86/hvm/vmx/vmcs.h static inline int vmx_add_host_load_msr(u32 msr) · line 513

Quoted 10 lines

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

+void vmx_vcpu_disable_pml(struct vcpu *v);
+void vmx_vcpu_flush_pml_buffer(struct vcpu *v);
+int vmx_domain_pml_enabled(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__ */
 
  /*

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

Quoted 18 lines · Andrew Cooper

Quoted 16 lines · Kai Huang

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

+}
+
+int vmx_vcpu_enable_pml(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    ASSERT(!vmx_vcpu_pml_enabled(v));
+
+    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);

Quoted 8 lines

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

Quoted 7 lines

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

+    __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
+
+    v->arch.hvm_vmx.secondary_exec_control |=

SECONDARY_EXEC_ENABLE_PML;

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

+
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+            v->arch.hvm_vmx.secondary_exec_control);

Alignment.

Do you mean to put 'v->arch.hvm_vmx.secondary_exec_control' to the same line with '__vmwrite(SECONDARY_VM_EXEC_CONTROL,'? In this case the number of characters will be 81.

Quoted 21 lines

Quoted 21 lines

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

+        unsigned long gfn;
+        mfn_t mfn;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
+        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+        if ( mfn_x(mfn) == INVALID_MFN )
+        {
+            /*
+             * Either EPT table entry for mapping the GFN has been

destroyed, or
+ * there's something wrong with hardware behavior, in
both cases we

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

+             * should report a warning.
+             */
+            dprintk(XENLOG_WARNING, "PML: vcpu %d: invalid GPA

0x%lx logged\n",
+ v->vcpu_id, pml_buf[pml_idx]);

Quoted 1 line

It would be shorter to log gfn rather than gpa.

Will do. And I'd also like to add the domain ID in the warning info.

Quoted 13 lines

Quoted 11 lines

(quoted diff) +2 -0

+{
+    return (d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED)

? 1 : 0;

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

+}
+
+/*
+ * This function enables PML for particular domain. It should be

called when
+ * domain is paused.

In which case assert that the domain is paused, or call domain_pause() yourself to take an extra pause refcount.

Which function should I use to assert domain is paused? I didn't find a function like "domain_paused". Is below good enough?

ASSERT(atomic_read(&d->pause_count));

Quoted 29 lines · Kai Huang

Quoted 26 lines · Andrew Cooper

Quoted 20 lines · Kai Huang

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

+}
+
+int vmx_vcpu_enable_pml(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+
+    ASSERT(!vmx_vcpu_pml_enabled(v));
+
+    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);

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

+    __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
+
+    v->arch.hvm_vmx.secondary_exec_control |=

SECONDARY_EXEC_ENABLE_PML;

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

Quoted 3 lines

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

+
+    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
+            v->arch.hvm_vmx.secondary_exec_control);

Alignment.

Quoted 3 lines

Do you mean to put 'v->arch.hvm_vmx.secondary_exec_control' to the same line with '__vmwrite(SECONDARY_VM_EXEC_CONTROL,'? In this case the number of characters will be 81.

Splitting the line is fine. The v should be vertically in line with S from SECONDARY

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

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

+        unsigned long gfn;
+        mfn_t mfn;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
+        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+        if ( mfn_x(mfn) == INVALID_MFN )
+        {
+            /*
+             * Either EPT table entry for mapping the GFN has been

destroyed, or
+ * there's something wrong with hardware behavior, in
both cases we

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

+             * should report a warning.
+             */
+            dprintk(XENLOG_WARNING, "PML: vcpu %d: invalid GPA

0x%lx logged\n",
+ v->vcpu_id, pml_buf[pml_idx]);

Quoted 2 lines

Quoted 1 line

It would be shorter to log gfn rather than gpa.

Will do. And I'd also like to add the domain ID in the warning info.

Ah of course - dprintk() doesn't identify current(). Use %pv with v.

Quoted 13 lines

Quoted 13 lines

Quoted 11 lines

(quoted diff) +2 -0

+{
+    return (d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED)

? 1 : 0;

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

+}
+
+/*
+ * This function enables PML for particular domain. It should be

called when
+ * domain is paused.

In which case assert that the domain is paused, or call domain_pause() yourself to take an extra pause refcount.

Quoted 4 lines

Which function should I use to assert domain is paused? I didn't find a function like "domain_paused". Is below good enough?

ASSERT(atomic_read(&d->pause_count));

Hmm - we indeed don't have an appropriate helper. That ASSERT() will do for now.

~Andrew

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

xen/arch/x86/hvm/vmx/vmcs.c +27 -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);
+
+    /* Do nothing if PML buffer is empty */
+    if ( pml_idx == (PML_ENTITY_NUM - 1) )
+        goto out;
+
+    pml_buf = map_domain_page(page_to_mfn(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 >= PML_ENTITY_NUM)
+        pml_idx = 0;
+    else
+        pml_idx++;
+

Hi,

Quoted 27 lines · Kai Huang

xen/arch/x86/hvm/vmx/vmcs.c +27 -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);
+
+    /* Do nothing if PML buffer is empty */
+    if ( pml_idx == (PML_ENTITY_NUM - 1) )
+        goto out;
+
+    pml_buf = map_domain_page(page_to_mfn(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 >= PML_ENTITY_NUM)
+        pml_idx = 0;
+    else
+        pml_idx++;
+

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

Quoted 10 lines

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

+    for ( ; pml_idx < PML_ENTITY_NUM; pml_idx++ )
+    {
+        struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+        unsigned long gfn;
+        mfn_t mfn;
+        p2m_type_t t;
+        p2m_access_t a;
+
+        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
+        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);

Please don't call p2m->get_entry() directly -- that interface should only be used inside the p2m code. As it happens, I don't think this lookup is correct anyway: the logging only sees races (which are not interesting) or buggy hardware (which is not worth the extra lookup to detect).

So you only need this to get 'mfn' to pass to paging_mark_dirty().

That's also buggy, because there's no locking here to make sure gfn->mfn->gfn ends up in the right place. :(

I think the right thing to do is:

- split paging_park_dirty() into paging_mark_gfn_dirty() (the bulk of the current function) and a paging_mark_dirty() wrapper that does get_gpfn_from_mfn(mfn_x(gmfn)) and calls paging_mark_gfn_dirty().

- call paging_mark_gfn_dirty() from vmx_vcpu_flush_pml_buffer().

That will avoid _two_ p2m lookups in this function. :)

Cheers,

Tim.

Quoted 4 lines · Tim Deegan

Hi,

That's also buggy, because there's no locking here to make sure gfn->mfn->gfn ends up in the right place. :(

Yes you are right. Thanks for pointing out.

Just curious, looks if I use get_gfn_type_access and put_gfn here, the gfn->mfn->gfn can be guaranteed, and it's safe to use them here, right?

Quoted 10 lines · Kai Huang

Quoted 4 lines · Tim Deegan

Hi,

That's also buggy, because there's no locking here to make sure gfn->mfn->gfn ends up in the right place. :(

Yes you are right. Thanks for pointing out.

Just curious, looks if I use get_gfn_type_access and put_gfn here, the gfn->mfn->gfn can be guaranteed, and it's safe to use them here, right?

Yes, that would be the right way to do it.

In this particular case, there's another race too: the gfn might have been entirely deleted after it's logged by PML but before the entry is processed here. I don't think that's a problem here, because if the gfn's not mapped any more it's not relevant that it's dirty. Just mentioning it out of interest.

Cheers,

Tim.

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

Quoted 4 lines · Kai Huang

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

+void vmx_vcpu_disable_pml(struct vcpu *v)
+{
+    ASSERT(vmx_vcpu_pml_enabled(v));
+

I think this function ought to call vmx_vcpu_flush_pml_buffer() before disabling PML. That way we don't need to worry about losing any information if a guest vcpu is reset or offlined during migration.

Cheers,

Tim.

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

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

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

index 453bcc5..fce3aa2 100644
@@ -116,6 +116,30 @@ 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.
+     *

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

+            vmx_destroy_vmcs(v);
+            return rc;
+        }
+    }
+
     vpmu_initialise(v);
 
     vmx_install_vlapic_mapping(v);

Quoted 27 lines · 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 <[email protected]>

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

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

index 453bcc5..fce3aa2 100644
@@ -116,6 +116,30 @@ 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