From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V4 4/6] x86/hvm: pkeys, add pkeys support for guest_walk_tables Date: Tue, 22 Dec 2015 01:43:58 -0700 Message-ID: <56791B5E02000078000C220F@prv-mh.provo.novell.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> <1450771932.4024.18.camel@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1450771932.4024.18.camel@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Huaitong Han Cc: Kevin Tian , "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" , Jun Nakajima , "keir@xen.org" List-Id: xen-devel@lists.xenproject.org >>> On 22.12.15 at 09:12, wrote: > 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. > " Okay, understood. But then you should follow _that_ model (using == instead of >=) instead of inventing a 3rd variant. Similar things should be done in similar ways so that they can be easily recognized being similar. Otoh the function isn't being called from code other than GUEST_PAGING_LEVELS == 4, so at present it could be static, which would take care of the latent linking issue Andrew referred to. >> > + 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". Perhaps just pkey_fault() then, since it's clear there's no other fault possible here besides a 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. Okay - I thought I had seen something like this mentioned in another sub-thread. >> > @@ -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. I didn't suggest to drop guest_l2e_get_pkey(). I'd like to see the #ifdef-s go away if at all possible, to help code readability. Jan