Re: [Xen-devel] [PATCH 5/6] x86/AMD: Fix handling of FPU pointer on Zen hardware

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.12.18 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> AMD hardware before Zen doesn't safe/restore the FPU error pointers
> unless an unmasked FPU exception is pending.  Zen processors have a
> feature bit indicating that this (mis)behaviour no longer exists.
> 
> Express the common logic in terms of cpu_bug_fpu_err_ptr as Hygon
> processors (being Zen derivatives) won't inherit this behaviour.

While they've decided to change the behavior, I still don't think this
was to be considered a bug, as it was well documented for a long
time (albeit perhaps not from the beginning). I again question the
validity of cpu_bug_ as a prefix here.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -369,15 +369,13 @@ void xrstor(struct vcpu *v, uint64_t mask)
>      unsigned int faults, prev_faults;
>  
>      /*
> -     * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
> +     * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
>       * is pending. Clear the x87 state here by setting it to fixed
>       * values. The hypervisor data segment can be sometimes 0 and
>       * sometimes new user value. Both should be ok. Use the FPU saved
>       * data block as a safe address because it should be in L1.
>       */
> -    if ( (mask & ptr->xsave_hdr.xstate_bv & X86_XCR0_FP) &&
> -         !(ptr->fpu_sse.fsw & ~ptr->fpu_sse.fcw & 0x003f) &&
> -         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +    if ( cpu_bug_fpu_err_ptr )
>          asm volatile ( "fnclex\n\t"        /* clear exceptions */
>                         "ffree %%st(7)\n\t" /* clear stack tag */
>                         "fildl %0"          /* load to clear state */

This change will result in clobbering FPU state when X86_CR0_FP
is not set in "mask & ptr->xsave_hdr.xstate_bv". I'm anyway
unconvinced that dropping the conditions is a good thing, even
less so in an unrelated patch (a dedicated patch would otherwise
also adjust the similar FXRSTOR code).

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -237,6 +237,7 @@ XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF 
> Read Only interface */
>  
>  /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
>  XEN_CPUFEATURE(CLZERO,        8*32+ 0) /*A  CLZERO instruction */
> +XEN_CPUFEATURE(XSAVEERRPTR,   8*32+ 2) /*A  (F)XSAVE Error pointers always 
> updated. */

Any particular reason why you don't derive from AMD's name
(RstrFpErrPtrs)? Also if their documentation is to be trusted,
then the comment is wrongly mentioning FXSAVE; it's
questionable whether mentioning XSAVE but not XRSTOR is a
good thing.

As a side note - this is an example of a CPUID bit which would
better not require white listing for guest exposure.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel