From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Han, Huaitong" Subject: Re: [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables Date: Wed, 16 Dec 2015 08:16:54 +0000 Message-ID: <1450253821.4539.3.camel@intel.com> References: <1449479780-19146-1-git-send-email-huaitong.han@intel.com> <1449479780-19146-8-git-send-email-huaitong.han@intel.com> <5669C23F.6080203@citrix.com> <566AA4D702000078000BE866@prv-mh.provo.novell.com> <1450167259.10563.9.camel@intel.com> <566FE51902000078000BF67A@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <566FE51902000078000BF67A@prv-mh.provo.novell.com> Content-Language: en-US Content-ID: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "JBeulich@suse.com" Cc: "Tian, Kevin" , "wei.liu2@citrix.com" , "ian.campbell@citrix.com" , "stefano.stabellini@eu.citrix.com" , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "ian.jackson@eu.citrix.com" , "george.dunlap@citrix.com" , "xen-devel@lists.xen.org" , "Nakajima, Jun" , "keir@xen.org" List-Id: xen-devel@lists.xenproject.org On Tue, 2015-12-15 at 02:02 -0700, Jan Beulich wrote: > > > > On 15.12.15 at 09:14, wrote: > > On Fri, 2015-12-11 at 02:26 -0700, Jan Beulich wrote: > > > > > > On 10.12.15 at 19:19, wrote: > > > > On 07/12/15 09:16, Huaitong Han wrote: > > > > > + if ( likely(!pte_pkeys) ) > > > > > + return 0; > > > > > + > > > > > + /* Update vcpu xsave area */ > > > > > + fpu_xsave(vcpu); > > > > > > > > Is there a reason you're calling fpu_xsave() directly here, > > > > rather > > > > than > > > > just calling vcpu_save_fpu()? That saves you actually doing > > > > the > > > > xsave > > > > if the fpu hasn't been modified since the last time you read > > > > it. > > > > > > I've already said on an earlier version that wholesale saving of > > > the > > > entire XSAVE state is wrong here. It should just be the single > > > piece > > > that we're actually interested in, and it quite likely shouldn't > > > go > > > into > > > struct vcpu (but e.g. into a local buffer). > > > > The comments on V2 version said using vcpu_save_fpu is wrong > > because of > > v->fpu_dirtied, but why wholesale saving of the entire XSAVE state > > is > > wrong here? I understand xsave maybe cost a little more. > > "A little" is quite a bit of an understatement. > > > But if we just > > save the single piece, many functions are not reused because > > xstate_comp_offsets is pointless, and we need add a new function as > > follow that looks not good: > > Well, I wouldn't want you to introduce a brand new function, but > instead just factor out the necessary piece from xsave() (making > the new one take a struct xsave_struct * instead of a struct vcpu *, > and calling it from what is now xsave()). So the function looks like this: unsigned int get_xsave_pkru(struct vcpu *v) { void *offset; struct xsave_struct *xsave_area; uint64_t mask = XSTATE_PKRU; unsigned int index = fls64(mask) - 1; unsigned int pkru = 0; if ( !cpu_has_xsave ) return 0; BUG_ON(xsave_cntxt_size < XSTATE_AREA_MIN_SIZE); xsave_area = _xzalloc(xsave_cntxt_size, 64); if ( xsave_area == NULL ) return 0; xsave(xsave_area, mask); offset = (void *)xsave_area + (xsave_area_compressed(xsave) ? XSTATE_AREA_MIN_SIZE : xstate_offsets[index] ); memcpy(&pkru, offset, sizeof(pkru)); xfree(xsave_area); return pkru; } > > Jan >