[PATCH 0/2] Deter enabling of EPT A/D bit plus coding style fix

[PATCH 0/2] Deter enabling of EPT A/D bit plus coding style fix

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

-- 
2.1.4

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

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

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 <kai.huang@linux.intel.com>
Suggested-by: Jan Beulich <jbeulich@suse.com>
---
 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(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 3592a88..c11f3ec 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -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;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 74ce9e0..86440fc 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -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;
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index f1126d4..ec526db 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -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__ */
 
 /*
-- 
2.1.4

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

[Jan Beulich]

>>> On 20.10.15 at 04:34, <kai.huang@linux.intel.com> 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).
> 
> 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 <kai.huang@linux.intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Andrew Cooper]

On 20/10/15 03:34, Kai Huang 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).
>
> 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 <kai.huang@linux.intel.com>
> Suggested-by: Jan Beulich <jbeulich@suse.com>

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

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

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> 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).
> 
> 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 <kai.huang@linux.intel.com>
> Suggested-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

[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 <kai.huang@linux.intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index c11f3ec..4ea1ad1 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -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);
 }
 
-- 
2.1.4

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

[Andrew Cooper]

On 20/10/15 03:34, Kai Huang wrote:
> According to Jan's comments, also fix the coding style of for_each_vcpu in
> existing PML functions.
>
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>

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

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

[Tian, Kevin]

> From: Kai Huang [mailto:kai.huang@linux.intel.com]
> 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 <kai.huang@linux.intel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 0/2] Deter enabling of EPT A/D bit plus coding style fix

[Kai Huang]

Hi Kevin,

Would you comment on the two patches?

Thanks,
-Kai

On 10/20/2015 10:34 AM, Kai Huang wrote:
> 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(-)
>