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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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