thr3ads.net - Xen devel - Scan initramfs for microcode cpio archive. (v2) [Jul 2013]

If this information is useful, please help other people find it:
Share via:

Konrad Rzeszutek Wilk

2013-Jul-18 16:36 UTC

Scan initramfs for microcode cpio archive. (v2)

Greetings,

Please see the following patch which implements a mechanism to scan
the initramfs for the format of an microcode files. This is a feature
that the Linux kernel has since v3.10 - where it searches in the
initramfs for an archive of the microcode blob. The format is documented
in the Linux tree and the commit description contains it.

The tool to make this work is the initramfs creator. The one tool
''dracut'' has support for this via the
''--early-microcode'' parameter.
(See http://news.gmane.org/gmane.linux.kernel.initramfs for dracut 030
announcement))

That, along with this patch, allows the Xen hypervisor to update the
microcode during bootup. Please review attached patch.

I had also tested just using Linux how well it deals with an initramfs
composed of two cpio images. Testing revealed that it worked great
even if the kernel did not have the early cpio support build in.
David Vrabel pointed out that it b/c:
"The kernel unpacks all cpio archives it finds in the initramfs image so
the kernel doesn''t have to be aware of the way tools have packed the
filesystem into different cpio archive."

The way to use this is by the ''ucode'' parameter. It has now
two meanings:
  <index>,initrd

Which can be used together. By default this auto scanning is turned off
as Jan pointed out that: "Xen otoh has to be careful not to
mis-interpret a blob passed to a non-Linux Dom0 as a CPIO. How
good the guarding against this is in the code I''ll have to check".

The author would like to have this on by default but that can wait
till a later time when maintainer is comfortable with this being
on by default.

 docs/misc/xen-command-line.markdown |   14 ++-
 xen/arch/x86/microcode.c            |  179 +++++++++++++++++++++++++++++++----
 xen/common/Makefile                 |    2 +-
 xen/common/earlycpio.c              |  151 +++++++++++++++++++++++++++++
 xen/include/xen/earlycpio.h         |   15 +++
 5 files changed, 340 insertions(+), 21 deletions(-)

Konrad Rzeszutek Wilk (1):
      microcode: Scan the initramfs payload for microcode blob.

Konrad Rzeszutek Wilk

2013-Jul-18 16:36 UTC

head link

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

The Linux kernel is able to update the microcode during early
bootup via inspection of the initramfs blob to see if there is an
cpio image with certain microcode files. Linux is able to function
with two (or more) cpio archives in the initrd b/c it unpacks
all of the cpio archives.

The format of the early initramfs is nicely documented
in Linux''s Documentation/x86/early-microcode.txt:

Early load microcode
===================By Fenghua Yu <fenghua.yu@intel.com>

Kernel can update microcode in early phase of boot time. Loading microcode early
can fix CPU issues before they are observed during kernel boot time.

Microcode is stored in an initrd file. The microcode is read from the initrd
file and loaded to CPUs during boot time.

The format of the combined initrd image is microcode in cpio format followed by
the initrd image (maybe compressed). Kernel parses the combined initrd image
during boot time. The microcode file in cpio name space is:
kernel/x86/microcode/GenuineIntel.bin

During BSP boot (before SMP starts), if the kernel finds the microcode file in
the initrd file, it parses the microcode and saves matching microcode in memory.
If matching microcode is found, it will be uploaded in BSP and later on in all
APs.

The cached microcode patch is applied when CPUs resume from a sleep state.

There are two legacy user space interfaces to load microcode, either through
/dev/cpu/microcode or through /sys/devices/system/cpu/microcode/reload file
in sysfs.

In addition to these two legacy methods, the early loading method described
here is the third method with which microcode can be uploaded to a
system''s
CPUs.

The following example script shows how to generate a new combined initrd file in
/boot/initrd-3.5.0.ucode.img with original microcode microcode.bin and
original initrd image /boot/initrd-3.5.0.img.

mkdir initrd
cd initrd
mkdir kernel
mkdir kernel/x86
mkdir kernel/x86/microcode
cp ../microcode.bin kernel/x86/microcode/GenuineIntel.bin
find .|cpio -oc >../ucode.cpio
cd ..
cat ucode.cpio /boot/initrd-3.5.0.img >/boot/initrd-3.5.0.ucode.img

