Re: [Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Eslam Elnikety <elnikety@amazon.com>]

Thanks for getting the other patches in the series onto master, Jan.

This is the only patch out of this series that did not make it through, 
so I keeping my comments here.

On 23.01.20 11:26, Jan Beulich wrote:
> On 22.01.2020 23:30, Eslam Elnikety wrote:
>> Decouple the microcode indexing mechanism when using GRUB to that
>> when using EFI. This allows us to avoid the "unspecified effect" of
>> using `<integer>` when booting via EFI.
> 
> Personally I'd prefer us to continue call this unspecified. It
> simply shouldn't be used. People shouldn't rely on it getting
> ignored. (Iirc, despite this being v1, you had previously
> submitted a patch to this effect, with me replaying about the
> same.)
> 
>> With that, Xen can explicitly ignore that option when using EFI.
> 
> Don't we do so already, by way of the "if ( !ucode_mod_forced )"
> you delete?
> 

Not quite. If cfg.efi does not specify "ucode=<filename>", then a 
`ucode=<integer>` from the commandline gets parsed, resulting in the 
"unspecified" behaviour. Here, the ucode_mod_idx will hold the <integer> 
which will be used as index into the modules prepared in whatever order 
the efi/boot.c defines.

In any case, let me know (and others too) if, later on, you may want to 
favor the explicitly ignore (opposed to unspecified) semantic and I will 
be happy to send another revision of this patch while addressing your 
comment below.

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2147,9 +2147,9 @@ 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
>> +image). This option should be used only with legacy boot, as it is explicitly
>> +ignored in EFI boot. When booting via EFI, the microcode update blob for
>> +early loading can be specified via the `ucode=<filename>` config file/section
>>   entry; see [EFI configuration file description](efi.html)).
> 
> Just like in patch 1, please distinguish "EFI boot" in general from
> "use of xen.efi" (relevant here of course only if indeed we went
> with this patch).
> 
> Jan
> 
You have mentioned this very same remark on the other patch too. My 
understanding is that "EFI boot" may be ambiguous between (a) we are 
actually booting from xen.efi or (b) a system with EFI support (and the 
latter may still be falling onto grub for booting). Let me know if 
that's not actually what your remark is about.

Cheers,
Eslam

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel