Deter enabling of EPT A/D bit plus coding style fix

PATCH In review Kai Huang 4 participants 9 messages Oct 20, 2015 – Oct 21, 2015

Share & tools

Kai Huang

author

2015-10-20 02:34 UTC #0 lore raw

cover Deter enabling of EPT A/D bit plus coding style fix

Patch 1 is the v2 of defering enabling of EPT A/D bit until PML get enabled, with comments from Jan in v1 addressed. Patch 2 is coding style fix of for_each_vcpu to existing PML functions according to Jan.

Kai Huang (2):
x86/ept: defer enabling of EPT A/D bit until PML get enabled. x86/vmx: fix coding style of PML functions

 xen/arch/x86/hvm/vmx/vmcs.c        | 32 ++++++++++++++++++++++++++++----
 xen/arch/x86/mm/p2m-ept.c          | 24 ++++++++++++++++++++----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
 3 files changed, 50 insertions(+), 8 deletions(-)

Signature · 1 line

2.1.4

Kai Huang

author

2015-10-20 02:34 UTC #1 lore raw

PATCH 1/2 x86/ept: defer enabling of EPT A/D bit until PML get enabled.

Existing PML implementation turns on EPT A/D bit unconditionally if PML is supported by hardware. This works but enabling of EPT A/D bit can be deferred until PML get enabled. There's no point in enabling the extra feature for every domain when we're not meaning to use it (yet).

Also added ASSERT of domain having been paused to ept_flush_pml_buffers to make it consistent with ept_enable{disable}_pml.

Sanity live migration and GUI display were tested on Broadwell Machine.

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

---
 xen/arch/x86/hvm/vmx/vmcs.c        | 24 ++++++++++++++++++++++++
 xen/arch/x86/mm/p2m-ept.c          | 24 ++++++++++++++++++++----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
 3 files changed, 46 insertions(+), 4 deletions(-)

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

index 3592a88..c11f3ec 100644
@@ -1553,6 +1553,30 @@ void vmx_domain_flush_pml_buffers(struct domain *d)
         vmx_vcpu_flush_pml_buffer(v);
 }
 