As such this code inspects the initrd to see if the microcode
signatures are present and if so updates the hypervisor.

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]

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).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Revised code based on Boris Ostrovsky''s comments]
[v2: Fix memory leak (forgot to call xfree)]
[v3: Added comments from David, stuck the option under ucode per Jan''s
     review]
---
 docs/misc/xen-command-line.markdown |   14 ++-
 xen/arch/x86/microcode.c            |  179 +++++++++++++++++++++++++++++++----
 xen/common/Makefile                 |    2 +-
 xen/common/earlycpio.c              |  151 +++++++++++++++++++++++++++++
 xen/include/xen/earlycpio.h         |   15 +++
 5 files changed, 340 insertions(+), 21 deletions(-)
 create mode 100644 xen/common/earlycpio.c
 create mode 100644 xen/include/xen/earlycpio.h

diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 708ffc2..70f873e 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -879,10 +879,12 @@ pages) must also be specified via the tbuf\_size
parameter.
 > `= unstable | skewed`
 
 ### ucode
-> `= <integer>`
+> `= <integer> , initrd`
+
+Specify how and where to find CPU microcode update blob.
 
-Specify the CPU microcode update blob module index. When positive, this
-specifies the n-th module (in the GrUB entry, zero based) to be used
+''interger'' specifies the CPU microcode update blob module
index. When positive,
+this specifies the n-th module (in the GrUB entry, zero based) to be used
 for updating CPU micrcode. When negative, counting starts at the end of
 the modules in the GrUB entry (so with the blob commonly being last,
 one could specify `ucode=-1`). Note that the value of zero is not valid
@@ -892,6 +894,12 @@ when used with xen.efi (there the concept of modules
doesn''t exist, and
 the blob gets specified via the `ucode=<filename>` config file/section
 entry; see [EFI configuration file description](efi.html)).
 
+''initrd'' instructs the hypervisor to scan the multiboot
images for an cpio
+image that contains microcode. Depending on the platform the format of
+the microcode blobs must be:
+  - on Intel: kernel/x86/microcode/GenuineIntel.bin
+  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
+
 ### unrestricted\_guest
 > `= <boolean>`
 
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index fbbf95b..b191e9f 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -39,25 +39,139 @@
 #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 NOT parse the multiboot modules to see if there is
+ * cpio image with the microcode images.
+ */
+static bool_t __initdata ucode_scan;
+
 void __init microcode_set_module(unsigned int idx)
 {
     ucode_mod_idx = idx;
     ucode_mod_forced = 1;
 }
-
+/*
+ * 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 )
-        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)
+            ucode_mod_idx = simple_strtol(s, NULL, 0);
+
+        if ( !strncmp(s, "initrd", 6) )
+            ucode_scan = 1;
+
+        s = ss + 1;
+    } while ( ss );
 }
 custom_param("ucode", parse_ucode);
 
+static __initdata const char * const ucodes[] =
{"kernel/x86/microcode/GenuineIntel.bin",
+                                                
"kernel/x86/microcode/AuthenticAMD.bin"};
+/*
+ * 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++ )
+    {
+        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;
+        }
+        cd.data = NULL;
+        cd.size = 0;
+        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /*
don''t care */);
+        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;
+                memcpy(ucode_blob.data, cd.data, cd.size);
+        }
+        bootmap(NULL);
+        if ( cd.data )
+            break;
+    }
+    return;
+err:
+    bootmap(NULL);
+}
 void __init microcode_grab_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi,
@@ -69,9 +183,12 @@ void __init microcode_grab_module(
         ucode_mod_idx += mbi->mods_count;
     if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
          !__test_and_clear_bit(ucode_mod_idx, module_map) )
-        return;
+        goto scan;
     ucode_mod = mod[ucode_mod_idx];
     ucode_mod_map = map;
+scan:
+    if ( ucode_scan )
+        microcode_scan_module(module_map, mbi, map);
 }
 
 const struct microcode_ops *microcode_ops;
@@ -236,7 +353,15 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void)
buf, unsigned long len)
 
 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;
