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 15:35:03 +0800 Message-ID: <561F5727.5060802@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> <561F4AE5.2080102@linux.intel.com> <561F6DCB02000078000AB338@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.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Zmd7Z-0003Dt-J7 for xen-devel@lists.xenproject.org; Thu, 15 Oct 2015 07:38:41 +0000 In-Reply-To: <561F6DCB02000078000AB338@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/15/2015 03:11 PM, Jan Beulich wrote: >>>> On 15.10.15 at 08:42, 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 > >