[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 27 Feb 2025 08:42:00 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 27 Feb 2025 07:42:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.02.2025 19:45, Andrew Cooper wrote:
> 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.

Hmm, while I don't like this, I'll accept it.

>>>      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.

Is it? In the config file only one "ramdisk=" is permitted per section. (Or to
be more precise, subsequent ones in the same section simply will be ignored.
Like is the case for other similar settings, like "kernel=".)

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.