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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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