| Message ID | 20250225222905.1182374-2-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: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2721,34 +2721,42 @@ 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>, nmi=<bool> ]` While this makes doc fit present behavior, it is the behavior that is wrong. It was - afaict - broken by 5ed12565aa32 ("microcode: rendezvous CPUs in NMI handler and load ucode"). There should not be both an integer and "scan=". (Really we should have taken measures to stay consistent even if multiple "ucode=" were on the command line.) You actually say so ... > Applicability: x86 > Default: `scan` is selectable via Kconfig, `nmi=true` > > -Controls for CPU microcode loading. For early loading, this parameter can > -specify how and where to find the microcode update blob. For late loading, > -this parameter specifies if the update happens within a NMI handler. > - > -'integer' specifies the CPU microcode update blob module index. When positive, > -this specifies the n-th module (in the GrUB entry, zero based) to be used > -for updating CPU micrcode. When negative, counting starts at the end of > -the modules in the GrUB entry (so with the blob commonly being last, > -one could specify `ucode=-1`). Note that the value of zero is not valid > -here (entry zero, i.e. the first module, is always the Dom0 kernel > -image). Note further that use of this option has an unspecified effect > -when used with xen.efi (there the concept of modules doesn't exist, and > -the blob gets specified via the `ucode=<filename>` config file/section > -entry; see [EFI configuration file description](efi.html)). > - > -'scan' instructs the hypervisor to scan the multiboot images for an cpio > -image that contains microcode. Depending on the platform the blob with the > -microcode in the cpio name space must be: > - - on Intel: kernel/x86/microcode/GenuineIntel.bin > - - on AMD : kernel/x86/microcode/AuthenticAMD.bin > -When using xen.efi, the `ucode=<filename>` config file setting takes > -precedence over `scan`. The default value for `scan` is set with > -`CONFIG_UCODE_SCAN_DEFAULT`. > +Controls for CPU microcode loading. > + > +In order to load microcode at boot, Xen needs to find a suitable update > +amongst the modules provided by the bootloader. Two kinds of microcode update > +are supported: > + > + 1. Raw microcode containers. The format of the container is CPU vendor > + specific. > + > + 2. CPIO archive. This is Linux's preferred mechanism, and involves having > + the raw containers expressed as files > + (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO > + archive, typically prepended to the initrd. > + > +The `<integer>` and `scan=` options are mutually exclusive and select between > +these two options. They are also invalid in EFI boots (see below). ... here. As to EFI boots: "scan=" ought to be usable there, as long as no "ucode=" was in the xen.efi config file. I think your code is correct in this regard, it's just the wording here which is too strict. > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -113,11 +113,6 @@ void __init microcode_set_module(unsigned int idx) > ucode_mod_forced = 1; > } > > -/* > - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. While personally I don't mind the removal, I think back at the time it was specifically asked to (also) put it here. Jan
On 26/02/2025 2:30 pm, Jan Beulich wrote: > On 25.02.2025 23:29, Andrew Cooper wrote: >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2721,34 +2721,42 @@ 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>, nmi=<bool> ]` > While this makes doc fit present behavior, it is the behavior that is wrong. > It was - afaict - broken by 5ed12565aa32 ("microcode: rendezvous CPUs in NMI > handler and load ucode"). There should not be both an integer and "scan=". > (Really we should have taken measures to stay consistent even if multiple > "ucode=" were on the command line.) You actually say so ... Yes that changed commit changed the behaviour. We discussed it during c25c964634b1 ("x86/ucode: Enforce invariant about module selection"). "Wrong" is more complicated. Neither behaviour is great, but we need regular comma separated operations. (I know I'm deleting nmi= in the next patch but I need to introduce a new one for the AMD fix). In the presence of comma separated options, one part being `<integer> | scan=<bool>` is pointless, because `ucode=1,1,1` is valid, as is `ucode=scan,scan,scan`, and then all you've got is an overly complicated description of what is identical to other regular list operations. For this corner case, it's simply easier to tell the user "don't do that", which is what we say elsewhere too. > >> Applicability: x86 >> Default: `scan` is selectable via Kconfig, `nmi=true` >> >> -Controls for CPU microcode loading. For early loading, this parameter can >> -specify how and where to find the microcode update blob. For late loading, >> -this parameter specifies if the update happens within a NMI handler. >> - >> -'integer' specifies the CPU microcode update blob module index. When positive, >> -this specifies the n-th module (in the GrUB entry, zero based) to be used >> -for updating CPU micrcode. When negative, counting starts at the end of >> -the modules in the GrUB entry (so with the blob commonly being last, >> -one could specify `ucode=-1`). Note that the value of zero is not valid >> -here (entry zero, i.e. the first module, is always the Dom0 kernel >> -image). Note further that use of this option has an unspecified effect >> -when used with xen.efi (there the concept of modules doesn't exist, and >> -the blob gets specified via the `ucode=<filename>` config file/section >> -entry; see [EFI configuration file description](efi.html)). >> - >> -'scan' instructs the hypervisor to scan the multiboot images for an cpio >> -image that contains microcode. Depending on the platform the blob with the >> -microcode in the cpio name space must be: >> - - on Intel: kernel/x86/microcode/GenuineIntel.bin >> - - on AMD : kernel/x86/microcode/AuthenticAMD.bin >> -When using xen.efi, the `ucode=<filename>` config file setting takes >> -precedence over `scan`. The default value for `scan` is set with >> -`CONFIG_UCODE_SCAN_DEFAULT`. >> +Controls for CPU microcode loading. >> + >> +In order to load microcode at boot, Xen needs to find a suitable update >> +amongst the modules provided by the bootloader. Two kinds of microcode update >> +are supported: >> + >> + 1. Raw microcode containers. The format of the container is CPU vendor >> + specific. >> + >> + 2. CPIO archive. This is Linux's preferred mechanism, and involves having >> + the raw containers expressed as files >> + (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO >> + archive, typically prepended to the initrd. >> + >> +The `<integer>` and `scan=` options are mutually exclusive and select between >> +these two options. They are also invalid in EFI boots (see below). > ... here. > > As to EFI boots: "scan=" ought to be usable there, as long as no "ucode=" > was in the xen.efi config file. I think your code is correct in this regard, > it's just the wording here which is too strict. There's still a sharp corner trying that, which is why I didn't address it. With EFI, there's no order of modules, so `scan` is still ambiguous if you've got multiple CPIO archives. > >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -113,11 +113,6 @@ void __init microcode_set_module(unsigned int idx) >> ucode_mod_forced = 1; >> } >> >> -/* >> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. > While personally I don't mind the removal, I think back at the time it was > specifically asked to (also) put it here. It was stale for the whole time of allow-same=<bool>. This patch was originally part of the post-"--force" cleanup series, but I deferred it because of the unanticipated fixes required for the boot module changes. ~Andrew
On 26.02.2025 19:45, Andrew Cooper wrote: > On 26/02/2025 2:30 pm, Jan Beulich wrote: >> On 25.02.2025 23:29, Andrew Cooper wrote: >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -2721,34 +2721,42 @@ 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>, nmi=<bool> ]` >> While this makes doc fit present behavior, it is the behavior that is wrong. >> It was - afaict - broken by 5ed12565aa32 ("microcode: rendezvous CPUs in NMI >> handler and load ucode"). There should not be both an integer and "scan=". >> (Really we should have taken measures to stay consistent even if multiple >> "ucode=" were on the command line.) You actually say so ... > > Yes that changed commit changed the behaviour. We discussed it during > c25c964634b1 ("x86/ucode: Enforce invariant about module selection"). > > "Wrong" is more complicated. Neither behaviour is great, but we need > regular comma separated operations. (I know I'm deleting nmi= in the > next patch but I need to introduce a new one for the AMD fix). > > In the presence of comma separated options, one part being `<integer> | > scan=<bool>` is pointless, because `ucode=1,1,1` is valid, as is > `ucode=scan,scan,scan`, and then all you've got is an overly complicated > description of what is identical to other regular list operations. > > For this corner case, it's simply easier to tell the user "don't do > that", which is what we say elsewhere too. Hmm, while I don't like this, I'll accept it. >>> Applicability: x86 >>> Default: `scan` is selectable via Kconfig, `nmi=true` >>> >>> -Controls for CPU microcode loading. For early loading, this parameter can >>> -specify how and where to find the microcode update blob. For late loading, >>> -this parameter specifies if the update happens within a NMI handler. >>> - >>> -'integer' specifies the CPU microcode update blob module index. When positive, >>> -this specifies the n-th module (in the GrUB entry, zero based) to be used >>> -for updating CPU micrcode. When negative, counting starts at the end of >>> -the modules in the GrUB entry (so with the blob commonly being last, >>> -one could specify `ucode=-1`). Note that the value of zero is not valid >>> -here (entry zero, i.e. the first module, is always the Dom0 kernel >>> -image). Note further that use of this option has an unspecified effect >>> -when used with xen.efi (there the concept of modules doesn't exist, and >>> -the blob gets specified via the `ucode=<filename>` config file/section >>> -entry; see [EFI configuration file description](efi.html)). >>> - >>> -'scan' instructs the hypervisor to scan the multiboot images for an cpio >>> -image that contains microcode. Depending on the platform the blob with the >>> -microcode in the cpio name space must be: >>> - - on Intel: kernel/x86/microcode/GenuineIntel.bin >>> - - on AMD : kernel/x86/microcode/AuthenticAMD.bin >>> -When using xen.efi, the `ucode=<filename>` config file setting takes >>> -precedence over `scan`. The default value for `scan` is set with >>> -`CONFIG_UCODE_SCAN_DEFAULT`. >>> +Controls for CPU microcode loading. >>> + >>> +In order to load microcode at boot, Xen needs to find a suitable update >>> +amongst the modules provided by the bootloader. Two kinds of microcode update >>> +are supported: >>> + >>> + 1. Raw microcode containers. The format of the container is CPU vendor >>> + specific. >>> + >>> + 2. CPIO archive. This is Linux's preferred mechanism, and involves having >>> + the raw containers expressed as files >>> + (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO >>> + archive, typically prepended to the initrd. >>> + >>> +The `<integer>` and `scan=` options are mutually exclusive and select between >>> +these two options. They are also invalid in EFI boots (see below). >> ... here. >> >> As to EFI boots: "scan=" ought to be usable there, as long as no "ucode=" >> was in the xen.efi config file. I think your code is correct in this regard, >> it's just the wording here which is too strict. > > There's still a sharp corner trying that, which is why I didn't address it. > > With EFI, there's no order of modules, so `scan` is still ambiguous if > you've got multiple CPIO archives. Is it? In the config file only one "ramdisk=" is permitted per section. (Or to be more precise, subsequent ones in the same section simply will be ignored. Like is the case for other similar settings, like "kernel=".) Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 0c6225391d55..47674025249a 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2721,34 +2721,42 @@ 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>, nmi=<bool> ]` Applicability: x86 Default: `scan` is selectable via Kconfig, `nmi=true` -Controls for CPU microcode loading. For early loading, this parameter can -specify how and where to find the microcode update blob. For late loading, -this parameter specifies if the update happens within a NMI handler. - -'integer' specifies the CPU microcode update blob module index. When positive, -this specifies the n-th module (in the GrUB entry, zero based) to be used -for updating CPU micrcode. When negative, counting starts at the end of -the modules in the GrUB entry (so with the blob commonly being last, -one could specify `ucode=-1`). Note that the value of zero is not valid -here (entry zero, i.e. the first module, is always the Dom0 kernel -image). Note further that use of this option has an unspecified effect -when used with xen.efi (there the concept of modules doesn't exist, and -the blob gets specified via the `ucode=<filename>` config file/section -entry; see [EFI configuration file description](efi.html)). - -'scan' instructs the hypervisor to scan the multiboot images for an cpio -image that contains microcode. Depending on the platform the blob with the -microcode in the cpio name space must be: - - on Intel: kernel/x86/microcode/GenuineIntel.bin - - on AMD : kernel/x86/microcode/AuthenticAMD.bin -When using xen.efi, the `ucode=<filename>` config file setting takes -precedence over `scan`. The default value for `scan` is set with -`CONFIG_UCODE_SCAN_DEFAULT`. +Controls for CPU microcode loading. + +In order to load microcode at boot, Xen needs to find a suitable update +amongst the modules provided by the bootloader. Two kinds of microcode update +are supported: + + 1. Raw microcode containers. The format of the container is CPU vendor + specific. + + 2. CPIO archive. This is Linux's preferred mechanism, and involves having + the raw containers expressed as files + (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO + archive, typically prepended to the initrd. + +The `<integer>` and `scan=` options are mutually exclusive and select between +these two options. They are also invalid in EFI boots (see below). + + * The `<integer>` option nominates a specific multiboot module as a raw + container (option 1 above). Valid options start from 1 (module 0 is + always the dom0 kernel). A negative number may be used, and will + back-reference from the end of the module list. i.e. `ucode=-1` will + nominate the final multiboot module. + + * The `scan=` option causes Xen to search all modules in order to find the + first CPIO archive containing the appropriate file (option 2 above). The + default for this option can be chosen at build time via + `CONFIG_UCODE_SCAN_DEFAULT`. + +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 diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index de00c22b4bd6..c8e14ed9b10c 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -113,11 +113,6 @@ void __init microcode_set_module(unsigned int idx) ucode_mod_forced = 1; } -/* - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are - * optional. If the EFI has forced which of the multiboot payloads is to be - * used, only nmi=<bool> is parsed. - */ static int __init cf_check parse_ucode(const char *s) { const char *ss; @@ -130,13 +125,24 @@ static int __init cf_check parse_ucode(const char *s) if ( (val = parse_boolean("nmi", s, ss)) >= 0 ) ucode_in_nmi = val; - else if ( !ucode_mod_forced ) /* Not forced by EFI */ + else if ( (val = parse_boolean("scan", s, ss)) >= 0 ) { - if ( (val = parse_boolean("scan", s, ss)) >= 0 ) + if ( ucode_mod_forced ) + printk(XENLOG_WARNING + "Ignoring ucode=%.*s setting; overridden by EFI\n", + (int)(ss - s), s); + else { opt_scan = val; opt_mod_idx = 0; } + } + else if ( isdigit(s[0]) || s[0] == '-' ) + { + if ( ucode_mod_forced ) + printk(XENLOG_WARNING + "Ignoring ucode=%.*s setting; overridden by EFI\n", + (int)(ss - s), s); else { const char *q; @@ -151,6 +157,8 @@ static int __init cf_check parse_ucode(const char *s) opt_scan = false; } } + else + rc = -EINVAL; s = ss + 1; } while ( *ss );
parse_ucode() is abnormal compared to similar parsing elsewhere in Xen. Invert the ucode_mod_forced check with respect to the "scan" and integer handling, so we can warn the user when we've elected to ignore the parameters, and yield -EINVAL for any unrecognised list element. Rewrite the ucode= command line docs for clarity. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- docs/misc/xen-command-line.pandoc | 56 ++++++++++++++++++------------- xen/arch/x86/cpu/microcode/core.c | 22 ++++++++---- 2 files changed, 47 insertions(+), 31 deletions(-)