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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2020 16:46, Andrew Cooper wrote:
> 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>

Thanks.

>> --- 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 ...

I'd prefer to stick to the colon here, while ...

>> + * 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].

... I've adjusted these.

Jan