On 2026-01-05 16:52, Jan Beulich wrote:
On 30.12.2025 14:54, Andrew Cooper wrote:
Use asm goto rather than hiding a memset() in the fixup section. With
the
compiler now able to see the write into fpu_ctxt (as opposed to the
asm
constraint erroneously stating it as input-only), it validly objects
to the
pointer being const.
While FXRSTOR oughtn't to fault on an all-zeros input, avoid a risk of
an
infinite loop entirely by using a fixup scheme similar to xrstor(),
and
crashing the domain if we run out options.
Question being - does ...
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -38,7 +38,8 @@ static inline void fpu_xrstor(struct vcpu *v,
uint64_t mask)
/* Restore x87 FPU, MMX, SSE and SSE2 state */
static inline void fpu_fxrstor(struct vcpu *v)
{
- const fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
+ fpusse_t *fpu_ctxt = &v->arch.xsave_area->fpu_sse;
+ unsigned int faults = 0;
/*
* Some CPUs don't save/restore FDP/FIP/FOP unless an exception
@@ -59,49 +60,41 @@ static inline void fpu_fxrstor(struct vcpu *v)
* possibility, which may occur if the block was passed to us by
control
* tools or through VCPUOP_initialise, by silently clearing the
block.
*/
+ retry:
switch ( __builtin_expect(fpu_ctxt->x[FPU_WORD_SIZE_OFFSET], 8) )
{
default:
- asm_inline volatile (
+ asm_inline volatile goto (
"1: fxrstorq %0\n"
- ".section .fixup,\"ax\" \n"
- "2: push %%"__OP"ax \n"
- " push %%"__OP"cx \n"
- " push %%"__OP"di \n"
- " lea %0,%%"__OP"di \n"
- " mov %1,%%ecx \n"
- " xor %%eax,%%eax \n"
- " rep ; stosl \n"
- " pop %%"__OP"di \n"
- " pop %%"__OP"cx \n"
- " pop %%"__OP"ax \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- :
- : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
+ _ASM_EXTABLE(1b, %l[fault])
+ :: "m" (*fpu_ctxt)
+ :: fault );
break;
+
case 4: case 2:
- asm_inline volatile (
- "1: fxrstor %0 \n"
- ".section .fixup,\"ax\"\n"
- "2: push %%"__OP"ax \n"
- " push %%"__OP"cx \n"
- " push %%"__OP"di \n"
- " lea %0,%%"__OP"di \n"
- " mov %1,%%ecx \n"
- " xor %%eax,%%eax \n"
- " rep ; stosl \n"
- " pop %%"__OP"di \n"
- " pop %%"__OP"cx \n"
- " pop %%"__OP"ax \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- :
- : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
+ asm_inline volatile goto (
+ "1: fxrstor %0\n"
+ _ASM_EXTABLE(1b, %l[fault])
+ :: "m" (*fpu_ctxt)
+ :: fault );
break;
}
+
+ return;
+
+ fault:
+ faults++;
+
+ switch ( faults )
+ {
+ case 1: /* Stage 1: Reset all state. */
+ memset(fpu_ctxt, 0, sizeof(*fpu_ctxt));
+ goto retry;
+
+ default: /* Stage 2: Nothing else to do. */
+ domain_crash(v->domain, "Uncorrectable FXRSTOR fault\n");
+ return;
... this then count as unreachable and/or dead code in Misra's terms?
Nicola?
Sure, Eclair wouldn't be able to spot it, but that's no excuse imo.
Jan
Right now, probably not, but even if it did, an ASSERT_UNREACHABLE can
be added in the default branch to deal with that.
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
