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
