From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH] x86/ept: defer enabling of EPT A/D bit until PML get enabled. Date: Fri, 16 Oct 2015 17:17:30 +0800 Message-ID: <5620C0AA.4050602@linux.intel.com> References: <1444962075-6713-1-git-send-email-kai.huang@linux.intel.com> <5620CEA302000078000ABB79@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5620CEA302000078000ABB79@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 Cc: george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, kevin.tian@intel.com, jun.nakajima@intel.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 10/16/2015 04:17 PM, Jan Beulich wrote: >>>> On 16.10.15 at 04:21, 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 >> Signed-off-by: Jan Beulich > 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 >