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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2025 23:52, Andrew Cooper wrote:
> 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.

Indeed, I was aware of Misra's dislike when writing the reply.

In any event - I'm okay with about any approach as long as the adjustment
to make (once FRED becomes supported) is both clear upfront and simple to
make (read: preferably a single line change). Readability is, as we know
from other recent instances, subjective. In the case here I think it
follows from my original comment that things weren't quite clear according
to my reading.

Jan