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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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