Xen devel - Sep 2013 - [PATCH] microcode: Scan the multiboot payloads for cpio format microcode blob. (v3).

Changelog, since v2 posting
(http://mid.gmane.org/1374165371-24716-1-git-send-email-konrad.wilk@oracle.com)
 - Addressed Jan''s comments (I hope all of them)

Greetings,

<from v1>
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 CANNOT 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.
</from v1>

There is also the question whether the parameter should be
''cpio'',''initrd''
or ''scan''. As in the future the extraction of the payload
could be from
a different format than the cpio (say a microcode blob with an magic
string at the start). The author believes that at that time the logic
to scan the mulitboot payloads can be expanded to also scan formats other
than cpio format.

These patches are also available at:

  git://xenbits.xen.org/people/konradwilk/xen.git microcode.v3

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

Konrad Rzeszutek Wilk (2):
      microcode: Scan the initramfs payload for microcode blob.
      microcode: Check whether the microcode is correct.

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:
 ''scan''      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 only
allowing
either parameter:
  ucode=[<index>|scan]

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]
[v4: Review comments from Jan]
---
 docs/misc/xen-command-line.markdown |  14 +++-
 xen/arch/x86/microcode.c            | 161 ++++++++++++++++++++++++++++++++----
 xen/common/Makefile                 |   2 +-
 xen/common/earlycpio.c              | 151 +++++++++++++++++++++++++++++++++
 xen/include/xen/earlycpio.h         |  14 ++++
 5 files changed, 322 insertions(+), 20 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 56c9729..aabd0d4 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -897,10 +897,12 @@ pages) must also be specified via the tbuf\_size
parameter.
 > `= unstable | skewed`
 
 ### ucode
-> `= <integer>`
+> `= <integer> , scan`
+
+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
@@ -910,6 +912,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)).
 
+''scan'' 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..8136d44 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -39,25 +39,131 @@
 #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>|scan]''. Both options are
optional.
+ * If the EFI has forced which of the multiboot payloads is to be used,
+ * no parsing will be attempted.
+ */
 static void __init parse_ucode(char *s)
 {
-    if ( !ucode_mod_forced )
-        ucode_mod_idx = simple_strtol(s, NULL, 0);
+    if ( ucode_mod_forced ) /* Forced by EFI */
+       return;
+
+    ucode_mod_idx = simple_strtol(s, NULL, 0);
+
+    if ( ucode_mod_idx == 0 && !strncmp(s, "scan", 4) )
+        ucode_scan = 1;
 }
 custom_param("ucode", parse_ucode);
 
+/*
+ * 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 = "kernel/x86/microcode/AuthenticAMD.bin";
+    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+        p = "kernel/x86/microcode/GenuineIntel.bin";
+    else
+        return;
+
+    /*
+     * Try all modules and see whichever could be the microcode blob.
+     */
+    for ( i = 1 /* Ignore dom0 kernel */; 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);
+            continue;
+        }
+        cd.data = NULL;
+        cd.size = 0;
+        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore
*/);
+        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 )
+                    cd.data = NULL;
+                else
+                    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 +175,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 +345,10 @@ 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_blob.size ? ucode_blob.size : ucode_mod.mod_end;
+
+    microcode_update_cpu(_data, len);
     cpumask_set_cpu(smp_processor_id(), &init_mask);
 }
 
@@ -246,18 +358,19 @@ 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);
+    data = ucode_blob.size ? ucode_blob.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 +382,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 +415,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 5486140..6da4651 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -49,7 +49,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
earlycpio,$(n).init.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..5e54142
--- /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 __init 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);
+			}
+			if (ch[C_NAMESIZE] - 1 /* includes \0 */ == mypathsize) {
+				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..85d144a
--- /dev/null
+++ b/xen/include/xen/earlycpio.h
@@ -0,0 +1,14 @@
+#ifndef _EARLYCPIO_H
+#define _EARLYCPIO_H
+
+#define MAX_CPIO_FILE_NAME 18
+
+struct cpio_data {
+	void *data;
+	size_t size;
+};
+
+struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
+				long *offset);
+
+#endif /* _EARLYCPIO_H */
-- 
1.8.3.1

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.

