Re: [Xen-devel] [PATCH 01/10] x86/hvm: pkeys, add pkeys support for cpuid handling

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of Andrew Cooper
> Sent: Monday, November 16, 2015 8:01 PM
> To: Han, Huaitong <huaitong.han@xxxxxxxxx>; jbeulich@xxxxxxxx; Nakajima,
> Jun <jun.nakajima@xxxxxxxxx>; Dong, Eddie <eddie.dong@xxxxxxxxx>; Tian,
> Kevin <kevin.tian@xxxxxxxxx>; george.dunlap@xxxxxxxxxxxxx;
> ian.jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> ian.campbell@xxxxxxxxxx; wei.liu2@xxxxxxxxxx; keir@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 01/10] x86/hvm: pkeys, add pkeys support
> for cpuid handling
> 
> On 16/11/15 10:31, Huaitong Han 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.
> >
> > Signed-off-by: Huaitong Han <huaitong.han@xxxxxxxxx>
> >
> > diff --git a/tools/libxc/xc_cpufeature.h b/tools/libxc/xc_cpufeature.h
> > index c3ddc80..f6a9778 100644
> > --- a/tools/libxc/xc_cpufeature.h
> > +++ b/tools/libxc/xc_cpufeature.h
> > @@ -141,5 +141,7 @@
> >  #define X86_FEATURE_ADX         19 /* ADCX, ADOX instructions */
> >  #define X86_FEATURE_SMAP        20 /* Supervisor Mode Access Protection
> */
> >
> > +/* 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 e146a3e..34bb964 100644
> > --- a/tools/libxc/xc_cpuid_x86.c
> > +++ b/tools/libxc/xc_cpuid_x86.c
> > @@ -367,9 +367,11 @@ static void xc_cpuid_hvm_policy(
> >                          bitmaskof(X86_FEATURE_ADX)  |
> >                          bitmaskof(X86_FEATURE_SMAP) |
> >                          bitmaskof(X86_FEATURE_FSGSBASE));
> > +            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 615fa89..66917ff 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4518,6 +4518,12 @@ void hvm_cpuid(unsigned int input, unsigned
> int *eax, unsigned int *ebx,
> >          /* Don't expose INVPCID to non-hap hvm. */
> >          if ( (count == 0) && !hap_enabled(d) )
> >              *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> > +
> > +        if ( (count == 0) && !(cpu_has_pku && hap_enabled(d)) )
> > +            *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
> > +        if ( (count == 0) && cpu_has_pku )
> > +            *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) ?
> > +                     cpufeat_mask(X86_FEATURE_OSPKE) : 0;
> 
> This logic (all being gated on count == 0 && !hap_enabled() ) should
> extend the INVPCID if() statement.
> 
> Setting OSPKE should be gated on *ecx having PKU and guest CR4 alone.

Although I agree that it is better to check *ecx having PKU here, however,

> As it currently stands, a guest could end up observing OSPKE but not PKU.

I am wondering how this can happen. If guest cannot see PKU, which means
this feature is not exposed to it, the guest will not set the PKE bit in cr4 
(guests
will set PKE only when CPUID says that this feature is supported), hence it
cannot see OSPKE bit.

BTW, Huaitong, it is better to put this patch in the end of this series, since
we usually expose the feature after all other things are finished.

Thanks,
Feng

> 
> >          break;
> >      case 0xb:
> >          /* Fix the x2APIC identifier. */
> > diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-
> x86/cpufeature.h
> > index 9a01563..3c3b95f 100644
> > --- a/xen/include/asm-x86/cpufeature.h
> > +++ b/xen/include/asm-x86/cpufeature.h
> > @@ -154,6 +154,10 @@
> >  #define X86_FEATURE_ADX            (7*32+19) /* ADCX, ADOX instructions
> */
> >  #define X86_FEATURE_SMAP   (7*32+20) /* Supervisor Mode Access
> Prevention */
> >
> > +/* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 8 */
> > +#define X86_FEATURE_PKU    (8*32+ 3) /* Protection Keys for Userspace */
> > +#define X86_FEATURE_OSPKE  (8*32+ 4) /* OS Protection Keys
> Enable */
> > +
> >  #if !defined(__ASSEMBLY__) && !defined(X86_FEATURES_ONLY)
> >  #include <xen/bitops.h>
> >
> > @@ -193,6 +197,7 @@
> >
> >  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
> >  #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
> > +#define cpu_has_pku             boot_cpu_has(X86_FEATURE_PKU)
> 
> This read overflows c->x86_capabilities, as you didn't bump NCAPINTs
> 
> I see that you bump it in the following patch, but you must move that
> hunk forwards to this patch.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel