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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/12/2025 7:55 am, Jan Beulich wrote:
> 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.

The documentation needs to describe how it is, not how one wants it to be.

Anyway, this is long overdue (and needs another rebase over Alejandro's
recent work), so I'm going to get it committed.

~Andrew