Re: [Xen-devel] [PATCH][XTF] add FPU/SIMD register state test

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/17 11:07, Jan Beulich wrote:
> Add tests to verify that
> - FPU insns leave correct (guest) values in FIP/FDP/FOP/FCS/FDS,
> - FPU insns writing memory don't update FPU register state when the
>   write faults (at the example of FISTP),
> - VCVTPS2PH (once implemented in the emulator) doesn't update MXCSR
>   if its write faults (VCVTPS2PH currently is the only SIMD insn
>   writing to memory and updating register state).
>
> Note that the AMD-specific code in fpu_fxrstor() and xrstor() causes
> the FISTP part of this test to always fail. I don't really have any
> idea what to do about this (other than perhaps skipping the test on AMD
> altogether).
>
> Note further that the FCS part of 64-bit variant of the FSTP test also
> always fails on AMD, since, other than on Intel, {F,}XRSTOR with REX.W
> set do not appear to clear FCS/FDS (I can't find any statement either
> way in the PM).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> TBD: It is not clear to me how to deal with the native execution
>      failure of VCVTPS2PH on at least some Intel models.

As a general comment, tests dealing with quirks like this should
tolerate real hardware behaviour, without causing the test to fail. 
There is nothing useful from having tests failing in cases we cant/wont
do anything to fix.

As for the Intel behaviour, Is this documented, or does it have an
erratum reference?

For the AMD behaviour, some more though is going to be required.

>
> --- a/include/arch/x86/cpuid.h
> +++ b/include/arch/x86/cpuid.h
> @@ -77,6 +77,7 @@ static inline bool cpu_has(unsigned int
>  #define cpu_has_pcid            cpu_has(X86_FEATURE_PCID)
>  #define cpu_has_xsave           cpu_has(X86_FEATURE_XSAVE)
>  #define cpu_has_avx             cpu_has(X86_FEATURE_AVX)
> +#define cpu_has_f16c            cpu_has(X86_FEATURE_F16C)
>  
>  #define cpu_has_syscall         cpu_has(X86_FEATURE_SYSCALL)
>  #define cpu_has_nx              cpu_has(X86_FEATURE_NX)
> --- /dev/null
> +++ b/tests/fpu-state/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := fpu-state
> +CATEGORY  := functional
> +TEST-ENVS := hvm64 hvm32pse
> +
> +obj-perenv += main.o
> +
> +include $(ROOT)/build/gen.mk
> --- /dev/null
> +++ b/tests/fpu-state/main.c
> @@ -0,0 +1,184 @@
> +/**
> + * @file tests/fpu-state/main.c
> + * @ref test-fpu-state - Emulation of FPU state
> + *
> + * @page test-fpu-state FPU State Emulation
> + *
> + * FPU code/data pointers and opcode must not be the ones resulting
> + * from the stub execution in the hypervisor.
> + *
> + * FPU and SIMD instructions faulting during memory write must not
> + * update the respective register files.
> + *
> + * @see tests/fpu-state/main.c
> + */
> +#include <xtf.h>
> +
> +#include <arch/x86/exinfo.h>
> +
> +const char test_title[] = "FPU State";
> +
> +struct x87_env {

Can this include 32bit in the name.  The 16bit layout is different.

In fact, if you could put it in arch/x86/x87.h, that would be even more
helpful.

> +    uint16_t cw, :16;
> +    uint16_t sw, :16;
> +    uint16_t tw, :16;
> +    uint32_t ip;
> +    uint16_t cs;
> +    uint16_t op:11, :5;
> +    uint32_t dp;
> +    uint16_t ds, :16;
> +};
> +
> +void probe_fstp(bool force)
> +{
> +    const uint8_t *fstp_offs;
> +    uint32_t flt;
> +    struct x87_env fenv;
> +
> +    fenv.cw = 0x35f; /* unmask PE */
> +    asm volatile ( "fninit;"
> +                   "fldcw %[cw];"
> +                   "fldpi;"
> +                   "mov $1f, %[offs];"

You can have the compiler do all of this by using a named label, and using

extern char fstp_offs[] asm("$label");

~Andrew

> +                   "test %[fep], %[fep];"
> +                   "jz 1f;"
> +                   _ASM_XEN_FEP
> +                   "1: fstps %[data]; 2:"
> +                   : [offs] "=&g" (fstp_offs), [data] "=m" (flt)
> +                   : [cw] "m" (fenv.cw), [fep] "q" (force) );
> +
> +    asm ( "fnstenv %0" : "=m" (fenv) );
> +    if ( fenv.ip != (unsigned long)fstp_offs )
> +        xtf_failure("Fail: FIP wrong (%08x)\n", fenv.ip);
> +    if ( fenv.cs && fenv.cs != __KERN_CS )
> +        xtf_failure("Fail: FCS wrong (%04x)\n", fenv.cs);
> +    if ( fenv.dp != (unsigned long)&flt )
> +        xtf_failure("Fail: FDP wrong (%08x)\n", fenv.dp);
> +    if ( fenv.ds && fenv.ds != __KERN_DS )
> +        xtf_failure("Fail: FDS wrong (%04x)\n", fenv.ds);
> +    /* Skip possible opcode prefixes before checking the opcode. */
> +    while ( (fstp_offs[0] & ~7) != 0xd8 )
> +        ++fstp_offs;
> +    if ( fenv.op && fenv.op != (((fstp_offs[0] & 7) << 8) | fstp_offs[1]) )
> +        xtf_failure("Fail: FOP wrong (%03x)\n", fenv.op);
> +}
>


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