Re: [PATCH 1/2] x86/ucode: Adjust parse_ucode() to match other list handling

[Jan Beulich <jbeulich@xxxxxxxx>]

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