+    }
+    microcode_update_cpu(_data, len);
     cpumask_set_cpu(smp_processor_id(), &init_mask);
 }
 
@@ -246,18 +371,22 @@ static int __init microcode_init(void)
     static struct tasklet __initdata tasklet;
     unsigned int cpu;
 
-    if ( !microcode_ops || !ucode_mod.mod_end )
+    if ( !microcode_ops )
+        return 0;
+
+    if ( !ucode_mod.mod_end && !ucode_blob.size )
         return 0;
 
-    data = ucode_mod_map(&ucode_mod);
+    if ( ucode_blob.size )
+        data = ucode_blob.data;
+    else
+        data = ucode_mod_map(&ucode_mod);
+
     if ( !data )
         return -ENOMEM;
 
     if ( microcode_ops->start_update &&
microcode_ops->start_update() != 0 )
-    {
-        ucode_mod_map(NULL);
-        return 0;
-    }
+        goto out;
 
     softirq_tasklet_init(&tasklet, _do_microcode_update, (unsigned
long)data);
 
@@ -269,7 +398,11 @@ static int __init microcode_init(void)
         } while ( !cpumask_test_cpu(cpu, &init_mask) );
     }
 
-    ucode_mod_map(NULL);
+out:
+    if ( ucode_blob.size )
+        xfree(data);
+    else
+        ucode_mod_map(NULL);
 
     return 0;
 }
@@ -298,14 +431,26 @@ static int __init microcode_presmp_init(void)
 {
     if ( microcode_ops )
     {
-        if ( ucode_mod.mod_end )
+        if ( ucode_mod.mod_end || ucode_blob.size )
         {
-            void *data = ucode_mod_map(&ucode_mod);
-
+            void *data;
+            size_t len;
+
+            if ( ucode_blob.size )
+            {
+                len = ucode_blob.size;
+                data = ucode_blob.data;
+            }
+            else
+            {
+                len = ucode_mod.mod_end;
+                data = ucode_mod_map(&ucode_mod);
+            }
             if ( data )
-                microcode_update_cpu(data, ucode_mod.mod_end);
+                microcode_update_cpu(data, len);
 
-            ucode_mod_map(NULL);
+            if ( !ucode_blob.size )
+                ucode_mod_map(NULL);
         }
 
         register_cpu_notifier(&microcode_percpu_nfb);
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 0dc2050..f85d3f5 100644
--- 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)
 
 obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
diff --git a/xen/common/earlycpio.c b/xen/common/earlycpio.c
new file mode 100644
index 0000000..8b5abb6
--- /dev/null
+++ b/xen/common/earlycpio.c
@@ -0,0 +1,151 @@
+/* ----------------------------------------------------------------------- *
+ *
+ *   Copyright 2012 Intel Corporation; author H. Peter Anvin
+ *
+ *   This file is part of the Linux kernel, and is made available
+ *   under the terms of the GNU General Public License version 2, as
+ *   published by the Free Software Foundation.
+ *
+ *   This program is distributed in the hope 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.
+ *
+ * ----------------------------------------------------------------------- */
+
+/*
+ * earlycpio.c
+ *
+ * Find a specific cpio member; must precede any compressed content.
+ * This is used to locate data items in the initramfs used by the
+ * kernel itself during early boot (before the main initramfs is
+ * decompressed.)  It is the responsibility of the initramfs creator
+ * to ensure that these items are uncompressed at the head of the
+ * blob.  Depending on the boot loader or package tool that may be a
+ * separate file or part of the same file.
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/string.h>
+#include <xen/earlycpio.h>
+
+#define ALIGN(x, a) ((x + (a) - 1) & ~((a) - 1))
+#define PTR_ALIGN(p, a)         ((typeof(p))ALIGN((unsigned long)(p), (a)))
+
+enum cpio_fields {
+	C_MAGIC,
+	C_INO,
+	C_MODE,
+	C_UID,
+	C_GID,
+	C_NLINK,
+	C_MTIME,
+	C_FILESIZE,
+	C_MAJ,
+	C_MIN,
+	C_RMAJ,
+	C_RMIN,
+	C_NAMESIZE,
+	C_CHKSUM,
+	C_NFIELDS
+};
+
+/**
+ * cpio_data find_cpio_data - Search for files in an uncompressed cpio
+ * @path:   The directory to search for, including a slash at the end
+ * @data:   Pointer to the the cpio archive or a header inside
+ * @len:    Remaining length of the cpio based on data pointer
+ * @offset: When a matching file is found, this is the offset to the
+ *          beginning of the cpio. It can be used to iterate through
+ *          the cpio to find all files inside of a directory path
+ *
+ * @return: struct cpio_data containing the address, length and
+ *          filename (with the directory path cut off) of the found file.
+ *          If you search for a filename and not for files in a directory,
+ *          pass the absolute path of the filename in the cpio and make sure
+ *          the match returned an empty filename string.
+ */
+
+struct cpio_data __cpuinit find_cpio_data(const char *path, 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! */
+		}
+		len -= (nptr - p);
+		p = nptr;
+	}
+
+quit:
+	return cd;
+}
+
diff --git a/xen/include/xen/earlycpio.h b/xen/include/xen/earlycpio.h
new file mode 100644
index 0000000..16d9404
--- /dev/null
+++ b/xen/include/xen/earlycpio.h
@@ -0,0 +1,15 @@
+#ifndef _EARLYCPIO_H
+#define _EARLYCPIO_H
+
+#define MAX_CPIO_FILE_NAME 18
+
+struct cpio_data {
+	void *data;
+	size_t size;
+	char name[MAX_CPIO_FILE_NAME];
+};
+
+struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
+				long *offset);
+
+#endif /* _EARLYCPIO_H */
-- 
1.7.7.6

