Re: [PATCH 21/22] x86/traps: Introduce FRED entrypoints

[Jan Beulich <jbeulich@suse.com>]

On 08.08.2025 22:23, Andrew Cooper wrote:
> Under FRED, there's one entrypoint from Ring 3, and one from Ring 0.
> 
> FRED gives us a good stack (even for SYSCALL/SYSENTER), and a unified event
> frame on the stack, meaing that all software needs to do is spill the GPRs
> with a line of PUSHes.  Introduce PUSH_AND_CLEAR_GPRS and POP_GPRS for this
> purpose.
> 
> Introduce entry_FRED_R0() which to a first appoximation is complete for all
> event handling within Xen.
> 
> entry_FRED_R0() needs deriving from entry_FRED_R3(), so introduce a basic
> handler.  There is more work required to make the return-to-guest path work
> under FRED, so leave a BUG clearly in place.
> 
> Also introduce entry_from_{xen,pv}() to be the C level handlers.  By simply
> copying regs->fred_ss.vector into regs->entry_vector, we can reuse all the
> existing fault handlers.
> 
> Extend fatal_trap() to render the event type, including by name, when FRED is
> active.  This is slightly complicated, because X86_ET_OTHER must not use
> vector_name() or SYSCALL and SYSENTER get rendered as #BP and #DB.  Also,
> {read,write}_gs_shadow() needs modifying to avoid the SWAPGS instruction,
> which is disallowed in FRED mode.
> 
> This is sufficient to handle all interrupts and exceptions encountered during
> development, including plenty of Double Faults.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> SIMICS hasn't been updated to the FRED v9, and still wants ENDBR instructions
> at the entrypoints.
> ---
>  xen/arch/x86/include/asm/asm_defns.h |  65 ++++++++++++
>  xen/arch/x86/include/asm/msr.h       |   8 +-
>  xen/arch/x86/traps.c                 | 153 ++++++++++++++++++++++++++-
>  xen/arch/x86/x86_64/Makefile         |   1 +
>  xen/arch/x86/x86_64/entry-fred.S     |  35 ++++++
>  5 files changed, 256 insertions(+), 6 deletions(-)
>  create mode 100644 xen/arch/x86/x86_64/entry-fred.S
> 
> diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h
> index 72a0082d319d..a81a4043d0f1 100644
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -315,6 +315,71 @@ static always_inline void stac(void)
>          subq  $-(UREGS_error_code-UREGS_r15+\adj), %rsp
>  .endm
>  
> +/*
> + * Push and clear GPRs
> + */
> +.macro PUSH_AND_CLEAR_GPRS
> +        push  %rdi
> +        xor   %edi, %edi
> +        push  %rsi
> +        xor   %esi, %esi
> +        push  %rdx
> +        xor   %edx, %edx
> +        push  %rcx
> +        xor   %ecx, %ecx
> +        push  %rax
> +        xor   %eax, %eax
> +        push  %r8
> +        xor   %r8d, %r8d
> +        push  %r9
> +        xor   %r9d, %r9d
> +        push  %r10
> +        xor   %r10d, %r10d
> +        push  %r11
> +        xor   %r11d, %r11d
> +        push  %rbx
> +        xor   %ebx, %ebx
> +        push  %rbp
> +#ifdef CONFIG_FRAME_POINTER
> +/* Indicate special exception stack frame by inverting the frame pointer. */
> +        mov   %rsp, %rbp
> +        notq  %rbp
> +#else
> +        xor   %ebp, %ebp
> +#endif
> +        push  %r12
> +        xor   %r12d, %r12d
> +        push  %r13
> +        xor   %r13d, %r13d
> +        push  %r14
> +        xor   %r14d, %r14d
> +        push  %r15
> +        xor   %r15d, %r15d
> +.endm
> +
> +/*
> + * POP GPRs from a UREGS_* frame on the stack.  Does not modify flags.
> + *
> + * @rax: Alternative destination for the %rax value on the stack.
> + */
> +.macro POP_GPRS rax=%rax
> +        pop   %r15
> +        pop   %r14
> +        pop   %r13
> +        pop   %r12
> +        pop   %rbp
> +        pop   %rbx
> +        pop   %r11
> +        pop   %r10
> +        pop   %r9
> +        pop   %r8
> +        pop   \rax
> +        pop   %rcx
> +        pop   %rdx
> +        pop   %rsi
> +        pop   %rdi
> +.endm
> +
>  #ifdef CONFIG_PV32
>  #define CR4_PV32_RESTORE                               \
>      ALTERNATIVE_2 "",                                  \
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index b6b85b04c3fd..01f510315ffe 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -202,9 +202,9 @@ static inline unsigned long read_gs_base(void)
>  
>  static inline unsigned long read_gs_shadow(void)
>  {
> -    unsigned long base;
> +    unsigned long base, cr4 = read_cr4();
>  
> -    if ( read_cr4() & X86_CR4_FSGSBASE )
> +    if ( !(cr4 & X86_CR4_FRED) && (cr4 & X86_CR4_FSGSBASE) )
>      {
>          asm volatile ( "swapgs" );
>          base = __rdgsbase();
> @@ -234,7 +234,9 @@ static inline void write_gs_base(unsigned long base)
>  
>  static inline void write_gs_shadow(unsigned long base)
>  {
> -    if ( read_cr4() & X86_CR4_FSGSBASE )
> +    unsigned long cr4 = read_cr4();
> +
> +    if ( !(cr4 & X86_CR4_FRED) && (cr4 & X86_CR4_FSGSBASE) )
>      {
>          asm volatile ( "swapgs\n\t"
>                         "wrgsbase %0\n\t"
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 270b93ed623e..e67a428e4362 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1013,6 +1013,32 @@ void show_execution_state_nmi(const cpumask_t *mask, bool show_all)
>          printk("Non-responding CPUs: {%*pbl}\n", CPUMASK_PR(&show_state_mask));
>  }
>  
> +static const char *x86_et_name(unsigned int type)
> +{
> +    static const char *const names[] = {
> +        [X86_ET_EXT_INTR]    = "EXT_INTR",
> +        [X86_ET_NMI]         = "NMI",
> +        [X86_ET_HW_EXC]      = "HW_EXC",
> +        [X86_ET_SW_INT]      = "SW_INT",
> +        [X86_ET_PRIV_SW_EXC] = "PRIV_SW_EXEC",
> +        [X86_ET_SW_EXC]      = "SW_EXEC",
> +        [X86_ET_OTHER]       = "OTHER",
> +    };
> +
> +    return (type < ARRAY_SIZE(names) && names[type]) ? names[type] : "???";
> +}
> +
> +static const char *x86_et_other_name(unsigned int vec)
> +{
> +    static const char *const names[] = {
> +        [0] = "MTF",
> +        [1] = "SYSCALL",
> +        [2] = "SYSENTER",
> +    };
> +
> +    return (vec < ARRAY_SIZE(names) && names[vec][0]) ? names[vec] : "???";
> +}
> +
>  const char *vector_name(unsigned int vec)
>  {
>      static const char names[][4] = {
> @@ -1091,9 +1117,42 @@ void fatal_trap(const struct cpu_user_regs *regs, bool show_remote)
>          }
>      }
>  
> -    panic("FATAL TRAP: vec %u, %s[%04x]%s\n",
> -          trapnr, vector_name(trapnr), regs->error_code,
> -          (regs->eflags & X86_EFLAGS_IF) ? "" : " IN INTERRUPT CONTEXT");
> +    if ( read_cr4() & X86_CR4_FRED )
> +    {
> +        bool render_ec = false;
> +        const char *vec_name = NULL;
> +
> +        switch ( regs->fred_ss.type )
> +        {
> +        case X86_ET_HW_EXC:
> +        case X86_ET_SW_INT:
> +        case X86_ET_PRIV_SW_EXC:
> +        case X86_ET_SW_EXC:
> +            render_ec = true;
> +            vec_name = vector_name(regs->fred_ss.vector);
> +            break;
> +
> +        case X86_ET_OTHER:
> +            vec_name = x86_et_other_name(regs->fred_ss.vector);
> +            break;
> +        }
> +
> +        if ( render_ec )
> +            panic("Fatal TRAP: type %u, %s, vec %u, %s[%04x]%s\n",
> +                  regs->fred_ss.type, x86_et_name(regs->fred_ss.type),
> +                  regs->fred_ss.vector, vec_name ?: "",
> +                  regs->error_code,
> +                  (regs->eflags & X86_EFLAGS_IF) ? "" : " IN INTERRUPT CONTEXT");
> +        else
> +            panic("Fatal TRAP: type %u, %s, vec %u, %s%s\n",
> +                  regs->fred_ss.type, x86_et_name(regs->fred_ss.type),
> +                  regs->fred_ss.vector, vec_name ?: "",
> +                  (regs->eflags & X86_EFLAGS_IF) ? "" : " IN INTERRUPT CONTEXT");
> +    }
> +    else
> +        panic("FATAL TRAP: vec %u, %s[%04x]%s\n",
> +              trapnr, vector_name(trapnr), regs->error_code,
> +              (regs->eflags & X86_EFLAGS_IF) ? "" : " IN INTERRUPT CONTEXT");
>  }
>  
>  void asmlinkage noreturn do_unhandled_trap(struct cpu_user_regs *regs)
> @@ -2181,6 +2240,94 @@ void asmlinkage check_ist_exit(const struct cpu_user_regs *regs, bool ist_exit)
>  }
>  #endif
>  
> +void asmlinkage entry_from_pv(struct cpu_user_regs *regs)
> +{
> +    /* Copy fred_ss.vector into entry_vector as IDT delivery would have done. */
> +    regs->entry_vector = regs->fred_ss.vector;
> +
> +    switch ( regs->fred_ss.type )
> +    {
> +    case X86_ET_EXT_INTR:
> +        do_IRQ(regs);
> +        break;
> +
> +    case X86_ET_NMI:
> +        do_nmi(regs);
> +        break;
> +
> +    case X86_ET_HW_EXC:
> +    case X86_ET_SW_INT:
> +    case X86_ET_PRIV_SW_EXC:
> +    case X86_ET_SW_EXC:
> +        goto fatal;
> +
> +    default:
> +        goto fatal;
> +    }
> +
> +    return;
> +
> + fatal:
> +    fatal_trap(regs, false);
> +}

Noticed only now: Shouldn't this be surrounded with #ifdef CONFIG_PV (with
knock-on effects elsewhere)?

Jan