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

["Han, Huaitong" <huaitong.han@intel.com>]

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
>