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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

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

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

Jan

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