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

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

On 09.12.19 16:19, Andrew Cooper wrote:
> On 09/12/2019 08:41, Eslam Elnikety wrote:
> > diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt
> > new file mode 100644
> > index 0000000000..43bb60d3eb
> Instead of introducing a new file, please extend
> docs/admin-guide/microcode-loading.rst
>
> I have an in-prep docs/hypervisor-guide/microcode-loading.rst as well,
> which I'll see about posting.  It would be a more appropriate place for
> the technical details.
Agreed!

While the existing docs/admin-guide/microcode-loading.rst speaks a 
different tone than what I added, that documentation anyway needs to be 
updated with builtin if such support were to be added. I will adjust 
accordingly. If docs/hypervisor-guide/microcode-loading.rst makes it in 
time for v2 of this patch, I will reflect changes there too.
> > diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> > index 6ced293d88..7afbe44286 100644
> > --- a/xen/arch/x86/microcode.c
> > +++ b/xen/arch/x86/microcode.c
> > @@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob;
> >    */
> >   static bool_t __initdata ucode_scan;
> >   
> > +#ifdef CONFIG_BUILTIN_UCODE
> > +/* builtin is the default when BUILTIN_UCODE is set */
> > +static bool_t __initdata ucode_builtin = 1;
> bool, true.
Will fix in v2.

> > +
> > +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[];
> > +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[];
> > +#endif
> > +
> >   /* By default, ucode loading is done in NMI handler */
> >   static bool ucode_in_nmi = true;
> >   
> > @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx)
> >   }
> >   
> >   /*
> > - * 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.
> > + * The format is '[<integer>|scan=<bool>|builtin=<bool>, nmi=<bool>]'. All
> > + * options are optional. If the EFI has forced which of the multiboot payloads
> > + * is to be used, only nmi=<bool> is parsed.
> >    */
> Please delete this, or I'll do a prereq patch to fix it and the command
> line docs.  (Both are in a poor state.)
Unless you are planning that along your on-going 
docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this 
clean-up/prereq patch myself. What do you have in mind? (Or point me to 
a good example and I will figure things out).
> > @@ -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);
> No need to cast the result.  Also, preferred Xen style would be to have
> - on the preceding line.
Will fix in v2.

> > +    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 )
> > +    {
> > +        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;
> Any chance we can reuse the "fits" logic to avoid holding every
> inapplicable blob in memory as well?
I think this would be a welcomed change. It seems to me that we have two 
ways to go about it.
1) We factor the code in the intel-/amd-specific cpu_request_microcode 
to extract logic for finding a match into its own new function, expose 
that through microcode_ops, and finally do xalloc only for the matching 
microcode when early loading is scan or builtin.
2) Cannot we just do away completely with xalloc? I see that each 
individual microcode update gets allocated anyway in 
microcode_intel.c/get_next_ucode_from_buffer() and in 
microcode_amd.c/cpu_request_microcode(). Unless I am missing something, 
the xmalloc_bytes for ucode_blob.data is redundant.
Thoughts?

> > +
> > +    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);
> > +#endif
> >   }
> >   
> >   const struct microcode_ops *microcode_ops;
> > diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile
> > new file mode 100644
> > index 0000000000..6d585c5482
> > --- /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@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.
> > +
> > +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"

Thoughts?

> > +
> > +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs)
> > +       # Create AMD microcode blob if there are AMD updates on the build system
> > +       if [ ! -z "$(amd-blobs)" ]; then \
> > +               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 $@.amd; \
> > +               rm -f $@.bin; \
> > +       fi
> > +       # Create INTEL microcode blob if there are INTEL updates on the build 
> > system
> > +       if [ ! -z "$(intel-blobs)" ]; then \
> > +               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 $@.intel; \
> > +               rm -f $@.bin; \
> > +       fi
> > +       # Create fake builtin_ucode.o if no updates were present. Otherwise, 
> > builtin_ucode.o carries the available updates
> > +       if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \
> > +               $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \
> > +       else \
> > +               $(LD) $(LDFLAGS) -r -o $@ $@.*; \
> > +               rm -f $@.*; \
> > +       fi
> How about using weak symbols, rather than playing games like this?
Just to make sure we are on the same page. You are after a dummy binary 
with weak symbols that eventually get overridden when I link the actual 
microcode binaries into builtin_ucode.o? If so, possible of course. 
Except that I do not particularly see the downside of the existing 
approach with dummy builtin_ucode.o.
> ~Andrew
Thanks a lot for those insightful comments, Andrew.

Cheers,
Eslam

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