Skip to contentSkip to conversation

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

PATCH In review Kai Huang 3 participants 5 messages Oct 16, 2015

lore
Share & tools
lore

Kai Huang

author
#0 lore raw

PATCH 1/1 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).

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

Signed-off-by Kai Huang <[email protected]>
Signed-off-by Jan Beulich <[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..cddab15 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
#1 lore raw
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 04:21, <[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 4 lines

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

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

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:.

xen/arch/x86/hvm/vmx/vmcs.c void vmx_domain_flush_pml_buffers(struct domain *d) · line 1572

Quoted 6 lines
xen/arch/x86/hvm/vmx/vmcs.c +6 -0
+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.

Quoted 1 line

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

Review

Quoted 4 lines

static void ept_flush_pml_buffers(struct p2m_domain *p2m) {

(quoted diff) +2 -0
+    /* 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

author
#2 lore raw
Quoted 8 lines · Jan Beulich
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 04:21, <[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
Quoted 4 lines

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

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

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.

Quoted 8 lines

xen/arch/x86/hvm/vmx/vmcs.c void vmx_domain_flush_pml_buffers(struct domain *d) · line 1572

Quoted 7 lines
xen/arch/x86/hvm/vmx/vmcs.c +6 -0
+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?

Quoted 6 lines
Quoted 2 lines

+ 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.

Quoted 6 lines

Review

Quoted 5 lines
  static void ept_flush_pml_buffers(struct p2m_domain *p2m)
  {
(quoted diff) +2 -0
+    /* 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

Quoted 9 lines

Jan

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

#3 lore raw
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 11:17, <[email protected]> wrote:
Quoted 8 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_domain_flush_pml_buffers(struct domain *d) · line 1572

Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 04:21, <[email protected]> wrote:
xen/arch/x86/hvm/vmx/vmcs.c +6 -0
+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.

Quoted 3 lines

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.

Quoted 8 lines
Quoted 5 lines

Review

Quoted 4 lines
  static void ept_flush_pml_buffers(struct p2m_domain *p2m)
  {
(quoted diff) +2 -0
+    /* 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

#4 lore raw
Quoted 10 lines · Jan Beulich
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 11:17, <[email protected]> wrote:
Quoted 8 lines · Jan Beulich

xen/arch/x86/hvm/vmx/vmcs.c void vmx_domain_flush_pml_buffers(struct domain *d) · line 1572

Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 16.10.15 at 04:21, <[email protected]> wrote:
xen/arch/x86/hvm/vmx/vmcs.c +6 -0
+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.

Quoted 4 lines
Quoted 3 lines

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