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

[Jan Beulich <jbeulich@xxxxxxxx>]

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