This patch handles this odd situation.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Redid it as we are not doing scan and <index> at the same time]
---
 xen/arch/x86/microcode.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 8136d44..0bc95e6 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -419,6 +419,7 @@ static int __init microcode_presmp_init(void)
         {
             void *data;
             size_t len;
+            int rc = 0;
 
             if ( ucode_blob.size )
             {
@@ -431,10 +432,23 @@ static int __init microcode_presmp_init(void)
                 data = ucode_mod_map(&ucode_mod);
             }
             if ( data )
-                microcode_update_cpu(data, len);
+                rc = microcode_update_cpu(data, len);
+            else
+                rc = -ENOMEM;
 
             if ( !ucode_blob.size )
                 ucode_mod_map(NULL);
+
+            if ( rc )
+            {
+                if ( ucode_blob.size )
+                {
+                    xfree(ucode_blob.data);
+                    ucode_blob.size = 0;
+                    ucode_blob.data = NULL;
+                } else
+                    ucode_mod.mod_end = 0;
+            }
         }
 
         register_cpu_notifier(&microcode_percpu_nfb);
-- 
1.8.3.1

On 25/09/13 13:57, Konrad Rzeszutek Wilk wrote:
> 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:
>  ''scan''      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 only
allowing
> either parameter:
>   ucode=[<index>|scan]
>
> 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]
> [v4: Review comments from Jan]
> ---
>  docs/misc/xen-command-line.markdown |  14 +++-
>  xen/arch/x86/microcode.c            | 161
++++++++++++++++++++++++++++++++----
>  xen/common/Makefile                 |   2 +-
>  xen/common/earlycpio.c              | 151
+++++++++++++++++++++++++++++++++
>  xen/include/xen/earlycpio.h         |  14 ++++
>  5 files changed, 322 insertions(+), 20 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 56c9729..aabd0d4 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -897,10 +897,12 @@ pages) must also be specified via the tbuf\_size
parameter.
>  > `= unstable | skewed`
>  
>  ### ucode
> -> `= <integer>`
> +> `= <integer> , scan`
> +
> +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,
sp. integer
> +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
> @@ -910,6 +912,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)).
>  
> +''scan'' 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..8136d44 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -39,25 +39,131 @@
>  #include <asm/setup.h>
>  #include <asm/microcode.h>
>  
> +#include <xen/earlycpio.h>
> +
This include should probably be up a bit with the previous #include
<xen/* bunch
>  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>|scan]''. Both options
are optional.
> + * If the EFI has forced which of the multiboot payloads is to be used,
> + * no parsing will be attempted.
> + */
>  static void __init parse_ucode(char *s)
>  {
> -    if ( !ucode_mod_forced )
> -        ucode_mod_idx = simple_strtol(s, NULL, 0);
> +    if ( ucode_mod_forced ) /* Forced by EFI */
> +       return;
> +
> +    ucode_mod_idx = simple_strtol(s, NULL, 0);
> +
> +    if ( ucode_mod_idx == 0 && !strncmp(s, "scan", 4) )
> +        ucode_scan = 1;
This code matches neither the description in the comment above, nor the
description in the command line markdown document.

Realistically, "scan" is mutually exclusive with specifying an index.

Might it be better to have:

if ( !strncmp(s, "scan", 4) )
  ucode_scan = 1;
else
  ucode_mod_index = simple_strtol(s, NULL, 0);

~Andrew
>  }
>  custom_param("ucode", parse_ucode);
>  
> +/*
> + * 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 = "kernel/x86/microcode/AuthenticAMD.bin";
> +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +        p = "kernel/x86/microcode/GenuineIntel.bin";
> +    else
> +        return;
> +
> +    /*
> +     * Try all modules and see whichever could be the microcode blob.
> +     */
> +    for ( i = 1 /* Ignore dom0 kernel */; 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);
> +            continue;
> +        }
> +        cd.data = NULL;
> +        cd.size = 0;
> +        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /*
ignore */);
> +        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 )
> +                    cd.data = NULL;
> +                else
> +                    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 +175,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 +345,10 @@ 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_blob.size ? ucode_blob.size : ucode_mod.mod_end;
> +
> +    microcode_update_cpu(_data, len);
>      cpumask_set_cpu(smp_processor_id(), &init_mask);
>  }
>  
> @@ -246,18 +358,19 @@ 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);
> +    data = ucode_blob.size ? ucode_blob.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 +382,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 +415,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 5486140..6da4651 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -49,7 +49,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
earlycpio,$(n).init.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..5e54142
> --- /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 __init 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);
> +			}
> +			if (ch[C_NAMESIZE] - 1 /* includes \0 */ == mypathsize) {
> +				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..85d144a
> --- /dev/null
> +++ b/xen/include/xen/earlycpio.h
> @@ -0,0 +1,14 @@
> +#ifndef _EARLYCPIO_H
> +#define _EARLYCPIO_H
> +
> +#define MAX_CPIO_FILE_NAME 18
> +
> +struct cpio_data {
> +	void *data;
> +	size_t size;
> +};
> +
> +struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
> +				long *offset);
> +
> +#endif /* _EARLYCPIO_H */

