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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2025 22:23, Andrew Cooper wrote:
> --- 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

Hmm, yes, differences are apparently large enough to warrant the redundancy
with SAVE_ALL / RESTORE_ALL.

> --- 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"

I don't quite get how these changes fit into this patch.

> --- 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)

This isn't really a vector, is it?

> +{
> +    static const char *const names[] = {
> +        [0] = "MTF",
> +        [1] = "SYSCALL",
> +        [2] = "SYSENTER",
> +    };
> +
> +    return (vec < ARRAY_SIZE(names) && names[vec][0]) ? names[vec] : "???";

Did you mean to check names[ves] for being NULL? Or is this a leftover
from the array being something like names[][10]?

> --- a/xen/arch/x86/x86_64/Makefile
> +++ b/xen/arch/x86/x86_64/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_PV32) += compat/
>  
>  obj-bin-y += entry.o
> +obj-bin-y += entry-fred.o

For the ordering here, ...

> --- /dev/null
> +++ b/xen/arch/x86/x86_64/entry-fred.S
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +        .file "x86_64/entry-fred.S"
> +
> +#include <asm/asm_defns.h>
> +#include <asm/page.h>
> +
> +        .section .text.entry, "ax", @progbits
> +
> +        /* The Ring3 entry point is required to be 4k aligned. */
> +
> +FUNC(entry_FRED_R3, 4096)

... doesn't this 4k-alignment requirement suggest we want to put
entry-fred.o first? Also, might it be more natural to use PAGE_SIZE
here?

Jan