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

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

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.
> > 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.
> > --- 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.
> > --- a/xen/arch/x86/microcode.c
> > +++ b/xen/arch/x86/microcode.c
> > @@ -60,7 +60,7 @@
> >   
> >   static module_t __initdata ucode_mod;
> >   static signed int __initdata ucode_mod_idx;
> > -static bool_t __initdata ucode_mod_forced;
> > +static signed int __initdata ucode_mod_efi_idx;
> I don't see anything negative be put into here - should be unsigned
> int then.
Correct! The type just carried over from the coupling with 
ucode_mod_idx. Will adjust in v3.
> > @@ -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?
> >   }
> >   
> > -/*
> > - * 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.

Thanks for your comments, Jan.

-- Eslam

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