| Message ID | 20250225222905.1182374-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Cmdline fixes/improvements | expand |
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@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@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. Jan
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@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@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. ~Andrew
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.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@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 <jbeulich@suse.com> Patch 1 may still need a little bit of tweaking, though. Jan
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 ]` Applicability: x86 - Default: `scan` is selectable via Kconfig, `nmi=true` + Default: `scan` is selectable via Kconfig Controls for CPU microcode loading. @@ -2758,10 +2758,6 @@ When booting xen.efi natively, the concept of multiboot modules doesn't exist. Instead, in the [EFI configuration file](efi.html), `ucode=<filename>` can be used to identify a file as a raw container (option 1 above). -'nmi' determines late loading is performed in NMI handler or just in -stop_machine context. In NMI handler, even NMIs are blocked, which is -considered safer. The default value is `true`. - ### unrestricted_guest (Intel) > `= <boolean>` diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index c8e14ed9b10c..4898920b9c52 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -82,9 +82,6 @@ struct patch_with_flags { const struct microcode_patch *patch; }; -/* By default, ucode loading is done in NMI handler */ -static bool ucode_in_nmi = true; - /* Protected by microcode_mutex */ static struct microcode_patch *microcode_cache; @@ -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 @@ -297,12 +292,10 @@ static int cf_check microcode_nmi_callback( return 0; /* - * Primary threads load ucode in NMI handler on if ucode_in_nmi is true. - * Secondary threads are expected to stay in NMI handler regardless of - * ucode_in_nmi. + * The BSP doesn't enter NMI context for microcode loading, as it's the + * entity organising the rendezvous. */ - if ( cpu == cpumask_first(&cpu_online_map) || - (!ucode_in_nmi && primary_cpu) ) + if ( cpu == cpumask_first(&cpu_online_map) ) return 0; if ( primary_cpu ) @@ -343,22 +336,17 @@ static int primary_thread_fn(const struct microcode_patch *patch, if ( !wait_for_state(LOADING_CALLIN) ) return -EBUSY; - if ( ucode_in_nmi ) - { - self_nmi(); - - /* - * Wait for ucode loading is done in case that the NMI does not arrive - * synchronously, which may lead to a not-yet-updated error is returned - * below. - */ - if ( unlikely(!wait_for_state(LOADING_EXIT)) ) - ASSERT_UNREACHABLE(); + self_nmi(); - return this_cpu(loading_err); - } + /* + * Wait for ucode loading is done in case that the NMI does not arrive + * synchronously, which may lead to a not-yet-updated error is returned + * below. + */ + if ( unlikely(!wait_for_state(LOADING_EXIT)) ) + ASSERT_UNREACHABLE(); - return primary_thread_work(patch, flags); + return this_cpu(loading_err); } static int control_thread_fn(const struct microcode_patch *patch,
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@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(-)