On 25/09/13 13:57, Konrad Rzeszutek Wilk wrote:
> 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.
>
> This patch handles this odd situation.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
While not strictly relevant to this patch, I simply noticed when
reviewing it that ucode_blob is __initdata.  Given the comment "The
cached microcode patch is applied when CPUs resume from a sleep state."
in the previous patch, what is the policy for reloading microcode after
deep sleep / cpu hotplug ?
> [v2: Redid it as we are not doing scan and <index> at the same time]
> ---
>  xen/arch/x86/microcode.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 8136d44..0bc95e6 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -419,6 +419,7 @@ static int __init microcode_presmp_init(void)
>          {
>              void *data;
>              size_t len;
> +            int rc = 0;
>  
>              if ( ucode_blob.size )
>              {
> @@ -431,10 +432,23 @@ static int __init microcode_presmp_init(void)
>                  data = ucode_mod_map(&ucode_mod);
>              }
>              if ( data )
> -                microcode_update_cpu(data, len);
> +                rc = microcode_update_cpu(data, len);
> +            else
> +                rc = -ENOMEM;
>  
>              if ( !ucode_blob.size )
>                  ucode_mod_map(NULL);
> +
> +            if ( rc )
> +            {
> +                if ( ucode_blob.size )
> +                {
> +                    xfree(ucode_blob.data);
> +                    ucode_blob.size = 0;
> +                    ucode_blob.data = NULL;
> +                } else
> +                    ucode_mod.mod_end = 0;
> +            }
>          }
>  
>          register_cpu_notifier(&microcode_percpu_nfb);

On Wed, Sep 25, 2013 at 02:13:40PM +0100, Andrew Cooper
wrote:
> On 25/09/13 13:57, Konrad Rzeszutek Wilk wrote:
> > 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.
> >
> > This patch handles this odd situation.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> While not strictly relevant to this patch, I simply noticed when
> reviewing it that ucode_blob is __initdata.  Given the comment "The
> cached microcode patch is applied when CPUs resume from a sleep
state."
> in the previous patch, what is the policy for reloading microcode after
> deep sleep / cpu hotplug ?
The underlaying architecture microcode (microcode_intel or microcode_amd)
end up saving it in ''struct ucode_cpu_info'' which is a per-cpu
data
structure (see ucode_cpu_info). They end up saving it when doing the
pre-SMP (for CPU0) and SMP (for the rest) microcode loading.

And naturally if one does a hypercall to update the microcode and it is
newer, then the old per-cpu data is replaced.

Would you like me to update the git commit description with
this?
> 
> > [v2: Redid it as we are not doing scan and <index> at the same
time]
> > ---
> >  xen/arch/x86/microcode.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> > index 8136d44..0bc95e6 100644
> > --- a/xen/arch/x86/microcode.c
> > +++ b/xen/arch/x86/microcode.c
> > @@ -419,6 +419,7 @@ static int __init microcode_presmp_init(void)
> >          {
> >              void *data;
> >              size_t len;
> > +            int rc = 0;
> >  
> >              if ( ucode_blob.size )
> >              {
> > @@ -431,10 +432,23 @@ static int __init microcode_presmp_init(void)
> >                  data = ucode_mod_map(&ucode_mod);
> >              }
> >              if ( data )
> > -                microcode_update_cpu(data, len);
> > +                rc = microcode_update_cpu(data, len);
> > +            else
> > +                rc = -ENOMEM;
> >  
> >              if ( !ucode_blob.size )
> >                  ucode_mod_map(NULL);
> > +
> > +            if ( rc )
> > +            {
> > +                if ( ucode_blob.size )
> > +                {
> > +                    xfree(ucode_blob.data);
> > +                    ucode_blob.size = 0;
> > +                    ucode_blob.data = NULL;
> > +                } else
> > +                    ucode_mod.mod_end = 0;
> > +            }
> >          }
> >  
> >          register_cpu_notifier(&microcode_percpu_nfb);
>

