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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2025 5:00 pm, Jan Beulich wrote:
> On 15.12.2025 16:32, Andrew Cooper wrote:
>> 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@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit I'm not quite happy with ...
>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2752,34 +2752,52 @@ performance.
>>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own 
>> risk.
>>  
>>  ### ucode
>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>> +> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
> ... this change when ...
>
>>      Applicability: x86
>>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>>  
>> -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=<bool>` options are mutually exclusive and select
>> +between these two options.  Further restrictions exist for booting xen.efi
>> +(see below).
> ... then you say verbally that the two are exclusive of one another. That's
> what the | there was intended to indicate. IOW I would prefer that line to
> be left alone, but I'm not intending to insist.

You said that last time around, but it's still not how the parsing works.

ucode=1,1,1,scan,scan,scan,2 is legal.  The latest takes priority and
cancels prior selections.

The reality is that | used in this context is meaningless when there's a
comma separated loop around the whole thing.

If you don't like "mutually exclusive", what else do you suggest?

~Andrew