Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 11/08/2010 10:22 PM, Haitao Shan wrote:
> Hi, Jeremy,
>
> This patch allows pv-ops kernel to detect whether XSAVE is supported
> (before masking it unconditionally through xen_cpuid).
> Can you please have review? Thanks!
>
> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>
> Shan Haitao
>

For future reference:

Please post patches inline if possible.

If you must use an attachment to prevent your mail system from
corrupting the patch, please include a complete description of the patch
(what is it trying to do, how does it do it, what is the outcome?) with
signed-off-bys in the the patch itself.

> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index
> fd3803e..03bfaf7 100644 --- a/arch/x86/xen/enlighten.c +++
> b/arch/x86/xen/enlighten.c @@ -252,6 +252,13 @@ static __init void
> xen_init_cpuid_mask(void) (1 << X86_FEATURE_MCA) | /* disable MCA */
> (1 << X86_FEATURE_APIC) | /* disable local APIC */ (1 <<
> X86_FEATURE_ACPI)); /* disable ACPI */ + ax = 1; + xen_cpuid(&ax, &bx,
> &cx, &dx); + + /* Xen will set CR4.OSXSAVE if supported and not
> disabled by force */ + if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) && +
> cx & (1 << (X86_FEATURE_OSXSAVE % 32)) ) + return;
> cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable
> XSAVE */ 


I cleaned this up a bit (fixed formatting to Linux style, reversed the
sense of the if() and masked OSXSAVE as well, even though it won't make
a difference).  But I'm still a bit concerned about the back-compat
issues around this.

What happens if the guest OS does not support XSAVE (older versions of
Linux)?  Could usermode code see OSXSAVE set in the cpuid feature flags
and attempt to use XSAVE, even if XSAVE isn't set?  In other words, can
usermode ever normally see OSXSAVE set, but XSAVE clear?

We can't mask OSXSAVE to usermode because they can run the naked CPUID
instruction, so I guess the only way to cause it to be cleared would be
to clear CR4.OSXSAVE?  But that seems like a very expensive thing to do
on a vcpu context switch.

How much testing of real usermode code have you done with this?  How
many combinations of XSAVE support in Xen, Linux and usermode have you
tried?

    J