On 25/09/13 14:59, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 25, 2013 at 02:13:40PM +0100, Andrew Cooper wrote:
>> On 25/09/13 13:57, Konrad Rzeszutek Wilk wrote:
>>> 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.
>>>
>>> This patch handles this odd situation.
>>>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> While not strictly relevant to this patch, I simply noticed when
>> reviewing it that ucode_blob is __initdata.  Given the comment
"The
>> cached microcode patch is applied when CPUs resume from a sleep
state."
>> in the previous patch, what is the policy for reloading microcode after
>> deep sleep / cpu hotplug ?
> The underlaying architecture microcode (microcode_intel or microcode_amd)
> end up saving it in ''struct ucode_cpu_info'' which is a
per-cpu data
> structure (see ucode_cpu_info). They end up saving it when doing the
> pre-SMP (for CPU0) and SMP (for the rest) microcode loading.
>
> And naturally if one does a hypercall to update the microcode and it is
> newer, then the old per-cpu data is replaced.
Ah ok - I missed that.
>
> Would you like me to update the git commit description with this?
It might be worth having a note to this effect in the commit message of
the previous patch.

~Andrew
>>> [v2: Redid it as we are not doing scan and <index> at the
same time]
>>> ---
>>>  xen/arch/x86/microcode.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>>> index 8136d44..0bc95e6 100644
>>> --- a/xen/arch/x86/microcode.c
>>> +++ b/xen/arch/x86/microcode.c
>>> @@ -419,6 +419,7 @@ static int __init microcode_presmp_init(void)
>>>          {
>>>              void *data;
>>>              size_t len;
>>> +            int rc = 0;
>>>  
>>>              if ( ucode_blob.size )
>>>              {
>>> @@ -431,10 +432,23 @@ static int __init microcode_presmp_init(void)
>>>                  data = ucode_mod_map(&ucode_mod);
>>>              }
>>>              if ( data )
>>> -                microcode_update_cpu(data, len);
>>> +                rc = microcode_update_cpu(data, len);
>>> +            else
>>> +                rc = -ENOMEM;
>>>  
>>>              if ( !ucode_blob.size )
>>>                  ucode_mod_map(NULL);
>>> +
>>> +            if ( rc )
>>> +            {
>>> +                if ( ucode_blob.size )
>>> +                {
>>> +                    xfree(ucode_blob.data);
>>> +                    ucode_blob.size = 0;
>>> +                    ucode_blob.data = NULL;
>>> +                } else
>>> +                    ucode_mod.mod_end = 0;
>>> +            }
>>>          }
>>>  
>>>          register_cpu_notifier(&microcode_percpu_nfb);

. snip..
> > -        ucode_mod_idx = simple_strtol(s, NULL, 0);
> > +    if ( ucode_mod_forced ) /* Forced by EFI */
> > +       return;
> > +
> > +    ucode_mod_idx = simple_strtol(s, NULL, 0);
> > +
> > +    if ( ucode_mod_idx == 0 && !strncmp(s, "scan",
4) )
> > +        ucode_scan = 1;
> 
> This code matches neither the description in the comment above, nor the
> description in the command line markdown document.
> 
> Realistically, "scan" is mutually exclusive with specifying an
index.
> 
> Might it be better to have:
> 
> if ( !strncmp(s, "scan", 4) )
>   ucode_scan = 1;
> else
>   ucode_mod_index = simple_strtol(s, NULL, 0);
> 
> ~Andrew
.. snip.. How does this patch look like (in microcode.v3.1 branch):

From 586e18b79f9db8de460ea1bfc66d5a2f8b5cec04 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 10 Jul 2013 16:49:30 -0400
Subject: [PATCH 1/2] 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:
 ''scan''      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 only
allowing
either parameter:
  ucode=[<index>|scan]

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]
[v4: Review comments from Jan]
[v5: Review comments from Andrew]
---
 docs/misc/xen-command-line.markdown |  14 +++-
 xen/arch/x86/microcode.c            | 158 ++++++++++++++++++++++++++++++++----
 xen/common/Makefile                 |   2 +-
 xen/common/earlycpio.c              | 151 ++++++++++++++++++++++++++++++++++
 xen/include/xen/earlycpio.h         |  14 ++++
 5 files changed, 320 insertions(+), 19 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 56c9729..60892d6 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -897,10 +897,12 @@ pages) must also be specified via the tbuf\_size
parameter.
 > `= unstable | skewed`
 
 ### ucode
-> `= <integer>`
+> `= <integer> , scan`
+
+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
+''integer'' 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
@@ -910,6 +912,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)).
 
+''scan'' 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..e6344cf 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -33,6 +33,7 @@
 #include <xen/spinlock.h>
 #include <xen/tasklet.h>
 #include <xen/guest_access.h>
+#include <xen/earlycpio.h>
 
 #include <asm/msr.h>
 #include <asm/processor.h>
