| Message ID | b2667393-0196-30de-86e9-b7a6145ed03d@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86emul: further FPU env testing relaxation for AMD-like CPUs | expand |
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@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- 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) > { >
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@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> 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
--- 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 + * 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 + * bit set, zeroes will get written (and hence later restored). */ static void zap_fpsel(unsigned int *env, bool is_32bit) { @@ -765,6 +772,21 @@ static void zap_fpsel(unsigned int *env, env[2] &= ~0xffff; env[3] &= ~0xffff; } + + if ( cp.x86_vendor != X86_VENDOR_AMD && cp.x86_vendor != X86_VENDOR_HYGON ) + return; + + if ( is_32bit ) + { + env[3] = 0; + env[4] = 0; + env[5] = 0; + } + else + { + env[1] &= 0xffff; + env[2] = 0; + } } static void zap_xfpsel(unsigned int *env) @@ -2460,6 +2482,7 @@ int main(int argc, char **argv) regs.edx = (unsigned long)res; rc = x86_emulate(&ctxt, &emulops); asm volatile ( "fnstenv %0" : "=m" (res[9]) :: "memory" ); + zap_fpsel(&res[9], true); if ( (rc != X86EMUL_OKAY) || memcmp(res + 2, res + 9, 28) || (regs.eip != (unsigned long)&instr[3]) ) @@ -2487,6 +2510,7 @@ int main(int argc, char **argv) res[23] = 0xaa55aa55; res[24] = 0xaa55aa55; rc = x86_emulate(&ctxt, &emulops); + zap_fpsel(&res[0], false); if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 25, 94) || (res[23] >> 16) != 0xaa55 || @@ -2514,6 +2538,7 @@ int main(int argc, char **argv) regs.edx = (unsigned long)res; rc = x86_emulate(&ctxt, &emulops); asm volatile ( "fnsave %0" : "=m" (res[27]) :: "memory" ); + zap_fpsel(&res[27], true); if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 27, 108) || (regs.eip != (unsigned long)&instr[2]) )
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@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>