Konrad Rzeszutek Wilk

2013-Jul-19 03:59 UTC

head link

Re: Scan initramfs for microcode cpio archive. (v2)

As I thought about this more I realized that the user might have been
careless and misconstructed the payloads in myriad of ways. The patch
that is threaded attempts to fix this case.

I can squash it as part of the initial ''microcode: Scan the initramfs
payload for microcode blob" if that would be preferred of course.

 xen/arch/x86/microcode.c |   52 ++++++++++++++++++++++++++++++----------------
 1 files changed, 34 insertions(+), 18 deletions(-)

Konrad Rzeszutek Wilk (1):
      microcode: Check whether the microcode is correct.

Konrad Rzeszutek Wilk

2013-Jul-19 03:59 UTC

head link

[PATCH] microcode: Check whether the microcode is correct.

We do the microcode code update in two steps - the presmp:
 ''microcode_presmp_init'' and when CPUs are brought up:
''microcode_init''. The earlier performs the microcode update
on the BSP - but unfortunately it does not check whether the
update failed. Which means that we might try later to update
a incorrect payload on the rest of CPUs.

Furthermore now that the ucode= parameter can specify two
sources of microcode blob we have to be cognizant whether
both (or either) of the payloads are correct.

For example, the user could have constructed the cpio archive
with a empty GenuineIntel.bin file and provided the correct
GenuineIntel.bin as a seperate payload:

xen.gz ucode=2,initrd --- vmlinuz --- initramfs.cpio.gz --- GenuineIntel.bin

[the initramfs.cpio.gz has the cpio archive of the empty file
kernel/x86/microcode/GenuineIntel.bin pre-pended]

in which case we need to skip the microcode payload from the
initramfs.cpio.gz and use the GenuineIntel.bin payload.

Or if the user mishandled the GenuineIntel.bin seperate payload and
had the initramfs.cpio.gz correct. Or if both payloads had
bogus data.

