On 26.02.2025 20:00, Andrew Cooper wrote:
> 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.coop...@citrix.com>
>>> ---
>>> CC: Jan Beulich <jbeul...@suse.com>
>>> CC: Roger Pau Monné <roger....@citrix.com>
>>>
>>> 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.
Right, and hence for the patch here
Reviewed-by: Jan Beulich <jbeul...@suse.com>
Patch 1 may still need a little bit of tweaking, though.
Jan