@@ -45,19 +46,123 @@ 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>|scan]''. Both options are
optional.
+ * If the EFI has forced which of the multiboot payloads is to be used,
+ * no parsing will be attempted.
+ */
 static void __init parse_ucode(char *s)
 {
-    if ( !ucode_mod_forced )
+    if ( ucode_mod_forced ) /* Forced by EFI */
+       return;
+
+    if ( !strncmp(s, "scan", 4) )
+        ucode_scan = 1;
+    else
         ucode_mod_idx = simple_strtol(s, NULL, 0);
 }
 custom_param("ucode", parse_ucode);
 
+/*
+ * 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 = "kernel/x86/microcode/AuthenticAMD.bin";
+    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+        p = "kernel/x86/microcode/GenuineIntel.bin";
+    else
+        return;
+
+    /*
+     * Try all modules and see whichever could be the microcode blob.
+     */
+    for ( i = 1 /* Ignore dom0 kernel */; 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);
+            continue;
+        }
+        cd.data = NULL;
+        cd.size = 0;
+        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore
*/);
+        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 )
+                    cd.data = NULL;
+                else
+                    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 +174,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 +344,10 @@ 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_blob.size ? ucode_blob.size : ucode_mod.mod_end;
+
+    microcode_update_cpu(_data, len);
     cpumask_set_cpu(smp_processor_id(), &init_mask);
 }
 
@@ -246,18 +357,19 @@ 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);
+    data = ucode_blob.size ? ucode_blob.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 +381,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 +414,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 5486140..6da4651 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -49,7 +49,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
earlycpio,$(n).init.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..5e54142
--- /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 __init 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);
+			}
+			if (ch[C_NAMESIZE] - 1 /* includes \0 */ == mypathsize) {
+				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..85d144a
--- /dev/null
+++ b/xen/include/xen/earlycpio.h
@@ -0,0 +1,14 @@
+#ifndef _EARLYCPIO_H
+#define _EARLYCPIO_H
+
+#define MAX_CPIO_FILE_NAME 18
+
+struct cpio_data {
+	void *data;
+	size_t size;
+};
+
+struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
+				long *offset);
+
+#endif /* _EARLYCPIO_H */
-- 
1.8.3.1

On 25/09/13 15:11, Konrad Rzeszutek Wilk wrote:
> . snip..
>>> -        ucode_mod_idx = simple_strtol(s, NULL, 0);
>>> +    if ( ucode_mod_forced ) /* Forced by EFI */
>>> +       return;
>>> +
>>> +    ucode_mod_idx = simple_strtol(s, NULL, 0);
>>> +
>>> +    if ( ucode_mod_idx == 0 && !strncmp(s,
"scan", 4) )
>>> +        ucode_scan = 1;
>> This code matches neither the description in the comment above, nor the
>> description in the command line markdown document.
>>
>> Realistically, "scan" is mutually exclusive with specifying
an index.
>>
>> Might it be better to have:
>>
>> if ( !strncmp(s, "scan", 4) )
>>   ucode_scan = 1;
>> else
>>   ucode_mod_index = simple_strtol(s, NULL, 0);
>>
>> ~Andrew
> .. snip.. How does this patch look like (in microcode.v3.1 branch):
>
> From 586e18b79f9db8de460ea1bfc66d5a2f8b5cec04 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 10 Jul 2013 16:49:30 -0400
> Subject: [PATCH 1/2] 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:
>  ''scan''      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 only
allowing
> either parameter:
>   ucode=[<index>|scan]
>
> 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]
> [v4: Review comments from Jan]
> [v5: Review comments from Andrew]
> ---
>  docs/misc/xen-command-line.markdown |  14 +++-
>  xen/arch/x86/microcode.c            | 158
++++++++++++++++++++++++++++++++----
>  xen/common/Makefile                 |   2 +-
>  xen/common/earlycpio.c              | 151
++++++++++++++++++++++++++++++++++
>  xen/include/xen/earlycpio.h         |  14 ++++
>  5 files changed, 320 insertions(+), 19 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 56c9729..60892d6 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -897,10 +897,12 @@ pages) must also be specified via the tbuf\_size
parameter.
>  > `= unstable | skewed`
>  
>  ### ucode
> -> `= <integer>`
> +> `= <integer> , scan`
+> `= [<integer> | scan]`

