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

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

Picking this up again after the break. Apologies for the delay.

On 20.12.19 10:53, Jan Beulich wrote:
> 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?
I do not see "ucode=scan" applicable in anyway in the case of EFI. In 
EFI, there are not "modules" to scan through, but rather the efi config 
points exactly to the microcode blob.
> > > > --- 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?
Ack

> > > > @@ -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.
That's a valid point indeed. v3 will handle index being zero.

> > > >    }
> > > >    
> > > > -/*
> > > > - * 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.
Ack

-- Eslam

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