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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@intel.com>
>
> 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. 
As it currently stands, a guest could end up observing OSPKE but not PKU.

>          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