~Andrew
> +
> +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
> +''integer'' 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
> @@ -910,6 +912,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)).
>  
> +''scan'' 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..e6344cf 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -33,6 +33,7 @@
>  #include <xen/spinlock.h>
>  #include <xen/tasklet.h>
>  #include <xen/guest_access.h>
> +#include <xen/earlycpio.h>
>  
>  #include <asm/msr.h>
>  #include <asm/processor.h>
> @@ -45,19 +46,123 @@ 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>|scan]''. Both options
are optional.
> + * If the EFI has forced which of the multiboot payloads is to be used,
> + * no parsing will be attempted.
> + */
>  static void __init parse_ucode(char *s)
>  {
> -    if ( !ucode_mod_forced )
> +    if ( ucode_mod_forced ) /* Forced by EFI */
> +       return;
> +
> +    if ( !strncmp(s, "scan", 4) )
> +        ucode_scan = 1;
> +    else
>          ucode_mod_idx = simple_strtol(s, NULL, 0);
>  }
>  custom_param("ucode", parse_ucode);
>  
> +/*
> + * 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 = "kernel/x86/microcode/AuthenticAMD.bin";
> +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +        p = "kernel/x86/microcode/GenuineIntel.bin";
> +    else
> +        return;
> +
> +    /*
> +     * Try all modules and see whichever could be the microcode blob.
> +     */
> +    for ( i = 1 /* Ignore dom0 kernel */; 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);
> +            continue;
> +        }
> +        cd.data = NULL;
> +        cd.size = 0;
> +        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /*
ignore */);
> +        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 )
> +                    cd.data = NULL;
> +                else
> +                    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 +174,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 +344,10 @@ 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_blob.size ? ucode_blob.size : ucode_mod.mod_end;
> +
> +    microcode_update_cpu(_data, len);
>      cpumask_set_cpu(smp_processor_id(), &init_mask);
>  }
>  
> @@ -246,18 +357,19 @@ 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);
> +    data = ucode_blob.size ? ucode_blob.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 +381,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 +414,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 5486140..6da4651 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -49,7 +49,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
earlycpio,$(n).init.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..5e54142
> --- /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 __init 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);
> +			}
> +			if (ch[C_NAMESIZE] - 1 /* includes \0 */ == mypathsize) {
> +				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..85d144a
> --- /dev/null
> +++ b/xen/include/xen/earlycpio.h
> @@ -0,0 +1,14 @@
> +#ifndef _EARLYCPIO_H
> +#define _EARLYCPIO_H
> +
> +#define MAX_CPIO_FILE_NAME 18
> +
> +struct cpio_data {
> +	void *data;
> +	size_t size;
> +};
> +
> +struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
> +				long *offset);
> +
> +#endif /* _EARLYCPIO_H */

On 09/25/2013 05:57 AM, Konrad Rzeszutek Wilk wrote:
> The way to use this is by the ''ucode'' parameter. It has
now two meanings:
>   [<index>|initrd]
>
> Which CANNOT 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".
> [...]
> There is also the question whether the parameter should be
''cpio'',''initrd''
> or ''scan''. As in the future the extraction of the payload
could be from
> a different format than the cpio (say a microcode blob with an magic
> string at the start). The author believes that at that time the logic
> to scan the mulitboot payloads can be expanded to also scan formats other
> than cpio format.
Why treat a microcode.bin and cpio differently? Aren''t they both just
file formats that can be parsed to (possibly) extract microcode update
info? That being so, why change the ucode parameter? Wouldn''t you just
set it to ucode=N, where N is the module index either a microcode.bin or
cpio or anything else multiboot module?

    J

>
> These patches are also available at:
>
>   git://xenbits.xen.org/people/konradwilk/xen.git microcode.v3
>
>  docs/misc/xen-command-line.markdown |  14 ++-
>  xen/arch/x86/microcode.c            | 177
++++++++++++++++++++++++++++++++----
>  xen/common/Makefile                 |   2 +-
>  xen/common/earlycpio.c              | 151 ++++++++++++++++++++++++++++++
>  xen/include/xen/earlycpio.h         |  14 +++
>  5 files changed, 337 insertions(+), 21 deletions(-)
>
> Konrad Rzeszutek Wilk (2):
>       microcode: Scan the initramfs payload for microcode blob.
>       microcode: Check whether the microcode is correct.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On Wed, Sep 25, 2013 at 12:36:01PM -0700, Jeremy Fitzhardinge
wrote:
> On 09/25/2013 05:57 AM, Konrad Rzeszutek Wilk wrote:
> > The way to use this is by the ''ucode'' parameter. It
has now two meanings:
> >   [<index>|initrd]
> >
> > Which CANNOT 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".
> > [...]
> > There is also the question whether the parameter should be
''cpio'',''initrd''
> > or ''scan''. As in the future the extraction of the
payload could be from
> > a different format than the cpio (say a microcode blob with an magic
> > string at the start). The author believes that at that time the logic
> > to scan the mulitboot payloads can be expanded to also scan formats
other
> > than cpio format.
> 
> Why treat a microcode.bin and cpio differently? Aren''t they both
just
> file formats that can be parsed to (possibly) extract microcode update
> info? That being so, why change the ucode parameter? Wouldn''t you
just
Unfortunatly no. The blobs are blobs that have no signature unless
you are an microcode driver and know what to look for.

