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 <[email protected]> >> --- >> CC: Jan Beulich <[email protected]> >> CC: Roger Pau Monné <[email protected]> >> >> 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
