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

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

On Wed, Aug 07, 2013 at 03:09:43PM +0100, Jan Beulich wrote:
> >>> On 18.07.13 at 18:36, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > The option to turn this scan on/off is gated by the 'ucode'
> > parameter. The options are now:
> >  'initrd'    Scan for the microcode in any multiboot payload.
> >  <index>     Attempt to load microcode blob (not the cpio archive
> >              format) from the multiboot payload number.
> > 
> > This option alters slightly the 'ucode' parameter by now allowing
> > two parameters:
> >   ucode=[<index>,][initrd]
> 
> I don't really like this - it should permit a number _or_ "initrd", not
> both (as that's meaningless).

You could have have a microcode blob (with microcode v1), and there
*might* have a blob in initramfs (with microcode v2 or without). Since
the 'initrd' scans first - if it does not find it in the initramfs
it will fallback to the microcode blob (which might have slightly older
microcode blob data or perhaps is corrupted and does not have it).

Maybe my example is too contrived for reality.
> 
> Also I think the term "initrd" usually refers to the old style variant
> not using cpio format (see init/initramfs.c:populate_rootfs() in the
> Linux sources), hence "initramfs", "cpio", or "initrd-cpio" might be
> better to avoid confusion.

Perhaps then just 'scan' to have it all under one option?
> 
> > When this code works, the hypervisor ring buffer will have:
> > (XEN) microcode: CPU0 updated from revision 0x25 to 0x28, date = 2012-04-24
> > .. and so on for all the other CPUs.
> > (This is an example from a SandyBridge CPU).
> 
> This is misleading - the code might work, and there still might not
> be any such messages: Firmware may have loaded an up-to-date
> ucode version already.

I will remove it then.
> 
> >  void __init microcode_set_module(unsigned int idx)
> >  {
> >      ucode_mod_idx = idx;
> >      ucode_mod_forced = 1;
> >  }
> > -
> 
> Please don't drop newlines between functions.

<nods>
> 
> > +/*
> > + * The format is '<integer>,[initrd]'. Both options are optional.
> > + * If the EFI has forced which of the multiboot payloads is to be used,
> > + * the <index> parsing will not be attempted.
> > + */
> >  static void __init parse_ucode(char *s)
> >  {
> > -    if ( !ucode_mod_forced )
> 
> This conditional should remain here, now guarding the whole rest
> of the function (perhaps by inverting it and bailing out early): If
> there was a "ucode=" in the EFI config file, then that's what we
> ought to use; no attempts should be made to override the admin's
> choice.

OK.
> 
> > -        ucode_mod_idx = simple_strtol(s, NULL, 0);
> > +    char *ss;
> > +
> > +    do {
> > +        ss = strchr(s, ',');
> > +        if ( ss )
> > +            *ss = '\0';
> > +
> > +        if ( !ucode_mod_forced && ucode_mod_idx == 0)
> 
> Coding style.

Ah, that ')' is off. Thanks for spotting.

> 
> > +static __initdata const char * const ucodes[] = {"kernel/x86/microcode/GenuineIntel.bin",
> 
> Some gcc versions will cause errors mixing constant and non-constant
> data placed in the same overridden section. Please drop the second
> const.
> 
> > +                                                 "kernel/x86/microcode/AuthenticAMD.bin"};
> 
> I wonder anyway whether we wouldn't be better off using CPUID
> function 0 output here to generate the name, and drop this pretty
> contrived array approach (you don't use the array as such
> anyway, i.e. you could as well inline the uses).

We can do that, however - we would have to use an snprintf and such - 
which would make the code a bit lengthier (or maybe not, since the CPUID
value is of fixed size).

How about for this patch we just do inline the uses and then
in another fortcomming patch I can try the cpuid approach and see
how that pans out?

And if the cpuid approach is cleaner I can just squash both of them
when its time to put this in.
> 
> > +/*
> > + * 8MB ought to be enough.
> > + */
> > +#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
> > +
> > +void __init microcode_scan_module(
> > +    unsigned long *module_map,
> > +    const multiboot_info_t *mbi,
> > +    void *(*bootmap)(const module_t *))
> > +{
> > +    module_t *mod = (module_t *)__va(mbi->mods_addr);
> > +    uint64_t *_blob_start;
> > +    unsigned long _blob_size;
> > +    struct cpio_data cd;
> > +    long offset;
> > +    const char *p = NULL;
> > +    int i;
> > +
> > +    ucode_blob.size = 0;
> > +    if ( !ucode_scan )
> > +        return;
> > +
> > +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> > +        p = ucodes[1];
> > +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> > +        p = ucodes[0];
> > +    else
> > +        return;
> > +
> > +    /*
> > +     * Try all modules and see whichever could be the microcode blob.
> > +     */
> > +    for ( i = 0; i < mbi->mods_count; i++ )
> 
> Module 0 is unconditionally the Dom0 kernel image, so no need to
> look at it.

OK
> 
> > +    {
> > +        if ( !test_bit(i, module_map) )
> > +            continue;
> > +
> > +        _blob_start = bootmap(&mod[i]);
> > +        _blob_size = mod[i].mod_end;
> > +        if ( !_blob_start )
> > +        {
> > +            printk("Could not map multiboot module #%d (size: %ld)\n",
> > +                   i, _blob_size);
> > +            return;
> 
> continue?

OK.
> 
> > +        }
> > +        cd.data = NULL;
> > +        cd.size = 0;
> > +        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* don't care */);
> 
> Line length. (Is the comment really valuable?)

I think it makes it easier to discard that information when looking at the code
and not having to say: "ok , so there is _blob_start, _blob_size and offset, where
is that being used..".

> 
> > +        if ( cd.data )
> > +        {
> > +                /*
> > +                 * This is an arbitrary check - it would be sad if the blob
> > +                 * consumed most of the memory and did not allow guests
> > +                 * to launch.
> > +                 */
> > +                if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
> > +                {
> > +                    printk("Multiboot %d microcode payload too big! (%ld, we can do %d)\n",
> > +                           i, cd.size, MAX_EARLY_CPIO_MICROCODE);
> > +                    goto err;
> > +                }
> > +                ucode_blob.size = cd.size;
> > +                ucode_blob.data = xmalloc_bytes(cd.size);
> > +                if ( !ucode_blob.data )
> > +                    goto err;
> 
> Again, no reason to bail entirely. Clear cd.data, put the memcpy()
> that follows past an "else", and all is fine.

OK.
> 
> >  static void __init _do_microcode_update(unsigned long data)
> >  {
> > -    microcode_update_cpu((void *)data, ucode_mod.mod_end);
> > +    void *_data = (void *)data;
> > +    size_t len = ucode_mod.mod_end;
> > +
> > +    if ( ucode_blob.size )
> > +    {
> > +        _data = (void *)ucode_blob.data;
> > +        len = ucode_blob.size;
> 
> Here it becomes very visible why allowing both methods at the
> same time is bad: What tells you that what you found in the
> cpio is better than what was specified as an explicit module?

Perhaps then try both? And whichever one that the underlaying
platform code would like to use -  that is the one we will stick
with for the SMP case?

(Perhaps as a different a patch to not make this patch more complex).
> 
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -48,7 +48,7 @@ obj-y += radix-tree.o
> >  obj-y += rbtree.o
> >  obj-y += lzo.o
> >  
> > -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo,$(n).init.o)
> > +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo,$(n).init.o earlycpio.o)
> 
> I think you meant
> 
> obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo earlycpio,$(n).init.o)
> 
> ?

Yes.
> 
> > +struct cpio_data __cpuinit find_cpio_data(const char *path, void *data,
> 
> struct cpio_data __init find_cpio_data(const char *path, const void *data,
> 
> > +					  size_t len,  long *offset)
> > +{
> > +	const size_t cpio_header_len = 8*C_NFIELDS - 2;
> > +	struct cpio_data cd = { NULL, 0, "" };
> > +	const char *p, *dptr, *nptr;
> > +	unsigned int ch[C_NFIELDS], *chp, v;
> > +	unsigned char c, x;
> > +	size_t mypathsize = strlen(path);
> > +	int i, j;
> > +
> > +	p = data;
> > +
> > +	while (len > cpio_header_len) {
> > +		if (!*p) {
> > +			/* All cpio headers need to be 4-byte aligned */
> > +			p += 4;
> > +			len -= 4;
> > +			continue;
> > +		}
> > +
> > +		j = 6;		/* The magic field is only 6 characters */
> > +		chp = ch;
> > +		for (i = C_NFIELDS; i; i--) {
> > +			v = 0;
> > +			while (j--) {
> > +				v <<= 4;
> > +				c = *p++;
> > +
> > +				x = c - '0';
> > +				if (x < 10) {
> > +					v += x;
> > +					continue;
> > +				}
> > +
> > +				x = (c | 0x20) - 'a';
> > +				if (x < 6) {
> > +					v += x + 10;
> > +					continue;
> > +				}
> > +
> > +				goto quit; /* Invalid hexadecimal */
> > +			}
> > +			*chp++ = v;
> > +			j = 8;	/* All other fields are 8 characters */
> > +		}
> > +
> > +		if ((ch[C_MAGIC] - 0x070701) > 1)
> > +			goto quit; /* Invalid magic */
> > +
> > +		len -= cpio_header_len;
> > +
> > +		dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
> > +		nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
> > +
> > +		if (nptr > p + len || dptr < p || nptr < dptr)
> > +			goto quit; /* Buffer overrun */
> > +
> > +		if ((ch[C_MODE] & 0170000) == 0100000 &&
> > +		    ch[C_NAMESIZE] >= mypathsize &&
> > +		    !memcmp(p, path, mypathsize)) {
> > +			*offset = (long)nptr - (long)data;
> > +			if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
> > +				printk(
> > +				"File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
> > +				p, MAX_CPIO_FILE_NAME);
> > +			}
> > +			strlcpy(cd.name, p + mypathsize, MAX_CPIO_FILE_NAME);
> > +
> > +			cd.data = (void *)dptr;
> > +			cd.size = ch[C_FILESIZE];
> > +			return cd; /* Found it! */
> 
> I realize that you took this from Linux, but it looks wrong
> nevertheless: The code compares the name with the passed in
> path up to the length of that path, and stores the remainder in
> cd.name. Yet the caller doesn't ever look at cd.name, and
> hence something like kernel/x86/microcode/GenuineIntel.bin.sig
> would match too.
> 
> For the purposes here, you could make the check above
> ch[C_NAMESIZE] == mypathsize and drop the name field from
> struct cpio_data.

Of course.
> 
> Jan