[PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

There's no point in enabling the extra feature for every domain when
we're not meaning to use it (yet). Just setting the flag should be
sufficient - the domain is required to be paused for PML enabling
anyway, i.e. hardware will pick up the new setting the next time
each vCPU of the guest gets scheduled.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Kai Huang <kai.huang@linux.intel.com>
---
VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
for your help here?

--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1127,6 +1127,7 @@ void ept_sync_domain(struct p2m_domain *
 
 static void ept_enable_pml(struct p2m_domain *p2m)
 {
+    p2m->ept.ept_ad = 1;
     /*
      * No need to check if vmx_domain_enable_pml has succeeded or not, as
      * ept_p2m_type_to_flags will do the check, and write protection will be
@@ -1137,6 +1138,7 @@ static void ept_enable_pml(struct p2m_do
 
 static void ept_disable_pml(struct p2m_domain *p2m)
 {
+    p2m->ept.ept_ad = 0;
     vmx_domain_disable_pml(p2m->domain);
 }
 
@@ -1164,8 +1166,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;




[-- Attachment #2: x86-PML-defer-AD-enable.patch --]
[-- Type: text/plain, Size: 1560 bytes --]

x86/EPT: defer enabling of A/D maintenance 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). Just setting the flag should be
sufficient - the domain is required to be paused for PML enabling
anyway, i.e. hardware will pick up the new setting the next time
each vCPU of the guest gets scheduled.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Kai Huang <kai.huang@linux.intel.com>
---
VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
for your help here?

--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1127,6 +1127,7 @@ void ept_sync_domain(struct p2m_domain *
 
 static void ept_enable_pml(struct p2m_domain *p2m)
 {
+    p2m->ept.ept_ad = 1;
     /*
      * No need to check if vmx_domain_enable_pml has succeeded or not, as
      * ept_p2m_type_to_flags will do the check, and write protection will be
@@ -1137,6 +1138,7 @@ static void ept_enable_pml(struct p2m_do
 
 static void ept_disable_pml(struct p2m_domain *p2m)
 {
+    p2m->ept.ept_ad = 0;
     vmx_domain_disable_pml(p2m->domain);
 }
 
@@ -1164,8 +1166,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;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[George Dunlap]

On Mon, Sep 28, 2015 at 3:42 PM, Jan Beulich <JBeulich@suse.com> wrote:
> There's no point in enabling the extra feature for every domain when
> we're not meaning to use it (yet). Just setting the flag should be
> sufficient - the domain is required to be paused for PML enabling
> anyway, i.e. hardware will pick up the new setting the next time
> each vCPU of the guest gets scheduled.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Kai Huang <kai.huang@linux.intel.com>

FWIW seems sensible to me.
 -George

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Andrew Cooper]

On 28/09/15 15:42, Jan Beulich wrote:
> There's no point in enabling the extra feature for every domain when
> we're not meaning to use it (yet). Just setting the flag should be
> sufficient - the domain is required to be paused for PML enabling
> anyway, i.e. hardware will pick up the new setting the next time
> each vCPU of the guest gets scheduled.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Kai Huang <kai.huang@linux.intel.com>

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

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

Good to me, if you have tested it. Sorry I cannot test it as I am
taking vacation  until Oct.8.

Thanks,
-Kai

On Mon, Sep 28, 2015 at 10:42 PM, Jan Beulich <JBeulich@suse.com> wrote:
> There's no point in enabling the extra feature for every domain when
> we're not meaning to use it (yet). Just setting the flag should be
> sufficient - the domain is required to be paused for PML enabling
> anyway, i.e. hardware will pick up the new setting the next time
> each vCPU of the guest gets scheduled.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Kai Huang <kai.huang@linux.intel.com>
> ---
> VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
> for your help here?
>
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1127,6 +1127,7 @@ void ept_sync_domain(struct p2m_domain *
>
>  static void ept_enable_pml(struct p2m_domain *p2m)
>  {
> +    p2m->ept.ept_ad = 1;
>      /*
>       * No need to check if vmx_domain_enable_pml has succeeded or not, as
>       * ept_p2m_type_to_flags will do the check, and write protection will be
> @@ -1137,6 +1138,7 @@ static void ept_enable_pml(struct p2m_do
>
>  static void ept_disable_pml(struct p2m_domain *p2m)
>  {
> +    p2m->ept.ept_ad = 0;
>      vmx_domain_disable_pml(p2m->domain);
>  }
>
> @@ -1164,8 +1166,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-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>



-- 
Thanks,
-Kai

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Jan Beulich]

>>> On 30.09.15 at 10:58, <kaih.linux@gmail.com> wrote:
> Good to me, if you have tested it. Sorry I cannot test it as I am
> taking vacation  until Oct.8.

Note how I asked for help with testing ...

> On Mon, Sep 28, 2015 at 10:42 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> There's no point in enabling the extra feature for every domain when
>> we're not meaning to use it (yet). Just setting the flag should be
>> sufficient - the domain is required to be paused for PML enabling
>> anyway, i.e. hardware will pick up the new setting the next time
>> each vCPU of the guest gets scheduled.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Kai Huang <kai.huang@linux.intel.com>
>> ---
>> VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
>> for your help here?

... here. This patch can certainly wait until you get back from
vacation.

Jan

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

On Wed, Sep 30, 2015 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.09.15 at 10:58, <kaih.linux@gmail.com> wrote:
>> Good to me, if you have tested it. Sorry I cannot test it as I am
>> taking vacation  until Oct.8.
>
> Note how I asked for help with testing ...
>
>> On Mon, Sep 28, 2015 at 10:42 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> There's no point in enabling the extra feature for every domain when
>>> we're not meaning to use it (yet). Just setting the flag should be
>>> sufficient - the domain is required to be paused for PML enabling
>>> anyway, i.e. hardware will pick up the new setting the next time
>>> each vCPU of the guest gets scheduled.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Cc: Kai Huang <kai.huang@linux.intel.com>
>>> ---
>>> VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
>>> for your help here?
>
> ... here. This patch can certainly wait until you get back from
> vacation.

Thanks. I'll test it or ask someone has machine to test it after I get back.

Thanks,
-Kai
>
> Jan
>



-- 
Thanks,
-Kai

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

Hi Jan,

Our QA tested this patch but this patch broke PML. Neither GUI display 
(video ram tracking also uses PML) nor live migration works. I'll 
investigate what's wrong and get back to you.

Thanks,
-Kai

On 09/30/2015 08:45 PM, Kai Huang wrote:
> On Wed, Sep 30, 2015 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 30.09.15 at 10:58, <kaih.linux@gmail.com> wrote:
>>> Good to me, if you have tested it. Sorry I cannot test it as I am
>>> taking vacation  until Oct.8.
>> Note how I asked for help with testing ...
>>
>>> On Mon, Sep 28, 2015 at 10:42 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> There's no point in enabling the extra feature for every domain when
>>>> we're not meaning to use it (yet). Just setting the flag should be
>>>> sufficient - the domain is required to be paused for PML enabling
>>>> anyway, i.e. hardware will pick up the new setting the next time
>>>> each vCPU of the guest gets scheduled.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Kai Huang <kai.huang@linux.intel.com>
>>>> ---
>>>> VT-x maintainers, Kai: Me lacking the hardware to test this, may I ask
>>>> for your help here?
>> ... here. This patch can certainly wait until you get back from
>> vacation.
> Thanks. I'll test it or ask someone has machine to test it after I get back.
>
> Thanks,
> -Kai
>> Jan
>>
>
>

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

Hi Jan,

After some thinking, just set/clear p2m->ept.ept_ad is not enough -- we 
also need to __vmwrite it to VMCS's EPTP, and then call ept_sync_domain. 
I have verified attached patch can work.

Which implementation would you prefer, existing code or with attached 
patch? If you prefer the latter, please provide comments.

Thanks,
-Kai

On 10/14/2015 09:19 AM, Kai Huang wrote:
> Hi Jan,
>
> Our QA tested this patch but this patch broke PML. Neither GUI display 
> (video ram tracking also uses PML) nor live migration works. I'll 
> investigate what's wrong and get back to you.
>
> Thanks,
> -Kai
>
> On 09/30/2015 08:45 PM, Kai Huang wrote:
>> On Wed, Sep 30, 2015 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 30.09.15 at 10:58, <kaih.linux@gmail.com> wrote:
>>>> Good to me, if you have tested it. Sorry I cannot test it as I am
>>>> taking vacation  until Oct.8.
>>> Note how I asked for help with testing ...
>>>
>>>> On Mon, Sep 28, 2015 at 10:42 PM, Jan Beulich <JBeulich@suse.com> 
>>>> wrote:
>>>>> There's no point in enabling the extra feature for every domain when
>>>>> we're not meaning to use it (yet). Just setting the flag should be
>>>>> sufficient - the domain is required to be paused for PML enabling
>>>>> anyway, i.e. hardware will pick up the new setting the next time
>>>>> each vCPU of the guest gets scheduled.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Cc: Kai Huang <kai.huang@linux.intel.com>
>>>>> ---
>>>>> VT-x maintainers, Kai: Me lacking the hardware to test this, may I 
>>>>> ask
>>>>> for your help here?
>>> ... here. This patch can certainly wait until you get back from
>>> vacation.
>> Thanks. I'll test it or ask someone has machine to test it after I 
>> get back.
>>
>> Thanks,
>> -Kai
>>> Jan
>>>
>>
>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>


[-- Attachment #2: 0001-x86-ept-defer-enabling-EPT-A-D-bit-until-PML-is-enab.patch --]
[-- Type: text/x-patch, Size: 3468 bytes --]

>From cd01ef0908ee6d0931ea15ff25606f76fe859757 Mon Sep 17 00:00:00 2001
From: Kai Huang <kai.huang@linux.intel.com>
Date: Wed, 14 Oct 2015 17:01:24 +0800
Subject: [PATCH] x86/ept: defer enabling EPT A/D bit until PML is enabled.

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

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 3592a88..9bb278b 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1382,6 +1382,8 @@ bool_t vmx_vcpu_pml_enabled(const struct vcpu *v)
 
 int vmx_vcpu_enable_pml(struct vcpu *v)
 {
+    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+
     if ( vmx_vcpu_pml_enabled(v) )
         return 0;
 
@@ -1399,6 +1401,9 @@ int vmx_vcpu_enable_pml(struct vcpu *v)
     __vmwrite(SECONDARY_VM_EXEC_CONTROL,
               v->arch.hvm_vmx.secondary_exec_control);
 
+    /* we leave ept_sync_domain to vmx_domain_enable_pml */
+    __vmwrite(EPT_POINTER, ept_get_eptp(&p2m->ept));
+
     vmx_vmcs_exit(v);
 
     return 0;
@@ -1406,6 +1411,8 @@ int vmx_vcpu_enable_pml(struct vcpu *v)
 
 void vmx_vcpu_disable_pml(struct vcpu *v)
 {
+    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+
     if ( !vmx_vcpu_pml_enabled(v) )
         return;
 
@@ -1418,6 +1425,9 @@ void vmx_vcpu_disable_pml(struct vcpu *v)
     __vmwrite(SECONDARY_VM_EXEC_CONTROL,
               v->arch.hvm_vmx.secondary_exec_control);
 
+    /* we leave ept_sync_domain to vmx_domain_enable_pml */
+    __vmwrite(EPT_POINTER, ept_get_eptp(&p2m->ept));
+
     vmx_vmcs_exit(v);
 
     v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg);
@@ -1492,6 +1502,7 @@ bool_t vmx_domain_pml_enabled(const struct domain *d)
  */
 int vmx_domain_enable_pml(struct domain *d)
 {
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
     struct vcpu *v;
     int rc;
 
@@ -1500,10 +1511,14 @@ int vmx_domain_enable_pml(struct domain *d)
     if ( vmx_domain_pml_enabled(d) )
         return 0;
 
+    p2m->ept.ept_ad = 1;
+
     for_each_vcpu( d, v )
         if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
             goto error;
 
+    ept_sync_domain(p2m);
+
     d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
 
     return 0;
@@ -1523,6 +1538,7 @@ int vmx_domain_enable_pml(struct domain *d)
  */
 void vmx_domain_disable_pml(struct domain *d)
 {
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
     struct vcpu *v;
 
     ASSERT(atomic_read(&d->pause_count));
@@ -1530,10 +1546,14 @@ void vmx_domain_disable_pml(struct domain *d)
     if ( !vmx_domain_pml_enabled(d) )
         return;
 
+    p2m->ept.ept_ad = 0;
+
     for_each_vcpu( d, v )
         vmx_vcpu_disable_pml(v);
 
     d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED;
+
+    ept_sync_domain(p2m);
 }
 
 /*
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 74ce9e0..0d689b0 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1166,8 +1166,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;
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Jan Beulich]

>>> On 14.10.15 at 11:08, <kai.huang@linux.intel.com> wrote:
> After some thinking, just set/clear p2m->ept.ept_ad is not enough -- we 
> also need to __vmwrite it to VMCS's EPTP, and then call ept_sync_domain. 

Ah, yes, this makes sense of course.

> I have verified attached patch can work.

Thanks!

> Which implementation would you prefer, existing code or with attached 
> patch? If you prefer the latter, please provide comments.

I think it's marginal whether to flip the bit in ept_{en,dis}able_pml()
or vmx_domain_{en,dis}able_pml(); the former would seem slightly
more logical.

There's one possible problem with the patch though: Deferring the
sync from the vcpu to the domain function is fine when the domain
function is the caller, but what about the calls out of vmx.c? The
calls look safe as the domain isn't running (yet or anymore) at that
point, but the respective comments may need adjustment (and
the disable one should also refer to vmx_domain_disable_pml()),
in order to avoid confusing future readers. Also you'd need to fix
coding style of these new comments.

Jan

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

On 10/14/2015 05:26 PM, Jan Beulich wrote:
>>>> On 14.10.15 at 11:08, <kai.huang@linux.intel.com> wrote:
>> After some thinking, just set/clear p2m->ept.ept_ad is not enough -- we
>> also need to __vmwrite it to VMCS's EPTP, and then call ept_sync_domain.
> Ah, yes, this makes sense of course.
>
>> I have verified attached patch can work.
> Thanks!
>
>> Which implementation would you prefer, existing code or with attached
>> patch? If you prefer the latter, please provide comments.
> I think it's marginal whether to flip the bit in ept_{en,dis}able_pml()
> or vmx_domain_{en,dis}able_pml(); the former would seem slightly
> more logical.
>
> There's one possible problem with the patch though: Deferring the
> sync from the vcpu to the domain function is fine when the domain
> function is the caller, but what about the calls out of vmx.c? The
> calls look safe as the domain isn't running (yet or anymore) at that
> point, but the respective comments may need adjustment (and
> the disable one should also refer to vmx_domain_disable_pml()),
> in order to avoid confusing future readers. Also you'd need to fix
> coding style of these new comments.
Thanks for your comments Jan. Actually I am not happy with combining 
with EPT A/D bit update with PML enabling to single function. After 
thinking again, how about adding a separate vmx function (ex, 
vmx_domain_update_eptp) to update EPTP of VMCS of all vcpus of domain 
after p2m->ept.ept_ad is updated. Another good is this function can also 
be used in the future for other runtime updates to p2m->ept.

What's your idea?

Below is the temporary code verified to be able to work. If you are OK 
with this approach (and comments are welcome), I will send out the 
formal patch.

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 3592a88..cddab15 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..cbba06a 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1129,17 +1129,26 @@ void ept_sync_domain(struct p2m_domain *p2m)

  static void ept_enable_pml(struct p2m_domain *p2m)
  {
      /*
-     * No need to check if vmx_domain_enable_pml has succeeded or not, as
+     * No need to return if vmx_domain_enable_pml has succeeded or not, as
       * ept_p2m_type_to_flags will do the check, and write protection 
will be
       * used if PML is not enabled.
       */
-    vmx_domain_enable_pml(p2m->domain);
+    if ( vmx_domain_enable_pml(p2m->domain) )
+        return;
+
+    p2m->ept.ept_ad = 1;
+    vmx_domain_update_eptp(p2m->domain);
  }

  static void ept_disable_pml(struct p2m_domain *p2m)
  {
      vmx_domain_disable_pml(p2m->domain);
+
+    p2m->ept.ept_ad = 0;
+    vmx_domain_update_eptp(p2m->domain);
  }

  static void ept_flush_pml_buffers(struct p2m_domain *p2m)
@@ -1166,8 +1177,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__ */


Thanks,
-Kai

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

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Jan Beulich]

>>> On 15.10.15 at 08:42, <kai.huang@linux.intel.com> wrote:
> Thanks for your comments Jan. Actually I am not happy with combining 
> with EPT A/D bit update with PML enabling to single function. After 
> thinking again, how about adding a separate vmx function (ex, 
> vmx_domain_update_eptp) to update EPTP of VMCS of all vcpus of domain 
> after p2m->ept.ept_ad is updated. Another good is this function can also 
> be used in the future for other runtime updates to p2m->ept.
> 
> What's your idea?

I don't mind, but that's really more of a question to the VMX maintainers.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1129,17 +1129,26 @@ void ept_sync_domain(struct p2m_domain *p2m)
> 
>   static void ept_enable_pml(struct p2m_domain *p2m)
>   {
>       /*
> -     * No need to check if vmx_domain_enable_pml has succeeded or not, as
> +     * No need to return if vmx_domain_enable_pml has succeeded or not, as

It seems to me that you'd better use "whether" instead of "if" now
(and then perhaps also drop the "or not").

>        * 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;
> +
> +    p2m->ept.ept_ad = 1;
> +    vmx_domain_update_eptp(p2m->domain);

Shouldn't you enable A/D _before_ enabling PML, at least without
having a domain-is-paused check here?

Jan

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

On 10/15/2015 03:11 PM, Jan Beulich wrote:
>>>> On 15.10.15 at 08:42, <kai.huang@linux.intel.com> wrote:
>> Thanks for your comments Jan. Actually I am not happy with combining
>> with EPT A/D bit update with PML enabling to single function. After
>> thinking again, how about adding a separate vmx function (ex,
>> vmx_domain_update_eptp) to update EPTP of VMCS of all vcpus of domain
>> after p2m->ept.ept_ad is updated. Another good is this function can also
>> be used in the future for other runtime updates to p2m->ept.
>>
>> What's your idea?
> I don't mind, but that's really more of a question to the VMX maintainers.
Then I would prefer this way.

Kevin,

Do you have any comments on this thread?
>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1129,17 +1129,26 @@ void ept_sync_domain(struct p2m_domain *p2m)
>>
>>    static void ept_enable_pml(struct p2m_domain *p2m)
>>    {
>>        /*
>> -     * No need to check if vmx_domain_enable_pml has succeeded or not, as
>> +     * No need to return if vmx_domain_enable_pml has succeeded or not, as
> It seems to me that you'd better use "whether" instead of "if" now
> (and then perhaps also drop the "or not").
OK. Thanks.
>
>>         * 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;
>> +
>> +    p2m->ept.ept_ad = 1;
>> +    vmx_domain_update_eptp(p2m->domain);
> Shouldn't you enable A/D _before_ enabling PML, at least without
> having a domain-is-paused check here?
Looks we don't have such function. How about just add 
ASSERT(atomic_read(&d->pause_count)), just the same as in 
vmx_domain_enable_pml ?

Thanks,
-Kai
>
> Jan
>
>

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang]

On 10/15/2015 03:35 PM, Kai Huang wrote:
>
>
> On 10/15/2015 03:11 PM, Jan Beulich wrote:
>>>>> On 15.10.15 at 08:42, <kai.huang@linux.intel.com> wrote:
>>> Thanks for your comments Jan. Actually I am not happy with combining
>>> with EPT A/D bit update with PML enabling to single function. After
>>> thinking again, how about adding a separate vmx function (ex,
>>> vmx_domain_update_eptp) to update EPTP of VMCS of all vcpus of domain
>>> after p2m->ept.ept_ad is updated. Another good is this function can 
>>> also
>>> be used in the future for other runtime updates to p2m->ept.
>>>
>>> What's your idea?
>> I don't mind, but that's really more of a question to the VMX 
>> maintainers.
> Then I would prefer this way.
>
> Kevin,
>
> Do you have any comments on this thread?
>>
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -1129,17 +1129,26 @@ void ept_sync_domain(struct p2m_domain *p2m)
>>>
>>>    static void ept_enable_pml(struct p2m_domain *p2m)
>>>    {
>>>        /*
>>> -     * No need to check if vmx_domain_enable_pml has succeeded or 
>>> not, as
>>> +     * No need to return if vmx_domain_enable_pml has succeeded or 
>>> not, as
>> It seems to me that you'd better use "whether" instead of "if" now
>> (and then perhaps also drop the "or not").
> OK. Thanks.
>>
>>>         * 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;
>>> +
>>> +    p2m->ept.ept_ad = 1;
>>> +    vmx_domain_update_eptp(p2m->domain);
>> Shouldn't you enable A/D _before_ enabling PML, at least without
>> having a domain-is-paused check here?
> Looks we don't have such function. How about just add 
> ASSERT(atomic_read(&d->pause_count)), just the same as in 
> vmx_domain_enable_pml ?
I mean we can enable A/D before enabling PML, but if so we need 
additional code to clear A/D bit if vmx_domain_enable_pml failed. My 
thinking is considering  the function is called when domain is paused, 
so there's no difference to enable A/D before or after enabling PML.

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

Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Jan Beulich]

>>> On 15.10.15 at 09:35, <kai.huang@linux.intel.com> wrote:
> On 10/15/2015 03:11 PM, Jan Beulich wrote:
>>>>> On 15.10.15 at 08:42, <kai.huang@linux.intel.com> wrote:
>>>         * 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;
>>> +
>>> +    p2m->ept.ept_ad = 1;
>>> +    vmx_domain_update_eptp(p2m->domain);
>> Shouldn't you enable A/D _before_ enabling PML, at least without
>> having a domain-is-paused check here?
> Looks we don't have such function. How about just add 
> ASSERT(atomic_read(&d->pause_count)), just the same as in 
> vmx_domain_enable_pml ?

Indeed that was one of the two possible solutions I meant to hint at.

Jan