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

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

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

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
Signed-off-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..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..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] x86/ept: defer enabling of EPT A/D bit until PML get enabled.

[Jan Beulich]

>>> On 16.10.15 at 04:21, <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).
> 
> Sanity live migration and GUI display were tested on Broadwell Machine.
> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

There's so little in this patch that came from me that I don't think this is
warranted; but if you want to keep it, the order needs to be switched.
Instead I'd suggest Suggested-by:.

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

This should imo check controller_pause_count.

> +    for_each_vcpu( d, v )

Coding style: You need to settle on whether you want to treat
for_each_vcpu like a keyword (then there's a blank missing before
the opening paren) or like a normal identifier (then the blanks
immediately inside the parens need to go away).

>  static void ept_flush_pml_buffers(struct p2m_domain *p2m)
>  {
> +    /* Domain must have been paused */
> +    ASSERT(atomic_read(&p2m->domain->pause_count));

This seems unrelated - did you really mean it to go into this patch?

Jan

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

[Kai Huang]

On 10/16/2015 04:17 PM, Jan Beulich wrote:
>>>> On 16.10.15 at 04:21, <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).
>>
>> Sanity live migration and GUI display were tested on Broadwell Machine.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> There's so little in this patch that came from me that I don't think this is
> warranted; but if you want to keep it, the order needs to be switched.
> Instead I'd suggest Suggested-by:.
I'll change it to Suggested-by.

>
>> +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));
> This should imo check controller_pause_count.
This function is called between domain_pause and domain_unpause, and 
domain_pause increases d->pause_count, not d->controller_pause_count, so 
we should check d->pause_count, right?

>
>> +    for_each_vcpu( d, v )
> Coding style: You need to settle on whether you want to treat
> for_each_vcpu like a keyword (then there's a blank missing before
> the opening paren) or like a normal identifier (then the blanks
> immediately inside the parens need to go away).
Oh. I will add a blank before the opening paren.

>
>>   static void ept_flush_pml_buffers(struct p2m_domain *p2m)
>>   {
>> +    /* Domain must have been paused */
>> +    ASSERT(atomic_read(&p2m->domain->pause_count));
> This seems unrelated - did you really mean it to go into this patch?
This function is also supposed to be called when domain is paused, so 
making it consistent with ept_enable{disable}_pml, I also added the 
ASSERT here. Is this reasonable?

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 EPT A/D bit until PML get enabled.

[Jan Beulich]

>>> On 16.10.15 at 11:17, <kai.huang@linux.intel.com> wrote:
> On 10/16/2015 04:17 PM, Jan Beulich wrote:
>>>>> On 16.10.15 at 04:21, <kai.huang@linux.intel.com> wrote:
>>> +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));
>> This should imo check controller_pause_count.
> This function is called between domain_pause and domain_unpause, and 
> domain_pause increases d->pause_count, not d->controller_pause_count, so 
> we should check d->pause_count, right?

Ah, okay - I thought the pause was tool stack initiated.

>>>   static void ept_flush_pml_buffers(struct p2m_domain *p2m)
>>>   {
>>> +    /* Domain must have been paused */
>>> +    ASSERT(atomic_read(&p2m->domain->pause_count));
>> This seems unrelated - did you really mean it to go into this patch?
> This function is also supposed to be called when domain is paused, so 
> making it consistent with ept_enable{disable}_pml, I also added the 
> ASSERT here. Is this reasonable?

Then you need to explicitly mention that in the commit message.

Jan

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

[Andrew Cooper]

On 16/10/15 10:27, Jan Beulich wrote:
>>>> On 16.10.15 at 11:17, <kai.huang@linux.intel.com> wrote:
>> On 10/16/2015 04:17 PM, Jan Beulich wrote:
>>>>>> On 16.10.15 at 04:21, <kai.huang@linux.intel.com> wrote:
>>>> +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));
>>> This should imo check controller_pause_count.
>> This function is called between domain_pause and domain_unpause, and 
>> domain_pause increases d->pause_count, not d->controller_pause_count, so 
>> we should check d->pause_count, right?
> Ah, okay - I thought the pause was tool stack initiated.

ept_enable_pml() is the subject of p2m->enable_hardware_log_dirty(),
which happens during runtime with logdirty ops, etc.

~Andrew