From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V5 4/6] x86/hvm: pkeys, add pkeys support for guest_walk_tables Date: Tue, 22 Dec 2015 15:23:54 +0000 Message-ID: <56796B0A.8060608@citrix.com> References: <1450780234-17236-1-git-send-email-huaitong.han@intel.com> <1450780234-17236-5-git-send-email-huaitong.han@intel.com> <567957ED02000078000C23F5@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: <567957ED02000078000C23F5@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 , Huaitong Han Cc: kevin.tian@intel.com, 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@intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 22/12/15 13:02, Jan Beulich wrote: >>>> On 22.12.15 at 11:30, wrote: > > I dislike having to repeat this: Please trim your Cc lists. > >> --- a/xen/arch/x86/mm/guest_walk.c >> +++ b/xen/arch/x86/mm/guest_walk.c >> @@ -90,6 +90,57 @@ static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty) >> return 0; >> } >> >> +extern bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec, >> + uint32_t pte_flags, uint32_t pte_pkey); >> +#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS >> +bool_t pkey_fault(struct vcpu *vcpu, uint32_t pfec, >> + uint32_t pte_flags, uint32_t pte_pkey) >> +{ > > See my comments on the previous version. Please avoid sending new > versions without having addressed all comments on the previous one > (verbally or by code changes). Having done the suggested change > just partially (by removing the #ifdef-s from the call sites) you now > do the key check universally, and things remain correct just because > of the long mode check in the middle of the function. > >> + unsigned int pkru = 0; >> + bool_t pkru_ad, pkru_wd; >> + >> + bool_t pf = !!(pfec & PFEC_page_present); > > There's still this stray blank line above (and I continue to wonder > whether you really need all these boolean variables many of which > get used just once). I suspect the "stray blank line" was added for readability. But I agree that I'd prefer not to use local boolean variables, and just to put the flag checking inline. > >> + 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 isn't present, PKEY isn't checked. */ >> + if ( !pf || is_pv_vcpu(vcpu) ) >> + return 0; >> + >> + /* >> + * 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: *true >> + * 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 || !(pte_flags & _PAGE_USER) ) And I think you might as well make this one line per condition, something like this: if ( is_pv_vcpu(vcpu) || !hvm_pku_enabled(vcpu) || !hvm_long_mode_enabled(vcpu) !(pfec & PFEC_page_present) || (pfec & (PFEC_insn_fetch|PFEC_reserved_bit)) || !(pte_flags & _PAGE_USER) ) return 0; -George