+static void vmx_vcpu_update_eptp(struct vcpu *v, u64 eptp)
+{
+    vmx_vmcs_enter(v);
+    __vmwrite(EPT_POINTER, eptp);
+    vmx_vmcs_exit(v);
+}
+
+/*
+ * Update EPTP data to VMCS of all vcpus of the domain. Must be called when
+ * domain is paused.
+ */
+void vmx_domain_update_eptp(struct domain *d)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct vcpu *v;
+
+    ASSERT(atomic_read(&d->pause_count));
+
+    for_each_vcpu ( d, v )
+        vmx_vcpu_update_eptp(v, ept_get_eptp(&p2m->ept));
+
+    ept_sync_domain(p2m);
+}
+
 int vmx_create_vmcs(struct vcpu *v)
 {
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;

xen/arch/x86/mm/p2m-ept.c +20 -4

index 74ce9e0..86440fc 100644
@@ -1129,21 +1129,39 @@ void ept_sync_domain(struct p2m_domain *p2m)
 
 static void ept_enable_pml(struct p2m_domain *p2m)
 {
+    /* Domain must have been paused */
+    ASSERT(atomic_read(&p2m->domain->pause_count));
+
     /*
-     * No need to check if vmx_domain_enable_pml has succeeded or not, as
+     * No need to return whether vmx_domain_enable_pml has succeeded, as
      * ept_p2m_type_to_flags will do the check, and write protection will be
      * used if PML is not enabled.
      */
-    vmx_domain_enable_pml(p2m->domain);
+    if ( vmx_domain_enable_pml(p2m->domain) )
+        return;
+
+    /* Enable EPT A/D bit for PML */
+    p2m->ept.ept_ad = 1;
+    vmx_domain_update_eptp(p2m->domain);
 }
 
 static void ept_disable_pml(struct p2m_domain *p2m)
 {
+    /* Domain must have been paused */
+    ASSERT(atomic_read(&p2m->domain->pause_count));
+
     vmx_domain_disable_pml(p2m->domain);
+
+    /* Disable EPT A/D bit */
+    p2m->ept.ept_ad = 0;
+    vmx_domain_update_eptp(p2m->domain);
 }
 
 static void ept_flush_pml_buffers(struct p2m_domain *p2m)
 {
+    /* Domain must have been paused */
+    ASSERT(atomic_read(&p2m->domain->pause_count));
+
     vmx_domain_flush_pml_buffers(p2m->domain);
 }

@@ -1166,8 +1184,6 @@ int ept_p2m_init(struct p2m_domain *p2m)
 
     if ( cpu_has_vmx_pml )
     {
-        /* Enable EPT A/D bits if we are going to use PML. */
-        ept->ept_ad = cpu_has_vmx_pml ? 1 : 0;
         p2m->enable_hardware_log_dirty = ept_enable_pml;
         p2m->disable_hardware_log_dirty = ept_disable_pml;
         p2m->flush_hardware_cached_dirty = ept_flush_pml_buffers;

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

index f1126d4..ec526db 100644
@@ -518,6 +518,8 @@ 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);
 
+void vmx_domain_update_eptp(struct domain *d);
+
 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
 
 /*

Signature · 1 line

2.1.4

Jan Beulich

Reviewed-by on v1 1/2

2015-10-20 09:46 UTC #2 lore raw

Re: [PATCH 1/2] x86/ept: defer enabling of EPT A/D bit until PML get enabled.

Quoted 8 lines

Quoted 1 line
Quoted 1 line · On 20.10.15 at 04:34, <[email protected]> wrote:
Existing PML implementation turns on EPT A/D bit unconditionally if PML is supported by hardware. This works but enabling of EPT A/D bit can be deferred
until PML get enabled. There's no point in enabling the extra feature for every
domain when we're not meaning to use it (yet).

Quoted 7 lines

Also added ASSERT of domain having been paused to ept_flush_pml_buffers to make
it consistent with ept_enable{disable}_pml.
Sanity live migration and GUI display were tested on Broadwell Machine.
Signed-off-by Kai Huang <[email protected]>

Reviewed-by Jan Beulich <[email protected]>

Andrew Cooper

Reviewed-by on v1 1/2

2015-10-20 10:01 UTC #3 lore raw

Re: [PATCH 1/2] x86/ept: defer enabling of EPT A/D bit until PML get enabled.

In reply to Kai Huang

Quoted 5 lines · Kai Huang

Existing PML implementation turns on EPT A/D bit unconditionally if PML is supported by hardware. This works but enabling of EPT A/D bit can be deferred until PML get enabled. There's no point in enabling the extra feature for every domain when we're not meaning to use it (yet).

Quoted 7 lines

Also added ASSERT of domain having been paused to ept_flush_pml_buffers to make it consistent with ept_enable{disable}_pml.
Sanity live migration and GUI display were tested on Broadwell Machine.
Signed-off-by Kai Huang <[email protected]>
Suggested-by Jan Beulich <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>

Tian, Kevin

Acked-by on v1 1/2

2015-10-21 03:00 UTC #4 lore raw

Re: [PATCH 1/2] x86/ept: defer enabling of EPT A/D bit until PML get enabled.

In reply to Kai Huang

Quoted 8 lines

From: Kai Huang [mailto:[email protected]]
Sent: Tuesday, October 20, 2015 10:35 AM
Existing PML implementation turns on EPT A/D bit unconditionally if PML is supported by hardware. This works but enabling of EPT A/D bit can be deferred until PML get enabled. There's no point in enabling the extra feature for every domain when we're not meaning to use it (yet).

Quoted 7 lines

Also added ASSERT of domain having been paused to ept_flush_pml_buffers to make it consistent with ept_enable{disable}_pml.
Sanity live migration and GUI display were tested on Broadwell Machine.
Signed-off-by Kai Huang <[email protected]>
Suggested-by Jan Beulich <[email protected]>

Acked-by Kevin Tian <[email protected]>

Kai Huang

author

2015-10-20 02:34 UTC #5 lore raw

PATCH 2/2 x86/vmx: fix coding style of PML functions

In reply to Kai Huang

According to Jan's comments, also fix the coding style of for_each_vcpu in existing PML functions.

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

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

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

index c11f3ec..4ea1ad1 100644
@@ -1500,7 +1500,7 @@ int vmx_domain_enable_pml(struct domain *d)
     if ( vmx_domain_pml_enabled(d) )
         return 0;
 
-    for_each_vcpu( d, v )
+    for_each_vcpu ( d, v )
         if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
             goto error;

@@ -1509,7 +1509,7 @@ int vmx_domain_enable_pml(struct domain *d)
     return 0;
 
  error:
-    for_each_vcpu( d, v )
+    for_each_vcpu ( d, v )
         if ( vmx_vcpu_pml_enabled(v) )
             vmx_vcpu_disable_pml(v);
     return rc;

@@ -1530,7 +1530,7 @@ void vmx_domain_disable_pml(struct domain *d)
     if ( !vmx_domain_pml_enabled(d) )
         return;
 
-    for_each_vcpu( d, v )
+    for_each_vcpu ( d, v )
         vmx_vcpu_disable_pml(v);
 
     d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED;

@@ -1549,7 +1549,7 @@ void vmx_domain_flush_pml_buffers(struct domain *d)
     if ( !vmx_domain_pml_enabled(d) )
         return;
 
-    for_each_vcpu( d, v )
+    for_each_vcpu ( d, v )
         vmx_vcpu_flush_pml_buffer(v);
 }

Signature · 1 line

2.1.4

Andrew Cooper

Reviewed-by on v1 2/2

2015-10-20 10:01 UTC #6 lore raw

Re: [PATCH 2/2] x86/vmx: fix coding style of PML functions

Quoted 4 lines · Kai Huang

According to Jan's comments, also fix the coding style of for_each_vcpu in existing PML functions.
Signed-off-by Kai Huang <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>

Tian, Kevin

Acked-by on v1 2/2

2015-10-21 03:00 UTC #7 lore raw

Re: [PATCH 2/2] x86/vmx: fix coding style of PML functions

In reply to Kai Huang

Quoted 7 lines

From: Kai Huang [mailto:[email protected]]
Sent: Tuesday, October 20, 2015 10:35 AM
According to Jan's comments, also fix the coding style of for_each_vcpu in existing PML functions.
Signed-off-by Kai Huang <[email protected]>

Acked-by Kevin Tian <[email protected]>

Kai Huang

author

2015-10-20 10:44 UTC #8 lore raw

In reply to Kai Huang

Hi Kevin,

Would you comment on the two patches?

Thanks,
-Kai

Quoted 13 lines · Kai Huang

Patch 1 is the v2 of defering enabling of EPT A/D bit until PML get enabled, with comments from Jan in v1 addressed. Patch 2 is coding style fix of for_each_vcpu to existing PML functions according to Jan.
Kai Huang (2):
x86/ept: defer enabling of EPT A/D bit until PML get enabled. x86/vmx: fix coding style of PML functions
  xen/arch/x86/hvm/vmx/vmcs.c        | 32 ++++++++++++++++++++++++++++----
  xen/arch/x86/mm/p2m-ept.c          | 24 ++++++++++++++++++++----
  xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
  3 files changed, 50 insertions(+), 8 deletions(-)