[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] x86/ucode: Adjust parse_ucode() to match other list handling
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |