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: Thu, 12 Dec 2019 23:13:07 +0100	[thread overview]
Message-ID: <ff5b0699-5010-fabc-f7fd-2d40f8c56644@amazon.com> (raw)
In-Reply-To: <eb8beed3-3b9e-25f9-94bf-c6fe56a397f5@suse.com>

On 11.12.19 10:47, Jan Beulich wrote:
> On 10.12.2019 23:40, Eslam Elnikety wrote:
>> On 10.12.19 10:21, Jan Beulich wrote:
>>> On 09.12.2019 22:49, Eslam Elnikety wrote:
>>>> On 09.12.19 16:19, Andrew Cooper wrote:
>>>>> On 09/12/2019 08:41, Eslam Elnikety wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>>>> @@ -0,0 +1,40 @@
>>>>>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
>>>>>> +# Author: Eslam Elnikety <elnikety@amazon.com>
>>>>>> +#
>>>>>> +# This program is free software; you can redistribute it and/or modify
>>>>>> +# it under the terms of the GNU General Public License as published by
>>>>>> +# the Free Software Foundation; either version 2 of the License, or
>>>>>> +# (at your option) any later version.
>>>>>> +#
>>>>>> +# This program is distributed in the hope that it will be useful,
>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>> +# GNU General Public License for more details.
>>>>>> +
>>>>>> +obj-y += builtin_ucode.o
>>>>>> +
>>>>>> +# Directory holding the microcode updates.
>>>>>> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR))
>>>>>> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*)
>>>>>> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*)
>>>>>
>>>>> This is a little dangerous.  I can see why you want to do it like this,
>>>>> and I can't provide any obvious suggestions, but if this glob picks up
>>>>> anything which isn't a microcode file, it will break the logic to search
>>>>> for the right blob.
>>>>>
>>>>
>>>> We can limit the amd-blobs and intel-blob to binaries following the
>>>> naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and
>>>> intel, respectively. Yet, this would be imposing an unnecessary
>>>> restriction on administrators who may want to be innovative with naming
>>>> (or want to use the naming microcode_amd_*.bin or FF-MM-SS instead).
>>>>
>>>> Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and
>>>> CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an
>>>> administrator can specify exactly the microcodes to include relative to
>>>> the CONFIG_BUILTIN_UCODE_DIR. For example:
>>>> CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09"
>>>> CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>>>
>>> This would make the feature even less generic - I already meant to
>>
>> I do not follow the point about being less generic. (I hope my example
>> did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL}
>> allow for only a single microcode blob for a single signature).
> 
> Well, the example indeed has given this impression to me. I'm
> having a hard time seeing how, beyond very narrow special cases,
> either of the examples could be useful to anyone. Yet I think
> examples should be generally useful.
> 

Andrew's earlier response hinted at "what can we do to avoid picking 
random bits in the builtin microcode?" My response was outlining 
alternatives, and the examples there were not meant to be useful beyond 
explaining those alternatives.

>>> ask whether building ucode into binaries is really a useful thing
>>> when we already have more flexible ways. I could see this being
>>> useful if there was no other way to make ucode available at boot
>>> time.
>>
>> It is useful in addition to the existing ways to do early microcode
>> updates. First, when operating many hosts, using boot modules (either a
>> distinct microcode module or within an initrd) becomes involved. For
>> instance, tools to update boot entries (e.g.,
>> https://linux.die.net/man/8/grubby) do not support adding arbitrary
>> (microcode) modules.
> 
> I.e. you suggest to work around tools shortcomings by extending
> Xen? Wouldn't the more appropriate way to deal with this be to
> make the tools more capable?
> 
>> Second, there is often need to couple a Xen build with a minimum
>> microcode patch level. Having the microcode built within the Xen image
>> itself is a streamlined, natural way of achieving that.
> 
> Okay, I can accept this as a reason, to some degree at least. Yet
> as said elsewhere, I don't think you want then to override a
> possible "external" ucode module with the builtin blobs. Instead
> the newest of everything that's available should then be loaded.

Extending Xen to work around tools shortcomings is absolutely not what I 
have in mind. I should have started with the second reason. Read this 
as: Xen relies on a minimum microcode feature set, and it makes sense to 
couple both in one binary. This coupling just happens to provide an 
added benefit in the face of tools shortcoming.

Thanks,
Eslam

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