Re: [PATCH 3/4] x86/i387: Rework fpu_fxrstor() given a newer toolchain baseline

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/2026 4:13 pm, Nicola Vetrini wrote:
> 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.

It's fully reachable.

FXRSTOR can fault multiple times, and can fault for reasons unrelated to
the contents of the buffer.  Fault recovery isn't even limited to only
#GP[0] only, and FXRSTOR can suffer #AC from a misaligned pointer.

If Xen is operating properly, it oughtn't to fault more than once, but
right now the logic will livelock rather than terminate.

Further fixes being discussed (better auditing of toolstack-provided
buffers) should cause it never to fault for buffer-contents reasons, at
which point I'll be removing the retry aspect and escalating to
domain_crash() unconditionally.

~Andrew