Re: [PATCH] x86emul: further FPU env testing relaxation for AMD-like CPUs

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/08/2020 10:36, Jan Beulich wrote:
> See the code comment that's being extended. Additionally a few more
> zap_fpsel() invocations are needed - whenever we stored state after
> there potentially having been a context switch behind our backs.
>
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Tested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

>
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -752,6 +752,13 @@ static struct x86_emulate_ops emulops =
>   * 64-bit OSes may not (be able to) properly restore the two selectors in
>   * the FPU environment. Zap them so that memcmp() on two saved images will
>   * work regardless of whether a context switch occurred in the middle.
> + *
> + * Additionally on AMD-like CPUs FDP/FIP/FOP may get lost across context
> + * switches, when there's no unmasked pending FP exception: With

I think you want a full stop rather than a colon, and ...

> + * CPUID[80000008].EBX[2] clear, the fields don't get written/read by
> + * {F,}XSAVE / {F,}XRSTOR, which OSes often compensate for by invoking an
> + * insn forcing the fields to gain a deterministic value. Whereas with said

... a comma here rather than a full stop.

Having "whereas" at the beginning of a sentence like this is weird,
given that you're contrasting the behaviour of the CPUID bit.

Also, the more usual CPUID syntax would be CPUID.0x80000008.EBX[2].

~Andrew

> + * bit set, zeroes will get written (and hence later restored).
>   */
>  static void zap_fpsel(unsigned int *env, bool is_32bit)
>  {
>