This patch handles these odd situations.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/microcode.c |   52 ++++++++++++++++++++++++++++++----------------
 1 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index b191e9f..2ab2b46 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -433,24 +433,40 @@ static int __init microcode_presmp_init(void)
     {
         if ( ucode_mod.mod_end || ucode_blob.size )
         {
-            void *data;
-            size_t len;
-
-            if ( ucode_blob.size )
-            {
-                len = ucode_blob.size;
-                data = ucode_blob.data;
-            }
-            else
-            {
-                len = ucode_mod.mod_end;
-                data = ucode_mod_map(&ucode_mod);
-            }
-            if ( data )
-                microcode_update_cpu(data, len);
-
-            if ( !ucode_blob.size )
-                ucode_mod_map(NULL);
+            int rc;
+            do {
+                void *data = NULL;
+                size_t len;
+
+                rc = 0;
+                if ( ucode_blob.size )
+                {
+                    len = ucode_blob.size;
+                    data = ucode_blob.data;
+                }
+                else if ( ucode_mod.mod_end )
+                {
+                    len = ucode_mod.mod_end;
+                    data = ucode_mod_map(&ucode_mod);
+                }
+                if ( data )
+                    rc = microcode_update_cpu(data, len);
+
+                if ( !ucode_blob.size && ucode_mod.mod_end )
+                    ucode_mod_map(NULL);
+
+                if ( rc == -EINVAL )
+                {
+                    if ( ucode_blob.size ) /* That was tried first */
+                    {
+                        ucode_blob.size = 0;
+                        xfree(ucode_blob.data);
+                        continue;
+                    }
+                    if ( ucode_mod.mod_end )
+                        ucode_mod.mod_end = 0;
+                }
+            } while ( rc );
         }
 
         register_cpu_notifier(&microcode_percpu_nfb);
-- 
1.7.7.6

Jan Beulich

2013-Aug-07 14:09 UTC

head link

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

>>> 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).

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.
> 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.
>  void __init microcode_set_module(unsigned int idx)
>  {
>      ucode_mod_idx = idx;
>      ucode_mod_forced = 1;
>  }
> -
Please don''t drop newlines between functions.
> +/*
> + * 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.
> -        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.
> +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).
> +/*
> + * 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.
> +    {
> +        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?
> +        }
> +        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?)
> +        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.
>  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?
> --- 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)

?
> +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.

Jan

Jan Beulich

2013-Aug-07 14:17 UTC

head link

Re: [PATCH] microcode: Check whether the microcode is correct.

>>> On 19.07.13 at 05:59, Konrad Rzeszutek Wilk
<konrad@kernel.org> wrote:
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -433,24 +433,40 @@ static int __init microcode_presmp_init(void)
>      {
>          if ( ucode_mod.mod_end || ucode_blob.size )
>          {
> -            void *data;
> -            size_t len;
> -
> -            if ( ucode_blob.size )
> -            {
> -                len = ucode_blob.size;
> -                data = ucode_blob.data;
> -            }
> -            else
> -            {
> -                len = ucode_mod.mod_end;
> -                data = ucode_mod_map(&ucode_mod);
> -            }
> -            if ( data )
> -                microcode_update_cpu(data, len);
> -
> -            if ( !ucode_blob.size )
> -                ucode_mod_map(NULL);
> +            int rc;
> +            do {
> +                void *data = NULL;
> +                size_t len;
> +
> +                rc = 0;
> +                if ( ucode_blob.size )
> +                {
> +                    len = ucode_blob.size;
> +                    data = ucode_blob.data;
> +                }
> +                else if ( ucode_mod.mod_end )
> +                {
> +                    len = ucode_mod.mod_end;
> +                    data = ucode_mod_map(&ucode_mod);
> +                }
> +                if ( data )
> +                    rc = microcode_update_cpu(data, len);
else rc = -ENOMEM (yielding the initialization above pointless).
> +
> +                if ( !ucode_blob.size && ucode_mod.mod_end )
> +                    ucode_mod_map(NULL);
> +
> +                if ( rc == -EINVAL )
if ( rc )
> +                {
> +                    if ( ucode_blob.size ) /* That was tried first */
> +                    {
> +                        ucode_blob.size = 0;
> +                        xfree(ucode_blob.data);
> +                        continue;
> +                    }
> +                    if ( ucode_mod.mod_end )
> +                        ucode_mod.mod_end = 0;
> +                }
> +            } while ( rc );
>          }
>  
>          register_cpu_notifier(&microcode_percpu_nfb);
This may need re-doing anyway I we can agree that allowing both
location specifications at the same time is bogus and should hence
be dropped.

Jan

Konrad Rzeszutek Wilk

2013-Aug-08 00:50 UTC

head link

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

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

Konrad Rzeszutek Wilk

