Re: [PATCH 2/2] x86/ucode: Drop the ucode=nmi option

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/02/2025 2:46 pm, Jan Beulich wrote:
> On 25.02.2025 23:29, Andrew Cooper wrote:
>> This option is active by default, and despite what the documentation suggests
>> about choosing ucode=no-nmi, it only controls whether the primary threads 
>> move
>> into NMI context.
>>
>> Sibling threads unconditionally move into NMI context, which is confirmed by
>> an in-code comment.
>>
>> Not discussed is the fact that the BSP never enters NMI context, which works
>> only by luck (AMD CPUs, where we reload on sibling threads too, has working
>> in-core rendezvous and doesn't require NMI cover on the primary thread for
>> safety).  This does want addressing, but requires more untangling first.
>>
>> Overall, `ucode=no-nmi` is a misleading and pretty useless option.  Drop it,
>> and simplify the two users.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Despite the reasonably large diff in primary_thread_fn(), it's mostly just
>> unindenting the block, and dropping the final call to primary_thread_work()
>> which is made dead by this change.
>> ---
>>  docs/misc/xen-command-line.pandoc |  8 ++-----
>>  xen/arch/x86/cpu/microcode/core.c | 38 +++++++++++--------------------
>>  2 files changed, 15 insertions(+), 31 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.pandoc 
>> b/docs/misc/xen-command-line.pandoc
>> index 47674025249a..9702c36b8c39 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2721,10 +2721,10 @@ performance.
>>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own 
>> risk.
>>  
>>  ### ucode
>> -> `= List of [ <integer>, scan=<bool>, nmi=<bool> ]`
>> +> `= List of [ <integer>, scan=<bool ]`
> With this (taking my comment on patch 1 into account as well) I think ...
>
>> @@ -123,9 +120,7 @@ static int __init cf_check parse_ucode(const char *s)
>>          if ( !ss )
>>              ss = strchr(s, '\0');
>>  
>> -        if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
>> -            ucode_in_nmi = val;
>> -        else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
>> +        if ( (val = parse_boolean("scan", s, ss)) >= 0 )
>>          {
>>              if ( ucode_mod_forced )
>>                  printk(XENLOG_WARNING
> ... this function will want to transition back to what it was prior to
> the addition of the sub-option, preferably adjusted to account for the
> possibility of multiple "ucode=" on the command line, i.e. along the
> lines of
>
>     if ( !strncmp(s, "scan", 4) )
>     {
>         ucode_scan = 1;
>         ucode_mod_idx = 0;
>     }
>     else
>     {
>         ucode_scan = 0;
>         ucode_mod_idx = simple_strtol(s, &q, 0);
>     }
>
> That would then make patch 1 kind of unnecessary.

As said, I need to introduce a new option for the AMD fix, so it needs
to stay comma-separated.

~Andrew