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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.12.2019 11:34, Jürgen Groß wrote:
> On 20.12.19 11:12, Jan Beulich wrote:
>> On 19.12.2019 23:11, Eslam Elnikety wrote:
>>> On 18.12.19 13:42, Jan Beulich wrote:
>>>> On 18.12.2019 02:32, Eslam Elnikety wrote:
>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> +
>>>>> +Xen can bundle microcode updates within its image. This support is 
>>>>> conditional
>>>>> +on the build configuration BUILTIN_UCODE being enabled. Builtin 
>>>>> microcode is
>>>>> +useful to ensure that, by default, a minimum microcode patch level will 
>>>>> be
>>>>> +applied to the underlying CPU.
>>>>> +
>>>>> +To use microcode updates available on the build system as builtin,
>>>>> +use BUILTIN_UCODE_DIR to refer to the directory containing the firmware 
>>>>> updates
>>>>> +and specify the individual microcode patches via either 
>>>>> BUILTIN_UCODE_AMD or
>>>>> +BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. 
>>>>> For
>>>>> +instance, the configuration below is suitable for a build system which 
>>>>> has a
>>>>> +``/lib/firmware/`` directory which, in turn, includes the individual 
>>>>> microcode
>>>>> +patches ``amd-ucode/microcode_amd_fam15h.bin``, 
>>>>> ``intel-ucode/06-3a-09``, and
>>>>> +``intel-ucode/06-2f-02``.
>>>>> +
>>>>> +  CONFIG_BUILTIN_UCODE=y
>>>>> +  CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
>>>>> +  CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
>>>>> +  CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
>>>>
>>>> Rather than a blank as separator, the more conventional one on
>>>> Unix and alike would be : I think. Of course ideally there wouldn't
>>>> be any restriction at all on the characters usable here for file
>>>> names.
>>>>
>>>
>>> It would be great if there is a particular convention. The blank
>>> separator is aligned with Linux way of doing builtin microcode.
>>
>> Well, this is then another area where I would question whether we
>> really want to follow the Linux approach, but I'm not bothered
>> enough to make less non-conventional behavior here a requirement.
>>
>>>>> --- a/xen/arch/x86/Kconfig
>>>>> +++ b/xen/arch/x86/Kconfig
>>>>> @@ -218,6 +218,36 @@ config MEM_SHARING
>>>>>           bool "Xen memory sharing support" if EXPERT = "y"
>>>>>           depends on HVM
>>>>>    
>>>>> +config BUILTIN_UCODE
>>>>> + bool "Support for Builtin Microcode"
>>>>> + ---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.
>>>>
>>>> I continue to be unconvinced that this separate option is needed.
>>>> Albeit compared to the v1 approach I will agree that handling
>>>> would become more complicated without.
>>>
>>> Any particular preference between the v1 vs v2 approach?
>>
>> I definitely like the vendor separation.
>>
>>>>> @@ -701,7 +747,13 @@ static int __init microcode_init(void)
>>>>>         */
>>>>>        if ( ucode_blob.size )
>>>>>        {
>>>>> +#ifdef CONFIG_BUILTIN_UCODE
>>>>> +        /* No need to destroy module mappings if builtin was used */
>>>>> +        if ( !ucode_builtin )
>>>>> +            bootstrap_map(NULL);
>>>>> +#else
>>>>>            bootstrap_map(NULL);
>>>>> +#endif
>>>>
>>>> First of all - is there no ucode unrelated side effect of this
>>>> invocation? I.e. can it safely be skipped?
>>>
>>> Maybe I am missing something. Are you asking if we can safely skip the
>>> bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of
>>> course we really want these mappings to be gone")
>>
>> Yes - my point is that invoking the function here may in
>> principle cover for other mappings. However - this is the
>> invocation you've added in an earlier patch, isn't it? In
>> which case omitting it should be fine. Nevertheless I don't
>> see and harm in invoking the function, i.e. I'd rather keep
>> the code here simple.
>>
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/microcode/Makefile
>>>>> @@ -0,0 +1,46 @@
>>>>> +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
>>>>> +# Author: Eslam Elnikety <elnikety@xxxxxxxxxx>
>>>>> +#
>>>>> +# 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.
>>>>> +
>>>>> +# Remove quotes and excess spaces from configuration strings
>>>>> +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
>>>>> +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
>>>>> +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
>>>>> +
>>>>> +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing 
>>>>> blobs.
>>>>> +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
>>>>> +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
>>>>> +
>>>>> +ifneq ($(amd-blobs),)
>>>>> +obj-y += ucode_amd.o
>>>>> +endif
>>>>> +
>>>>> +ifneq ($(intel-blobs),)
>>>>> +obj-y += ucode_intel.o
>>>>> +endif
>>>>> +
>>>>> +ifeq ($(amd-blobs)$(intel-blobs),)
>>>>> +obj-y += ucode_dummy.o
>>>>> +endif
>>>>> +
>>>>> +ucode_amd.o: Makefile $(amd-blobs)
>>>>> + cat $(amd-blobs) > $@.bin
>>>>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section 
>>>>> .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@
>>>>> + rm -f $@.bin
>>>>> +
>>>>> +ucode_intel.o: Makefile $(intel-blobs)
>>>>> + cat $(intel-blobs) > $@.bin
>>>>> + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section 
>>>>> .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@
>>>>> + rm -f $@.bin
>>>>
>>>> This can be had with a pattern rule (with the vendor being the stem)
>>>> and hence without duplication, I think.
>>>>
>>>> Also - is simply concatenating the blobs reliable enough? There's no
>>>> build time diagnostic that the result would actually be understood
>>>> at runtime.
>>>>
>>>
>>> Concatenation is reliable (as long as the individual microcode blobs are
>>> not malformed, and in that case the builtin is not making matters worse
>>> compared to presenting the malformed update via <integer> | scan).
>>
>> A malformed update found the other way is a bug in the tools
>> constructing the respective images. A malformed built-in
>> update is a bug in the Xen build system. The put the question
>> differently: Is it specified somewhere that the blobs all have
>> to have certain properties, which the straight concatenation
>> relies upon?
>>
>>>>> +ucode_dummy.o: Makefile
>>>>> + $(CC) $(CFLAGS) -c -x c /dev/null -o $@;
>>>>
>>>> Since the commit message doesn't explain why this is needed, I
>>>> have to ask (I guess we somewhere have a dependency on $(obj-y)
>>>> not being empty).
>>>
>>> Your guess is correct. All sub-directories of xen/arch/x86 are expected
>>> to produce built_in.o. If there are not amd nor intel microcode blobs,
>>> there will be no build dependencies and the build fails preparing the
>>> built_in.o
>>
>> That's rather poor, but it's of course not your task to get this
>> fixed (it shouldn't be very difficult to create an empty
>> built_in.o for an empty $(obj-y)).
>>
>>>> _If_ it is needed, I don't see why you need
>>>> ifeq() around its use. In fact you could have
>>>>
>>>> obj-y := ucode-dummy.o
>>>>
>>>> right at the top of the file.
>>>>
>>>> Furthermore I don't really understand why you need this in the
>>>> first place. While cat won't do what you want with an empty
>>>> argument list, can't you simply prepend / append /dev/null?
>>>>
>>>
>>> To make sure we are on the same page. You are suggesting using
>>> "/dev/null" in case there are no amd/intel ucode to generate the
>>> ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
>>> input (complains about empty binary).
>>
>> That's again rather poor, this time of the utility - it should be
>> easy enough to produce an object with an empty .data (or whatever
>> it is) section. As above - I'm fine with you keeping the logic
>> then as is, provided you say in the description why it can't be
>> simplified.
> 
> What about using the attached patch for including the binary files?
> 
> I wanted to post that for my hypervisor-fs series, but I think it would
> fit here quite nice.

Why not, if it can be made fit the ucode situation.

Jan

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