On 15/08/2025 9:37 am, Jan Beulich wrote:
> On 14.08.2025 21:16, Andrew Cooper wrote:
>> 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.
> I guess I'm not following: If it was -1, and if the code here was
>
> if ( opt_fred < 0 )
> opt_fred = 0 /* !pv_shim */;
>
> why would things "fail spectacularly" unless someone passed "fred" on
> the command line?
Oh, that would work, but why bother? It's simply a less readable form
of mine, and if we're going to nitpick, it's commented out code.
~Andrew
