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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.12.2025 18:08, Andrew Cooper wrote:
> 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?

I'm happy with mutually exclusive. What I said I don't like is the dropping
of the |, expressing the same "mutually exclusive" in a non-verbal way. Imo
those short forms aren't supposed to describe how parsing works, but how the
options are intended to be used.

Jan