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 16:38:09 +0000 Message-ID: <56797C71.3090607@citrix.com> References: <1450780234-17236-1-git-send-email-huaitong.han@intel.com> <1450780234-17236-5-git-send-email-huaitong.han@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------000901040909000000020900" Return-path: In-Reply-To: <1450780234-17236-5-git-send-email-huaitong.han@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: Huaitong Han , jbeulich@suse.com, andrew.cooper3@citrix.com, jun.nakajima@intel.com, eddie.dong@intel.com, kevin.tian@intel.com, george.dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com, keir@xen.org Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --------------000901040909000000020900 Content-Type: text/plain; charset="utf-8" Content-Length: 1651 Content-Transfer-Encoding: quoted-printable On 22/12/15 10:30, Huaitong Han wrote: > Protection keys define a new 4-bit protection key field(PKEY) in bits 62:59 of > leaf entries of the page tables. > > PKRU register defines 32 bits, there are 16 domains and 2 attribute bits per > domain in pkru, for each i (0 =E2=89=A4 i =E2=89=A4 15), PKRU[2i] is the access-disable bit for > protection key i (ADi); PKRU[2i+1] is the write-disable bit for protection key > i (WDi). PKEY is index to a defined domain. > > A fault is considered as a PKU violation if all of the following conditions are > ture: > 1.CR4_PKE=3D1. > 2.EFER_LMA=3D1. > 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=3D1 > or The access is a data write and PKRU.WD=3D1 > and either CR0.WP=3D1 or it is a user access. One comment you didn't address from v3, however: "At the moment PFEC_insn_fetch is only set in hvm_fetch_from_guest_virt() if hvm_nx_enabled() or hvm_smep_enabled() are true. Which means that if you *don't* have nx or smep enabled, then the patch series as is will fault on instruction fetches when it shouldn't. (I don't remember anyone mentioning nx or smep being enabled as a prerequisite for pkeys.)" I think realistically the only way to address this is to start making the clean separation between "pfec in" and "pfec out" I mentioned in the previous discussion. I've coded up the attached patch, but only compile-tested it. Can you give it a look to see if you think it is correct, test it, include it in your next patch series=3F Thanks, -George --------------000901040909000000020900 Content-Type: text/x-patch; name="0001-xen-mm-Clean-up-pfec-handling-in-gva_to_gfn.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-xen-mm-Clean-up-pfec-handling-in-gva_to_gfn.patch" =46rom b8ec8e14c670215587a4e74c3aaec8dab6f26a8c Mon Sep 17 00:00:00 2001 From: George Dunlap Date: Tue, 22 Dec 2015 16:17:22 +0000 Subject: [PATCH] xen/mm: Clean up pfec handling in gva_to_gfn At the moment, the pfec argument to gva_to_gfn has two functions: * To inform guest_walk what kind of access is happenind * As a value to pass back into the guest in the event of a fault. Unfortunately this is not quite treated consistently: the hvm_fetch_* function will "pre-clear" the PFEC_insn_fetch flag before calling gva_to_gfn; meaning guest_walk doesn't actually know whether a given access is an instruction fetch or not. This works now, but will cause issues when pkeys are introduced, since guest_walk will need to know whether an access is an instruction fetch even if it doesn't return PFEC_insn_fetch. Fix this by making a clean separation for in and out functionalities of the pfec argument: 1. Always pass in the access type to gva_to_gfn 2. Filter out inappropriate access flags before returning from gva_to_gfn= =2E (The PFEC_insn_fetch flag should only be passed to the guest if either NX= or SMEP is enabled. See Intel 64 Developer's Manual, Volume 3, Section 4.7.= ) Signed-off-by: George Dunlap --- xen/arch/x86/hvm/hvm.c | 8 ++------ xen/arch/x86/mm/hap/guest_walk.c | 10 +++++++++- xen/arch/x86/mm/shadow/multi.c | 6 ++++++ 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 08cef1f..aa1f64f 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4423,11 +4423,9 @@ enum hvm_copy_result hvm_copy_from_guest_virt( enum hvm_copy_result hvm_fetch_from_guest_virt( void *buf, unsigned long vaddr, int size, uint32_t pfec) { - if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) ) - pfec |=3D PFEC_insn_fetch; return __hvm_copy(buf, vaddr, size, HVMCOPY_from_guest | HVMCOPY_fault | HVMCOPY_virt,= - PFEC_page_present | pfec); + PFEC_page_present | PFEC_insn_fetch | pfec); } =20 enum hvm_copy_result hvm_copy_to_guest_virt_nofault( @@ -4449,11 +4447,9 @@ enum hvm_copy_result hvm_copy_from_guest_virt_nofa= ult( enum hvm_copy_result hvm_fetch_from_guest_virt_nofault( void *buf, unsigned long vaddr, int size, uint32_t pfec) { - if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) ) - pfec |=3D PFEC_insn_fetch; return __hvm_copy(buf, vaddr, size, HVMCOPY_from_guest | HVMCOPY_no_fault | HVMCOPY_vi= rt, - PFEC_page_present | pfec); + PFEC_page_present | PFEC_insn_fetch | pfec); } =20 unsigned long copy_to_user_hvm(void *to, const void *from, unsigned int = len) diff --git a/xen/arch/x86/mm/hap/guest_walk.c b/xen/arch/x86/mm/hap/guest= _walk.c index 11c1b35..2dce111 100644 --- a/xen/arch/x86/mm/hap/guest_walk.c +++ b/xen/arch/x86/mm/hap/guest_walk.c @@ -82,7 +82,7 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)( if ( !top_page ) { pfec[0] &=3D ~PFEC_page_present; - return INVALID_GFN; + goto out_tweak_pfec; } top_mfn =3D _mfn(page_to_mfn(top_page)); =20 @@ -136,6 +136,14 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)= ( if ( missing & _PAGE_SHARED ) pfec[0] =3D PFEC_page_shared; =20 +out_tweak_pfec: + /*=20 + * Intel 64 Volume 3, Section 4.7: The PFEC_insn_fetch flag is set + * only when NX or SMEP are enabled. + */ + if ( !hvm_nx_enabled(v) && !hvm_smep_enabled(v) ) + pfec[0] &=3D ~PFEC_insn_fetch; + return INVALID_GFN; } =20 diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/mult= i.c index 58f7e72..3844c2d 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3668,6 +3668,12 @@ sh_gva_to_gfn(struct vcpu *v, struct p2m_domain *p= 2m, pfec[0] &=3D ~PFEC_page_present; if ( missing & _PAGE_INVALID_BITS ) pfec[0] |=3D PFEC_reserved_bit; + /*=20 + * Intel 64 Volume 3, Section 4.7: The PFEC_insn_fetch flag is + * set only when NX or SMEP are enabled. + */ + if ( !hvm_nx_enabled(v) && !hvm_smep_enabled(v) ) + pfec[0] &=3D ~PFEC_insn_fetch; return INVALID_GFN; } gfn =3D guest_walk_to_gfn(&gw); --=20 2.1.4 --------------000901040909000000020900 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------000901040909000000020900--