2013-Aug-08 00:50 UTC

head link

Re: [PATCH] microcode: Check whether the microcode is correct.

On Wed, Aug 07, 2013 at 03:17:07PM +0100, Jan Beulich
wrote:> >>> On 19.07.13 at 05:59, Konrad Rzeszutek Wilk
<konrad@kernel.org> wrote:
> > --- a/xen/arch/x86/microcode.c
> > +++ b/xen/arch/x86/microcode.c
> > @@ -433,24 +433,40 @@ static int __init microcode_presmp_init(void)
> >      {
> >          if ( ucode_mod.mod_end || ucode_blob.size )
> >          {
> > -            void *data;
> > -            size_t len;
> > -
> > -            if ( ucode_blob.size )
> > -            {
> > -                len = ucode_blob.size;
> > -                data = ucode_blob.data;
> > -            }
> > -            else
> > -            {
> > -                len = ucode_mod.mod_end;
> > -                data = ucode_mod_map(&ucode_mod);
> > -            }
> > -            if ( data )
> > -                microcode_update_cpu(data, len);
> > -
> > -            if ( !ucode_blob.size )
> > -                ucode_mod_map(NULL);
> > +            int rc;
> > +            do {
> > +                void *data = NULL;
> > +                size_t len;
> > +
> > +                rc = 0;
> > +                if ( ucode_blob.size )
> > +                {
> > +                    len = ucode_blob.size;
> > +                    data = ucode_blob.data;
> > +                }
> > +                else if ( ucode_mod.mod_end )
> > +                {
> > +                    len = ucode_mod.mod_end;
> > +                    data = ucode_mod_map(&ucode_mod);
> > +                }
> > +                if ( data )
> > +                    rc = microcode_update_cpu(data, len);
> 
> else rc = -ENOMEM (yielding the initialization above pointless).
> 
> > +
> > +                if ( !ucode_blob.size && ucode_mod.mod_end )
> > +                    ucode_mod_map(NULL);
> > +
> > +                if ( rc == -EINVAL )
> 
> if ( rc )
> 
> > +                {
> > +                    if ( ucode_blob.size ) /* That was tried first */
> > +                    {
> > +                        ucode_blob.size = 0;
> > +                        xfree(ucode_blob.data);
> > +                        continue;
> > +                    }
> > +                    if ( ucode_mod.mod_end )
> > +                        ucode_mod.mod_end = 0;
> > +                }
> > +            } while ( rc );
> >          }
> >  
> >          register_cpu_notifier(&microcode_percpu_nfb);
> 
> This may need re-doing anyway I we can agree that allowing both
> location specifications at the same time is bogus and should hence
> be dropped.
Right. Lets continue the discussion on that "both location
specifications" on the first patch and I can redo this based on
the result of that discussion.> 
> Jan
> 
>

Jan Beulich

2013-Aug-08 07:07 UTC

head link

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

>>> On 08.08.13 at 02:50, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> 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.
As said elsewhere - which of the two is "better" can''t be
known.
Hence let''s avoid the ambiguity (and possible surprise) by being
explicit in allowing either, but not both at the same time.
>> 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?
Well, "scan" is a little too wide a term. Otoh you don''t
really look
at only the initrd, you indeed scan all modules, so perhaps "scan"
indeed is the right name. The only thing I''m (mildly) worried about
then is how we''d name an eventual extension to scan other than
cpio format modules.
>> > +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?
Fine with me. The main request really was to get rid of the
array that''s never really used as such.
>> >  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?
So what if the first try finds a usable ucode update (newer than
what got loaded by firmware), yet the second one would have
been even more recent? Of course you could fully run through
both update cycles, but again - I''d favor avoiding the ambiguity
and explicitly allow only for either method.

Jan

Xen devel - Jul 2013 - Scan initramfs for microcode cpio archive. (v2)

Scan initramfs for microcode cpio archive. (v2)

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

Re: Scan initramfs for microcode cpio archive. (v2)

[PATCH] microcode: Check whether the microcode is correct.

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

Re: [PATCH] microcode: Check whether the microcode is correct.

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

Re: [PATCH] microcode: Check whether the microcode is correct.

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