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

From: Eslam Elnikety <elnikety@amazon.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Paul Durrant" <pdurrant@amazon.co.uk>,
	xen-devel@lists.xenproject.org,
	"David Woodhouse" <dwmw@amazon.co.uk>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
Date: Wed, 11 Dec 2019 00:18:50 +0100	[thread overview]
Message-ID: <1a17bb6d-6f32-280e-0d00-6d06f3520052@amazon.com> (raw)
In-Reply-To: <09e75904-8faa-fbae-39ea-d9bd026ea4fa@suse.com>

On 10.12.19 10:37, Jan Beulich wrote:
> 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 | ?
The useful case here would be specifying ucode=scan,builtin which would 
translate to fallback onto the builtin microcode if a scan fails. In 
fact, this is already the case given the implementation in v1. It will 
be better to clarify this semantic by allowing scan,builtin.

On that note, I really see the "<integer>" and "scan=<bool>" to be 
equal. Following the logic earlier we should probably also allow 
ucode=<integer>,builtin. This translates to fallback to builtin if there 
are no modules at index <integer>.

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

Indeed. This in any case will need a revamp to rework the ", instead of 
|". Will address in v2.

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

Will address in v2.

>> +	---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?
> 

I can go either way here. To me, two options is clearer.

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

Good catch! Will fix in v2.

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

Will adjust in v2.

>> +    {
>> +        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?

I am all onboard. See my earlier response to Andrew. I used the same 
logic that already exists for scan (which assumes that ucode_blob.data 
is allocated and should be freed when all CPUs are updated).

Thanks for the comments, Jan. On the earlier discussion, please do let 
me know (and others too) if you are convinced that builtin support for 
microcode is warranted/useful.

Cheers,
Eslam

> 
> Jan
> 

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