From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled Date: Thu, 15 Oct 2015 14:42:45 +0800 Message-ID: <561F4AE5.2080102@linux.intel.com> References: <56096DE002000078000A63EE@prv-mh.provo.novell.com> <560BCD6B02000078000A6FF0@prv-mh.provo.novell.com> <561DADA1.3050303@linux.intel.com> <561E1B7B.3000900@linux.intel.com> <561E3C0102000078000AACEB@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZmcIv-0007dh-Lh for xen-devel@lists.xenproject.org; Thu, 15 Oct 2015 06:46:21 +0000 In-Reply-To: <561E3C0102000078000AACEB@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Kai Huang Cc: Andrew Cooper , Kevin Tian , Jun Nakajima , xen-devel List-Id: xen-devel@lists.xenproject.org On 10/14/2015 05:26 PM, Jan Beulich wrote: >>>> On 14.10.15 at 11:08, 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 >