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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2025 22:40, Andrew Cooper wrote:
> On 14/08/2025 4:57 pm, Jan Beulich wrote:
>> On 08.08.2025 22:23, Andrew Cooper wrote:
>>> --- 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.
> 
> Without the change, read_registers() suffers #UD because of the SWAPGS.
> 
> This recurses until hitting the guard page, then repeats the same on the
> #DF stack.  And because stacks work nicely under FRED, you eventually
> hit #DF's guard page.
> 
> Strictly speaking it's only read_gs_shadow() which we need to change to
> make exception handling work, but I fixed both at the same time.
> 
> That said, I have actually cleaned this codepath up with the MSR work
> because the code gen in read_registers() is terrible.  Due to
> no-strict-aliasing, every store into state-> forces a recalculation of
> get_cpu_info(), meaning that read_cr4() cannot be hoisted, and there's a
> branch in every helper.
> 
> I'm still not sure how best to fit it into this series.

Could these two hunks move to another prereq patch, then coming with its
own description?

>>> --- 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?
> 
> Well - you are decoding the field name regs->fred_ss.vector.

Hmm, yes, the field is re-used, but I'm in trouble viewing these as vectors.
Anyway - I would prefer a rename, but I won't insist.

>>> --- 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?
> 
> Perhaps, but that is quite subtle.  I did also consider a
> .text.entry.page_aligned section, but .text.entry only matters for XPTI
> which (as agreed), I'm not intending to implement in FRED mode unless it
> proves to be necessary.
> 
> Also IIRC there's still a symbol bug where _sentrytext takes priority
> over entry_FRED_R3, so the backtrace is effectively wrong.
> 
> (These are all bad excuses, but some parts of this series are rather old.)

Are you sure this is still the case with entry_FRED_R<n> properly typed as
functions (while _stextentry has no type)? When choosing which symbol to
display, objdump prefers typed over type-less symbols:

  /* Sort function and object symbols before global symbols before
     local symbols before section symbols before debugging symbols.  */

>>  Also, might it be more natural to use PAGE_SIZE
>> here?
> 
> I did debate that, but the spec uses 0xfff, not pages, even if the
> pipline surely does have an optimisation for chopping 12 metadata bits
> off the bottom of a pointer.

Plus pretty certainly a page boundary is meant here, no matter how things
are worded.

Jan