Re: [PATCH 15/22] x86/traps: Introduce opt_fred

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/08/2025 2:30 pm, Jan Beulich wrote:
> On 08.08.2025 22:23, Andrew Cooper wrote:
>> ... disabled by default.  There is a lot of work before FRED can be enabled by
>> default.
>>
>> One part of FRED, the LKGS (Load Kernel GS) instruction, is enumerated
>> separately but is mandatory as FRED disallows the SWAPGS instruction.
>> Therefore, both CPUID bits must be checked.
> See my (further) reply to patch 13 - I think FRED simply ought to depend on
> LKGS.
>
>> @@ -20,6 +22,9 @@ unsigned int __ro_after_init ler_msr;
>>  static bool __initdata opt_ler;
>>  boolean_param("ler", opt_ler);
>>  
>> +int8_t __ro_after_init opt_fred = 0; /* -1 when supported. */
> I'm a little puzzled by the comment? DYM "once default-enabled"?

Well, I have this temporary patch
https://gitlab.com/xen-project/hardware/xen-staging/-/commit/70ef6a1178a411a29b7b1745a1112e267ffb6245
that will turn into a real patch when we enable FRED by default.

As much as anything else, it was just a TODO.


>  Then ...
>
>> @@ -305,6 +310,32 @@ void __init traps_init(void)
>>      /* Replace early pagefault with real pagefault handler. */
>>      _update_gate_addr_lower(&bsp_idt[X86_EXC_PF], entry_PF);
>>  
>> +    if ( !cpu_has_fred || !cpu_has_lkgs )
>> +    {
>> +        if ( opt_fred )
> ... this won't work anymore once the initializer is changed.

Hmm yes.  That wants to be an == 1 check.  Fixed.

>
>> +            printk(XENLOG_WARNING "FRED not available, ignoring\n");
>> +        opt_fred = false;
> Better use 0 here?
>
>> +    }
>> +
>> +    if ( opt_fred == -1 )
>> +        opt_fred = !pv_shim;
> Imo it would be better to have the initializer be -1 right away, and comment
> out the "!pv_shim" here, until we mean it to be default-enabled.

It cannot be -1, or Xen will fail spectacularly on any FRED capable
hardware.  Setting to -1 is the point at which FRED becomes security
supported.

>
>> +    if ( opt_fred )
>> +    {
>> +#ifdef CONFIG_PV32
>> +        if ( opt_pv32 )
>> +        {
>> +            opt_pv32 = 0;
>> +            printk(XENLOG_INFO "Disabling PV32 due to FRED\n");
>> +        }
>> +#endif
>> +        printk("Using FRED event delivery\n");
>> +    }
>> +    else
>> +    {
>> +        printk("Using IDT event delivery\n");
>> +    }
> Could I talk you into omitting the figure braces here? Hmm, or perhaps you
> mean to later move code here.

Indeed, patch 22.

~Andrew