The cpio format provides a means of identifying (the name of the file
in the archive matches a specific string) the microcode blob.
> set it to ucode=N, where N is the module index either a microcode.bin or
> cpio or anything else multiboot module?
That is an option too. It would mean re-organizing the code more as
the existing ''index'' ucode grabs the multiboot payload and
does not
allow the Linux kernel access to it. Would have to relax that somehow.

Lastly with the cpio format the microcode blob can be in initframfs.cpio
or in a seperate cpio archive (so two cpio archives along with Linux kernel).

Besides that, from the GRUB perspective it makes it much easier to support
as the grub tools can just add ''ucode=scan'' and not worry
about indexing
in the right payload.

Or that is me being lazy. I would rather have this thing be automatic and
be on by default and scan all the images and pluck the data out. No
dependency on anything at that point (which is what Linux does right now).
> 
>     J
> 
> 
> >
> > These patches are also available at:
> >
> >   git://xenbits.xen.org/people/konradwilk/xen.git microcode.v3
> >
> >  docs/misc/xen-command-line.markdown |  14 ++-
> >  xen/arch/x86/microcode.c            | 177
++++++++++++++++++++++++++++++++----
> >  xen/common/Makefile                 |   2 +-
> >  xen/common/earlycpio.c              | 151
++++++++++++++++++++++++++++++
> >  xen/include/xen/earlycpio.h         |  14 +++
> >  5 files changed, 337 insertions(+), 21 deletions(-)
> >
> > Konrad Rzeszutek Wilk (2):
> >       microcode: Scan the initramfs payload for microcode blob.
> >       microcode: Check whether the microcode is correct.
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> >
>

On 09/25/2013 01:03 PM, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 25, 2013 at 12:36:01PM -0700, Jeremy Fitzhardinge wrote:
>> On 09/25/2013 05:57 AM, Konrad Rzeszutek Wilk wrote:
>>> The way to use this is by the ''ucode'' parameter.
It has now two meanings:
>>>   [<index>|initrd]
>>>
>>> Which CANNOT 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".
>>> [...]
>>> There is also the question whether the parameter should be
''cpio'',''initrd''
>>> or ''scan''. As in the future the extraction of the
payload could be from
>>> a different format than the cpio (say a microcode blob with an
magic
>>> string at the start). The author believes that at that time the
logic
>>> to scan the mulitboot payloads can be expanded to also scan formats
other
>>> than cpio format.
>> Why treat a microcode.bin and cpio differently? Aren''t they
both just
>> file formats that can be parsed to (possibly) extract microcode update
>> info? That being so, why change the ucode parameter? Wouldn''t
you just
> Unfortunatly no. The blobs are blobs that have no signature unless
> you are an microcode driver and know what to look for.
Right, but that''s what we''re talking about here isn''t
it?
> The cpio format provides a means of identifying (the name of the file
> in the archive matches a specific string) the microcode blob.
Yep.
>> set it to ucode=N, where N is the module index either a microcode.bin
or
>> cpio or anything else multiboot module?
> That is an option too. It would mean re-organizing the code more as
> the existing ''index'' ucode grabs the multiboot payload
and does not
> allow the Linux kernel access to it. Would have to relax that somehow.
>
> Lastly with the cpio format the microcode blob can be in initframfs.cpio
> or in a seperate cpio archive (so two cpio archives along with Linux
kernel).
>
> Besides that, from the GRUB perspective it makes it much easier to support
> as the grub tools can just add ''ucode=scan'' and not worry
about indexing
> in the right payload.
>
> Or that is me being lazy. I would rather have this thing be automatic and
> be on by default and scan all the images and pluck the data out. No
> dependency on anything at that point (which is what Linux does right now).
Something completely automatic would be ideal, I agree. I just went
through the process of setting up ucode with the microcode.bin blob, and
while its straightforward I suspect some update will break it
inadventently, so something that is more in line with what Linux itself
wants to do is good.

It just occurred to me; I haven''t dug into the existing or new code to
see whether I''m really making things more complex.

    J

