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

[Kai Huang <kai.huang@xxxxxxxxxxxxxxx>]

On 10/14/2015 05:26 PM, Jan Beulich wrote:
> > > > On 14.10.15 at 11:08, <kai.huang@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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