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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.02.16 at 11:04, <huaitong.han@xxxxxxxxx> wrote:
> On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote:
>> > > > On 02.02.16 at 08:35, <huaitong.han@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxx 

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


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