From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Han, Huaitong" Subject: Re: [PATCH V4 4/6] x86/hvm: pkeys, add pkeys support for guest_walk_tables Date: Tue, 22 Dec 2015 08:12:05 +0000 Message-ID: <1450771932.4024.18.camel@intel.com> References: <1450682504-32286-1-git-send-email-huaitong.han@intel.com> <1450682504-32286-5-git-send-email-huaitong.han@intel.com> <567829B402000078000C1FCD@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: <567829B402000078000C1FCD@prv-mh.provo.novell.com> Content-Language: en-US Content-ID: <587968331A1B3447A96C5225AF08FCBE@intel.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: "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" , "xen-devel@lists.xen.org" , "Nakajima, Jun" , "keir@xen.org" List-Id: xen-devel@lists.xenproject.org On Mon, 2015-12-21 at 08:32 -0700, Jan Beulich wrote: > > > > On 21.12.15 at 08:21, wrote: > > --- a/xen/arch/x86/mm/guest_walk.c > > +++ b/xen/arch/x86/mm/guest_walk.c > > @@ -90,6 +90,55 @@ static uint32_t set_ad_bits(void *guest_p, void > > *walk_p, int set_dirty) > > return 0; > > } > > > > +#if GUEST_PAGING_LEVELS >= CONFIG_PAGING_LEVELS > > GUEST_PAGING_LEVELS >= 4 (just like further down) The code is modified according Andrew's comments: " This is a latent linking bug for the future when 5 levels comes along. It will probably be best to use the same trick as gw_page_flags to compile it once but use it multiple times. " > > > +bool_t leaf_pte_pkeys_check(struct vcpu *vcpu, > > + uint32_t pfec, uint32_t pte_pkey) > > +{ > > + unsigned int pkru = 0; > > + bool_t pkru_ad, pkru_wd; > > + > > Stray blank line. > > > + bool_t pf = !!(pfec & PFEC_page_present); > > + bool_t uf = !!(pfec & PFEC_user_mode); > > + bool_t wf = !!(pfec & PFEC_write_access); > > + bool_t ff = !!(pfec & PFEC_insn_fetch); > > + bool_t rsvdf = !!(pfec & PFEC_reserved_bit); > > + > > + /* When page is present, PFEC_prot_key is always checked */ > > + if ( !pf || is_pv_vcpu(vcpu) ) > > + return 0; > > I think for a function called "check" together with how its callers > use > it the return value meaning is inverted here. I will change the function name to "pkey_page_fault". > Also the comment seems > inverted wrt the actual check (and is missing a full stop). And > doesn't > key 0 have static meaning, in which case you could bail early (and > namely avoid the expensive RDPKRU further down)? Key 0 have no static meaning, the default key maybe different in different OS. > > > + /* > > + * PKU: additional mechanism by which the paging controls > > + * access to user-mode addresses based on the value in the > > + * PKRU register. A fault is considered as a PKU violation if > > all > > + * of the following conditions are ture: > > + * 1.CR4_PKE=1. > > + * 2.EFER_LMA=1. > > + * 3.page is present with no reserved bit violations. > > + * 4.the access is not an instruction fetch. > > + * 5.the access is to a user page. > > + * 6.PKRU.AD=1 > > + * or The access is a data write and PKRU.WD=1 > > + * and either CR0.WP=1 or it is a user access. > > + */ > > + if ( !hvm_pku_enabled(vcpu) || > > + !hvm_long_mode_enabled(vcpu) || rsvdf || ff ) > > Where's the "user page" check? Also - indentation. it should be: if ( !hvm_pku_enabled(vcpu) || !hvm_long_mode_enabled(vcpu) || rsvdf || ff || !(pte_flags & _PAGE_USER) ) > > > + return 0; > > + > > + pkru = read_pkru(); > > + if ( unlikely(pkru) ) > > + { > > + pkru_ad = read_pkru_ad(pkru, pte_pkey); > > + pkru_wd = read_pkru_wd(pkru, pte_pkey); > > + /* Condition 6 */ > > + if ( pkru_ad || (pkru_wd && wf && (hvm_wp_enabled(vcpu) || > > uf))) > > Ah, uf is being checked here. But according to the comment it could > (and should, again to avoid the RDPKRU) move up. > > > @@ -270,6 +324,12 @@ guest_walk_tables(struct vcpu *v, struct > > p2m_domain *p2m, > > > > pse2M = (gflags & _PAGE_PSE) && guest_supports_superpages(v); > > > > +#if GUEST_PAGING_LEVELS >= 4 > > + pkey = guest_l2e_get_pkey(gw->l2e); > > + if ( pse2M && leaf_pte_pkeys_check(v, pfec, pkey) ) > > + rc |= _PAGE_PKEY_BITS; > > +#endif > > I think the #ifdef isn't really needed here, if you moved the one > around leaf_pte_pkeys_check() into that function, and if you > perhaps also dropped the "pkey" local variable. guest_l2e_get_pkey has different macro depend on GUEST_PAGING_LEVELS too, and I think it's better to keep it. > > Jan >