Re: [RFC PATCH v1] microcode: Scan the initramfs payload for microcode blob.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jul 15, 2013 at 08:23:04AM +0100, Jan Beulich wrote:
> >>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -760,6 +760,18 @@ only available on 32-bit x86 platforms.
> >  
> >  `acpi` instructs Xen to reboot the host using RESET_REG in the ACPI FADT.
> >  
> > +### scan_ucode
> > +> `= <boolean>`
> > +
> > +> Default: `true`
> > +
> > +Flag to globally enable or disable support for scanning the multiboot images
> > +for an cpio image that contains microcode, which are:
> > + on Intel: kernel/x86/microcode/GenuineIntel.bin
> > + on AMD  : kernel/x86/microcode/AuthenticAMD.bin
> > +
> > +Note that is 'ucode' parameter is used this option becomes disabled.
> > +
> 
> I would want this to be an extension of the "ucode=" option, not
> an independent new one. E.g. "ucode=initrd" or
> "ucode=cpio:<num>" or some such.

I believe that it would be best suited for users if said feature was
turned on by default - and then if needed - can be turned off. Similarly
to how HAP is enabled by default.

Using the over-ride in 'ucode' parse code is fine with me. I would propose
'ucode=noscan' or 'ucode=noauto' as the parameters.
> 
> > --- a/xen/arch/x86/microcode.c
> > +++ b/xen/arch/x86/microcode.c
> > @@ -39,12 +39,33 @@
> >  #include <asm/setup.h>
> >  #include <asm/microcode.h>
> >  
> > +#include <xen/earlycpio.h>
> > +
> >  static module_t __initdata ucode_mod;
> >  static void *(*__initdata ucode_mod_map)(const module_t *);
> >  static signed int __initdata ucode_mod_idx;
> >  static bool_t __initdata ucode_mod_forced;
> >  static cpumask_t __initdata init_mask;
> >  
> > +/*
> > + * If we scan the initramfs.cpio for the early microcode code
> > + * and find it, then 'ucode_blob' will contain the pointer
> > + * and the size of said blob. It is allocated from Xen's heap
> > + * memory.
> > + */
> > +struct ucode_mod_blob {
> > +    void *data;
> > +    size_t size;
> > +};
> > +
> > +static struct ucode_mod_blob __initdata ucode_blob;
> > +/*
> > + * By default we will parse the multiboot modules to see if there is
> > + * cpio image with the microcode images.
> > + */
> > +static bool_t __initdata opt_scan_ucode = 1;
> > +boolean_param("scan_ucode", opt_scan_ucode);
> 
> I'm also unsure we really want this on by default. "ucode=<num>"
> also isn't having any "default on" effect.

Correct. Because it ('ucode=<num>') is unable to figure out which of
the multiboot payloads has the ucode. It needs the help from GRUB2
or other tools that construct the bootloader configuration files.

This code, similar to how XSM finds its policy, can find the microcode
blob without the help of the bootloader.

In the Linux code if this feature is turned on it will _always_
search the initramfs. I think that idea makes sense here as well -
we want to enable X feature if we detect that it exists.

> 
> Reviewing the rest of the patch requires clarification on the
> concept first, as asked for in response to the "overview" mail.
> 
> Jan
> 
>