Under FRED, the SWAPGS instructions is disallowed.  Therefore we must use the
MSR path instead.

read_registers() is in the show_registers() path, so this allows Xen to render
it's current state without suffering #UD (and recursing until the stack guard
page is hit).

All hardware with FRED is expected to have some kind of non-serialising access
to these registers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Broken out of subsequent patch.  Rebased over MSR cleanup.

In principle, the following can also be used for read_registers()

    diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
    index 5799770a2f71..0b0fdf2c5ac4 100644
    --- a/xen/arch/x86/traps.c
    +++ b/xen/arch/x86/traps.c
    @@ -125,16 +125,21 @@ static void read_registers(struct extra_state *state)
         state->cr3 = read_cr3();
         state->cr4 = read_cr4();

    -    if ( !(state->cr4 & X86_CR4_FRED) && (state->cr4 & X86_CR4_FSGSBASE) )
    +    if ( state->cr4 & X86_CR4_FSGSBASE )
         {
             state->fsb = __rdfsbase();
             state->gsb = __rdgsbase();
    +
    +        if ( state->cr4 & X86_CR4_FRED )
    +            goto gskern_fred;
    +
             state->gss = __rdgskern();
         }
         else
         {
             state->fsb = rdmsr(MSR_FS_BASE);
             state->gsb = rdmsr(MSR_GS_BASE);
    +    gskern_fred:
             state->gss = rdmsr(MSR_SHADOW_GS_BASE);
         }

but I'm not sure that it's a good enough improvement to warrant the
complexity.
---
 xen/arch/x86/include/asm/fsgsbase.h | 8 ++++++--
 xen/arch/x86/traps.c                | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/fsgsbase.h b/xen/arch/x86/include/asm/fsgsbase.h
index 24862a6bfea7..5faa3a324332 100644
--- a/xen/arch/x86/include/asm/fsgsbase.h
+++ b/xen/arch/x86/include/asm/fsgsbase.h
@@ -79,7 +79,9 @@ static inline unsigned long read_gs_base(void)
 
 static inline unsigned long read_gs_shadow(void)
 {
-    if ( read_cr4() & X86_CR4_FSGSBASE )
+    unsigned long cr4 = read_cr4();
+
+    if ( !(cr4 & X86_CR4_FRED) && (cr4 & X86_CR4_FSGSBASE) )
         return __rdgs_shadow();
     else
         return rdmsr(MSR_SHADOW_GS_BASE);
@@ -103,7 +105,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) )
         __wrgs_shadow(base);
     else
         wrmsrns(MSR_SHADOW_GS_BASE, base);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c11d72d47027..66308e7c9edf 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -118,7 +118,7 @@ static void read_registers(struct extra_state *state)
     state->cr3 = read_cr3();
     state->cr4 = read_cr4();
 
-    if ( state->cr4 & X86_CR4_FSGSBASE )
+    if ( !(state->cr4 & X86_CR4_FRED) && (state->cr4 & X86_CR4_FSGSBASE) )
     {
         state->fsb = __rdfsbase();
         state->gsb = __rdgsbase();
-- 
2.39.5

On 28.08.2025 17:03, Andrew Cooper wrote:
> In principle, the following can also be used for read_registers()
> 
>     diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>     index 5799770a2f71..0b0fdf2c5ac4 100644
>     --- a/xen/arch/x86/traps.c
>     +++ b/xen/arch/x86/traps.c
>     @@ -125,16 +125,21 @@ static void read_registers(struct extra_state *state)
>          state->cr3 = read_cr3();
>          state->cr4 = read_cr4();
> 
>     -    if ( !(state->cr4 & X86_CR4_FRED) && (state->cr4 & X86_CR4_FSGSBASE) )
>     +    if ( state->cr4 & X86_CR4_FSGSBASE )
>          {
>              state->fsb = __rdfsbase();
>              state->gsb = __rdgsbase();
>     +
>     +        if ( state->cr4 & X86_CR4_FRED )
>     +            goto gskern_fred;
>     +
>              state->gss = __rdgskern();

I'm irritated by this patch context here vs ...

> --- a/xen/arch/x86/include/asm/fsgsbase.h
> +++ b/xen/arch/x86/include/asm/fsgsbase.h
> @@ -79,7 +79,9 @@ static inline unsigned long read_gs_base(void)
>  
>  static inline unsigned long read_gs_shadow(void)
>  {
> -    if ( read_cr4() & X86_CR4_FSGSBASE )
> +    unsigned long cr4 = read_cr4();
> +
> +    if ( !(cr4 & X86_CR4_FRED) && (cr4 & X86_CR4_FSGSBASE) )
>          return __rdgs_shadow();

... the one here. Was the former (and the subject of the patch) not updated
yet (kern => shadow)? On the assumption that that's what has happened, and
hence preferably with the subject also adjusted
Reviewed-by: Jan Beulich <jbeulich@suse.com>

As to the alternative, I in particular don't overly like the goto there. I
would consider that an option only if in turn a simplification elsewhere
resulted.

Jan