Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2019 22:08, Eslam Elnikety wrote:
> On 18.12.19 12:49, Jan Beulich wrote:
>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>> Decouple the microcode referencing mechanism when using GRUB to that
>>> when using EFI. This allows us to avoid the "unspecified effect" of
>>> using `<integer> | scan` along xen.efi.
>>
>> I guess "unspecified effect" in the doc was pretty pointless - such
>> options have been ignored before; in fact ...
>>
>>> With that, Xen can explicitly
>>> ignore those named options when using EFI.
>>
>> ... I don't see things becoming any more explicit (not even parsing
>> the options was quite explicit to me).
>>
> 
> I agree that those options have been ignored so far in the case of EFI. 
> The documentation contradicts that however. The documentation:
> * says <integer> has unspecified effect.
> * does not mention anything about scan being ignored.
> 
> With this patch, it is explicit in code and in documentation that both 
> options are ignored in case of EFI.

But isn't it rather that ucode=scan could (and hence perhaps should)
also have its value on EFI?

>>> As an added benefit,
>>> we get a straightfoward parsing of the ucode parameter.
>>
>> It's a single if() you eliminate - for me this doesn't make it
>> meaningfully more straightforward.
>>
> 
> As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the 
> ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> 
> ]" and it does not have to cater for corner cases. In either case, if 
> you strongly disagree with the wording, I can drop that sentence.

Well, what I don't like is giving the impression that things have been
worse than they actually are.

>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -2128,7 +2128,13 @@ logic applies:
>>>   ### ucode (x86)
>>>   > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
>>>   
>>> -Specify how and where to find CPU microcode update blob.
>>> +    Applicability: x86
>>> +    Default: `nmi`
>>> +
>>> +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 or in
>>> +a stop_machine context.
>>
>> It's always stop_machine context, isn't it? I also don't think this
>> implementation detail belongs here.
>>
> 
> Needs a better wording indeed. Let me know if you have particular 
> suggestions, and I will incorporate in v3.

Just drop everything from "or" onwards?

>>> @@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
>>>   
>>>   void __init microcode_set_module(unsigned int idx)
>>>   {
>>> -    ucode_mod_idx = idx;
>>> -    ucode_mod_forced = 1;
>>> +    ucode_mod_efi_idx = idx;
>>
>> Is it guaranteed (now and forever) that the index passed in is
>> non-zero? You changes to microcode_grab_module() imply so, but
>> just looking at the call site of the function I can't convince
>> myself this is the case. _If_ it is (thought to be) guaranteed,
>> then I think you at least want to ASSERT() here, perhaps with
>> a comment.
>>
> 
> For x86, it seems we have that guarantee (given that we must have a 
> dom0). Right?

For fully bringing up the system - yes. But there's no reason to
have a Dom0 if all you're after is testing early Xen boot. There'll
be a panic() in the case, but there shouldn't be anything breaking
proper behavior prior to this point.

>>>   }
>>>   
>>> -/*
>>> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are
>>> - * optional. If the EFI has forced which of the multiboot payloads is to be
>>> - * used, only nmi=<bool> is parsed.
>>> - */
>>> -static int __init parse_ucode(const char *s)
>>> +static int __init parse_ucode_param(const char *s)
>>
>> Any particular reason for the renaming? The function name was quite
>> fine imo.
>>
> 
> To me, "parse_ucode" is a misnomer.

Well, parse_"ucode= isn't a valid identifier. parse_ucode is what
results when stripping everything that makes it invalid. I can
see the ambiguity with parsing actual ucode, but the context here
makes it pretty clear what the function is about. Personally I'd
prefer such unnecessary renames to be avoided.

Jan

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