From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types Date: Fri, 22 Nov 2013 12:40:08 -0800 Message-ID: <20131122124008.27129bdf@mantra.us.oracle.com> References: <20131121170051.424971fd@mantra.us.oracle.com> <528F3765.8080806@eu.citrix.com> <528F567F0200007800105CB0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <528F567F0200007800105CB0@nat28.tlf.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: "Xen-devel@lists.xensource.com" , George Dunlap , eddie.dong@intel.com, Keir Fraser , Jun Nakajima , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org On Fri, 22 Nov 2013 12:05:03 +0000 "Jan Beulich" wrote: > >>> On 22.11.13 at 11:52, George Dunlap > >>> wrote: > > On 22/11/13 01:00, Mukesh Rathor wrote: > >> For pvh guests, epte_get_entry_emt() is incorrectly returning WB > >> for all mem types because of the following check: > >> if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ) > >> Skip the check for pvh guests. > >> > >> Also note, MTRR ranges are not maintained for pvh, and a solution > >> is being contrived using PAT. > >> > >> Signed-off-by: Mukesh Rathor > >> --- > >> xen/arch/x86/hvm/mtrr.c | 7 ++++++- > >> 1 files changed, 6 insertions(+), 1 deletions(-) > >> > >> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c > >> index 4ff1e55..5427e1c 100644 > >> --- a/xen/arch/x86/hvm/mtrr.c > >> +++ b/xen/arch/x86/hvm/mtrr.c > >> @@ -693,7 +693,8 @@ uint8_t epte_get_entry_emt(struct domain *d, > >> unsigned > > long gfn, mfn_t mfn, > >> ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) ) > >> return MTRR_TYPE_WRBACK; > >> > >> - if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ) > >> + if ( !is_pvh_vcpu(v) && > >> + !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ) > >> return MTRR_TYPE_WRBACK; > >> > >> if ( !mfn_valid(mfn_x(mfn)) ) > >> @@ -717,6 +718,10 @@ uint8_t epte_get_entry_emt(struct domain *d, > >> unsigned > > long gfn, mfn_t mfn, > >> return MTRR_TYPE_WRBACK; > >> } > >> > >> + /* MTRR ranges are not maintained for pvh. */ > >> + if ( is_pvh_vcpu(v) ) > >> + return MTRR_TYPE_WRBACK; > >> + > >> gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << > >> PAGE_SHIFT)); hmtrr_mtype = get_mtrr_type(&mtrr_state, (mfn_x(mfn) > >> << PAGE_SHIFT)); return ((gmtrr_mtype <= hmtrr_mtype) ? > >> gmtrr_mtype : hmtrr_mtype); > > > > So this will bypass the host mtrr settings, and always return > > WRBACK, even if in mtrr_state it was set to something lower. > > Presumably that "min(host,guest)" was there for a reason. Are you > > sure it's safe to just ignore it? > > > > This is why I suggested the following instead: > > > > gmtrr_mtype = is_hvm_domain(v) ? > > get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << > > PAGE_SHIFT)) : MTRR_TYPE_WRBACK; > > Indeed, that's what we should use. Sorry, totally my bad. For some reason I had concluded that hmtrr_mtype was being set to UC. V2 patch on the way... thanks Mukesh