diff mbox

[V8,5/5] x86/hvm: pkeys, add pkeys support for cpuid handling

Message ID 1454398542-4815-6-git-send-email-huaitong.han@intel.com
State New, archived
Headers show

Commit Message

Huaitong Han Feb. 2, 2016, 7:35 a.m. UTC
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(-)

Comments

Jan Beulich Feb. 3, 2016, 9:50 a.m. UTC | #1
>>> 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
Huaitong Han Feb. 3, 2016, 10:04 a.m. UTC | #2
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
>
Jan Beulich Feb. 3, 2016, 11:05 a.m. UTC | #3
>>> 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 mbox

Patch

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);
         }