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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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