From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V8 5/5] x86/hvm: pkeys, add pkeys support for cpuid handling Date: Wed, 03 Feb 2016 04:05:53 -0700 Message-ID: <56B1ED2102000078000CDEC2@prv-mh.provo.novell.com> References: <1454398542-4815-1-git-send-email-huaitong.han@intel.com> <1454398542-4815-6-git-send-email-huaitong.han@intel.com> <56B1DB6F02000078000CDDD8@prv-mh.provo.novell.com> <1454493858.4350.10.camel@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1454493858.4350.10.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: "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "tim@xen.org" , "keir@xen.org" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org >>> On 03.02.16 at 11:04, wrote: > On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote: >> > > > On 02.02.16 at 08:35, wrote: >> > This patch adds pkeys support for cpuid handing. >> > >> > Pkeys hardware support is CPUID.7.0.ECX[3]:PKU. software support is >> > CPUID.7.0.ECX[4]:OSPKE and it reflects the support setting of >> > CR4.PKE. >> > >> > X86_FEATURE_OSXSAVE depends on guest X86_FEATURE_XSAVE, but >> > cpu_has_xsave >> > function reflects hypervisor X86_FEATURE_XSAVE, it is fixed too. >> > >> > Signed-off-by: Huaitong Han >> > --- >> > Changes in v7: >> > *Rebase in the latest tree. >> > *Add a comment for cpu_has_xsave adjustment. >> > *Adjust indentation. >> > >> > tools/libxc/xc_cpufeature.h | 3 +++ >> > tools/libxc/xc_cpuid_x86.c | 6 ++++-- >> > xen/arch/x86/hvm/hvm.c | 18 +++++++++++++----- >> > 3 files changed, 20 insertions(+), 7 deletions(-) >> >> This will need a tools maintainer's ack, so upon re-submission you >> should Cc them. > I will (again) defer this to x86 maintainers. -from wei.liu2@citrix.com Which means he makes his (future) ack dependent on ours; it does (to me at least) not mean tools maintainers don't need to give their ack anymore. >> > @@ -4593,16 +4593,24 @@ void hvm_cpuid(unsigned int input, unsigned >> > int *eax, unsigned int *ebx, >> > if ( !cpu_has_smap ) >> > *ebx &= ~cpufeat_mask(X86_FEATURE_SMAP); >> > >> > - /* Don't expose MPX to hvm when VMX support is not >> > available */ >> > + /* Don't expose MPX to hvm when VMX support is not >> > available. */ >> > if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) || >> > !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) ) >> > *ebx &= ~cpufeat_mask(X86_FEATURE_MPX); >> > >> > - /* Don't expose INVPCID to non-hap hvm. */ >> > if ( !hap_enabled(d) ) >> > - *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID); >> > + { >> > + /* Don't expose INVPCID to non-hap hvm. */ >> > + *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID); >> > + /* X86_FEATURE_PKU is not yet implemented for >> > shadow paging. */ >> > + *ecx &= ~cpufeat_mask(X86_FEATURE_PKU); >> > + } >> > + >> > + if ( (*ecx & cpufeat_mask(X86_FEATURE_PKU)) && >> > + (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) ) >> > + *ecx |= cpufeat_mask(X86_FEATURE_OSPKE); >> >> ... here - shouldn't we also clear the respective flag in the "else" >> case? > X86_FEATURE_OSPKE is not set by xc_cpuid_hvm_policy(tools), it reflects > the setting of CR4.PKE, so it does not need to be cleared like > X86_CR4_OSXSAVE. Tools side adjustments currently get done before applying config file overrides, and hence a config file could also wrongly set that flag - arguably one might call this an admin error, but why would we not adjust that if we easily can (the more that we also set the flag if we think it should be set)? Jan