>
>>     J
>>
>>
>>> These patches are also available at:
>>>
>>>   git://xenbits.xen.org/people/konradwilk/xen.git microcode.v3
>>>
>>>  docs/misc/xen-command-line.markdown |  14 ++-
>>>  xen/arch/x86/microcode.c            | 177
++++++++++++++++++++++++++++++++----
>>>  xen/common/Makefile                 |   2 +-
>>>  xen/common/earlycpio.c              | 151
++++++++++++++++++++++++++++++
>>>  xen/include/xen/earlycpio.h         |  14 +++
>>>  5 files changed, 337 insertions(+), 21 deletions(-)
>>>
>>> Konrad Rzeszutek Wilk (2):
>>>       microcode: Scan the initramfs payload for microcode blob.
>>>       microcode: Check whether the microcode is correct.
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>>

Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>On 09/25/2013 01:03 PM, Konrad Rzeszutek Wilk wrote:
>> On Wed, Sep 25, 2013 at 12:36:01PM -0700, Jeremy Fitzhardinge wrote:
>>> On 09/25/2013 05:57 AM, Konrad Rzeszutek Wilk wrote:
>>>> The way to use this is by the ''ucode''
parameter. It has now two
>meanings:
>>>>   [<index>|initrd]
>>>>
>>>> Which CANNOT 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".
>>>> [...]
>>>> There is also the question whether the parameter should be
>''cpio'',''initrd''
>>>> or ''scan''. As in the future the extraction of
the payload could be
>from
>>>> a different format than the cpio (say a microcode blob with an
>magic
>>>> string at the start). The author believes that at that time the
>logic
>>>> to scan the mulitboot payloads can be expanded to also scan
formats
>other
>>>> than cpio format.
>>> Why treat a microcode.bin and cpio differently? Aren''t
they both
>just
>>> file formats that can be parsed to (possibly) extract microcode
>update
>>> info? That being so, why change the ucode parameter?
Wouldn''t you
>just
>> Unfortunatly no. The blobs are blobs that have no signature unless
>> you are an microcode driver and know what to look for.
>
>Right, but that''s what we''re talking about here
isn''t it?
>
>> The cpio format provides a means of identifying (the name of the file
>> in the archive matches a specific string) the microcode blob.
>
>Yep.
>
>>> set it to ucode=N, where N is the module index either a
>microcode.bin or
>>> cpio or anything else multiboot module?
>> That is an option too. It would mean re-organizing the code more as
>> the existing ''index'' ucode grabs the multiboot
payload and does not
>> allow the Linux kernel access to it. Would have to relax that
>somehow.
>>
>> Lastly with the cpio format the microcode blob can be in
>initframfs.cpio
>> or in a seperate cpio archive (so two cpio archives along with Linux
>kernel).
>>
>> Besides that, from the GRUB perspective it makes it much easier to
>support
>> as the grub tools can just add ''ucode=scan'' and not
worry about
>indexing
>> in the right payload.
>>
>> Or that is me being lazy. I would rather have this thing be automatic
>and
>> be on by default and scan all the images and pluck the data out. No
>> dependency on anything at that point (which is what Linux does right
>now).
>
>Something completely automatic would be ideal, I agree. I just went
>through the process of setting up ucode with the microcode.bin blob,
>and
>while its straightforward I suspect some update will break it
>inadventently, so something that is more in line with what Linux itself
>wants to do is good.
>
>It just occurred to me; I haven''t dug into the existing or new code
to
>see whether I''m really making things more complex.
Use the latest version of dracut with --something-microcode and it will attach
the microcode blob to initramfs automatically.
>
>    J
>
>
>>
>>>     J
>>>
>>>
>>>> These patches are also available at:
>>>>
>>>>   git://xenbits.xen.org/people/konradwilk/xen.git microcode.v3
>>>>
>>>>  docs/misc/xen-command-line.markdown |  14 ++-
>>>>  xen/arch/x86/microcode.c            | 177
>++++++++++++++++++++++++++++++++----
>>>>  xen/common/Makefile                 |   2 +-
>>>>  xen/common/earlycpio.c              | 151
>++++++++++++++++++++++++++++++
>>>>  xen/include/xen/earlycpio.h         |  14 +++
>>>>  5 files changed, 337 insertions(+), 21 deletions(-)
>>>>
>>>> Konrad Rzeszutek Wilk (2):
>>>>       microcode: Scan the initramfs payload for microcode blob.
>>>>       microcode: Check whether the microcode is correct.
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>>>>