Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2019 09:41, Eslam Elnikety wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2113,7 +2113,7 @@ logic applies:
>     active by default.
>  
>  ### ucode (x86)
> -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
> +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]`

Despite my other question regarding the usefulness of this as a
whole a few comments.

Do "scan" and "builtin" really need to exclude each other? I.e.
don't you mean , instead of | ?

> @@ -2128,6 +2128,9 @@ 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
>  entry; see [EFI configuration file description](efi.html)).
>  
> +'builtin' instructs the hypervisor to use the builtin microcode update. This
> +option is available only if option BUILTIN_UCODE is enabled.

You also want to clarify its default - your reply to Andrew
suggested to me that only the negative form would really be
useful.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -218,6 +218,26 @@ config MEM_SHARING
>       bool "Xen memory sharing support" if EXPERT = "y"
>       depends on HVM
>  
> +config BUILTIN_UCODE
> +     def_bool n
> +     prompt "Support for Builtin Microcode"

These two lines should be folded into just

        bool "Support for Builtin Microcode"

irrespective of other bad examples you may find in the code base.
The again ...

> +     ---help---
> +       Include the CPU microcode update in the Xen image itself. With this
> +       support, Xen can update the CPU microcode upon boot using the builtin
> +       microcode, with no need for an additional microcode boot modules.
> +
> +       If unsure, say N.
> +
> +config BUILTIN_UCODE_DIR
> +     string
> +     default "/lib/firmware"
> +     depends on BUILTIN_UCODE

... are two separate options needed at all? Can't this latter one
being the empty string just imply the feature to be disabled?

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -7,6 +7,7 @@ subdir-y += mm
>  subdir-$(CONFIG_XENOPROF) += oprofile
>  subdir-$(CONFIG_PV) += pv
>  subdir-y += x86_64
> +subdir-$(CONFIG_BUILTIN_UCODE) += microcode

Please respect the (half way?) alphabetical sorting here, unless
adding last is a requirement (in which case a brief comment should
say so, and why).

> @@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s)
>          {
>              if ( (val = parse_boolean("scan", s, ss)) >= 0 )
>                  ucode_scan = val;
> +#ifdef CONFIG_BUILTIN_UCODE
> +         else if ( (val = parse_boolean("builtin", s, ss)) >= 0 )

Please watch out for hard tabs where they don't belong.

> @@ -237,6 +249,48 @@ void __init microcode_grab_module(
>  scan:
>      if ( ucode_scan )
>          microcode_scan_module(module_map, mbi);
> +
> +#ifdef CONFIG_BUILTIN_UCODE
> +    /*
> +     * Do not use the builtin microcode if:
> +     * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin)
> +     * (b) a microcode module has been specified or a scan is successful
> +     */
> +    if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size )
> +        return;
> +
> +    /* Set ucode_start/_end to the proper blob */
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +        ucode_blob.size = (size_t)(__builtin_amd_ucode_end
> +                                   - __builtin_amd_ucode_start);
> +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +        ucode_blob.size = (size_t)(__builtin_intel_ucode_end
> +                                   - __builtin_intel_ucode_start);
> +    else
> +        return;
> +
> +    if ( !ucode_blob.size )
> +    {
> +        printk("No builtin ucode! 'ucode=builtin' is nullified.\n");
> +        return;
> +    }
> +    else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE )

With the "return" above please omit the "else" here. But why this
restriction, and ...

> +    {
> +        printk("Builtin microcode payload too big! (%ld, we can do %d)\n",
> +               ucode_blob.size, MAX_EARLY_CPIO_MICROCODE);
> +        ucode_blob.size = 0;
> +        return;
> +    }
> +
> +    ucode_blob.data = xmalloc_bytes(ucode_blob.size);
> +    if ( !ucode_blob.data )
> +        return;
> +
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +        memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size);
> +    else
> +        memcpy(ucode_blob.data, __builtin_intel_ucode_start, 
> ucode_blob.size);

... why the copying? Can't you simply point ucode_blob.data at
__builtin_{amd,intel}_ucode_start?

Jan

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