| Message ID | 1454398542-4815-6-git-send-email-huaitong.han@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 02.02.16 at 08:35, <huaitong.han@intel.com> 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 <huaitong.han@intel.com> > --- > 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. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4572,7 +4572,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > __clear_bit(X86_FEATURE_APIC & 31, edx); > > /* Fix up OSXSAVE. */ > - if ( cpu_has_xsave ) > + if ( *ecx & cpufeat_mask(X86_FEATURE_XSAVE) ) > *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_OSXSAVE) ? > cpufeat_mask(X86_FEATURE_OSXSAVE) : 0; First of all I would have wished that since you're already touching it, you would have brought it into the same shape as you're doing for PKU below. And then here as well as ... > @@ -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? Jan
On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote: > > > > On 02.02.16 at 08:35, <huaitong.han@intel.com> 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 <huaitong.han@intel.com> > > --- > > 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 > > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -4572,7 +4572,7 @@ void hvm_cpuid(unsigned int input, unsigned > > int *eax, unsigned int *ebx, > > __clear_bit(X86_FEATURE_APIC & 31, edx); > > > > /* Fix up OSXSAVE. */ > > - if ( cpu_has_xsave ) > > + if ( *ecx & cpufeat_mask(X86_FEATURE_XSAVE) ) > > *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & > > X86_CR4_OSXSAVE) ? > > cpufeat_mask(X86_FEATURE_OSXSAVE) : 0; > > First of all I would have wished that since you're already touching > it, > you would have brought it into the same shape as you're doing for > PKU below. And then here as well as ... It will be updated in V9. > > > @@ -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. > > Jan >
>>> On 03.02.16 at 11:04, <huaitong.han@intel.com> wrote: > On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote: >> > > > On 02.02.16 at 08:35, <huaitong.han@intel.com> 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 <huaitong.han@intel.com> >> > --- >> > 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
diff --git a/tools/libxc/xc_cpufeature.h b/tools/libxc/xc_cpufeature.h index ee53679..866cf0b 100644 --- a/tools/libxc/xc_cpufeature.h +++ b/tools/libxc/xc_cpufeature.h @@ -144,4 +144,7 @@ #define X86_FEATURE_CLFLUSHOPT 23 /* CLFLUSHOPT instruction */ #define X86_FEATURE_CLWB 24 /* CLWB instruction */ +/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx) */ +#define X86_FEATURE_PKU 3 + #endif /* __LIBXC_CPUFEATURE_H */ diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index c142595..5408dd0 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -430,9 +430,11 @@ static void xc_cpuid_hvm_policy(xc_interface *xch, bitmaskof(X86_FEATURE_PCOMMIT) | bitmaskof(X86_FEATURE_CLWB) | bitmaskof(X86_FEATURE_CLFLUSHOPT)); + regs[2] &= bitmaskof(X86_FEATURE_PKU); } else - regs[1] = 0; - regs[0] = regs[2] = regs[3] = 0; + regs[1] = regs[2] = 0; + + regs[0] = regs[3] = 0; break; case 0x0000000d: diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 5ec2ae1..1389173 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4572,7 +4572,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, __clear_bit(X86_FEATURE_APIC & 31, edx); /* Fix up OSXSAVE. */ - if ( cpu_has_xsave ) + if ( *ecx & cpufeat_mask(X86_FEATURE_XSAVE) ) *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_OSXSAVE) ? cpufeat_mask(X86_FEATURE_OSXSAVE) : 0; @@ -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); - /* Don't expose PCOMMIT to hvm when VMX support is not available */ + /* Don't expose PCOMMIT to hvm when VMX support is not available. */ if ( !cpu_has_vmx_pcommit ) *ebx &= ~cpufeat_mask(X86_FEATURE_PCOMMIT); }
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 <huaitong.han@intel.com> --- 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(-)