Konrad Rzeszutek Wilk
2013-Jul-12 14:25 UTC
[RFC PATCH] Scan initramfs for microcode cpio archive. (v1)
Greetings,
Please see the following patch which implements a mechanism to scan
the initramfs for the format of an microcode files. This is a feature
that the Linux kernel has since v3.10 - where it searches in the
initramfs for an archive of the microcode blob. The format is documented
in the Linux tree and the commit description contains it.
The tool to make this work is the initramfs creator. The author has
provided an RFC patch:
http://article.gmane.org/gmane.linux.kernel.initramfs/3318
which does that.
That, along with this patch, allows the Xen hypervisor to update the
microcode during bootup. Please review attached RFC patch.
docs/misc/xen-command-line.markdown | 12 +++
xen/arch/x86/microcode.c | 160 ++++++++++++++++++++++++++++++++---
xen/common/Makefile | 2 +-
xen/common/earlycpio.c | 151 +++++++++++++++++++++++++++++++++
xen/include/xen/earlycpio.h | 15 +++
5 files changed, 326 insertions(+), 14 deletions(-)
Konrad Rzeszutek Wilk (1):
microcode: Scan the initramfs payload for microcode blob.
Konrad Rzeszutek Wilk
2013-Jul-12 14:25 UTC
[RFC PATCH v1] 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. This 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.
There is also the override boolean flag: "scan_ucode" to disable
this code. Also if "ucode" command line parameter is used this
code turns itself off.
When this code works, the hypervisor ring buffer will have:
(XEN) microcode: CPU0 updated from revision 0x25 to 0x28, date = 2012-04-24
.. and so on for all the other CPUs.
(This is an example from a SandyBridge CPU).
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Revised code based on Boris Ostrovsky''s comments]
[v2: Fix memory leak (forgot to call xfree)]
---
docs/misc/xen-command-line.markdown | 12 +++
xen/arch/x86/microcode.c | 160 ++++++++++++++++++++++++++++++++---
xen/common/Makefile | 2 +-
xen/common/earlycpio.c | 151 +++++++++++++++++++++++++++++++++
xen/include/xen/earlycpio.h | 15 +++
5 files changed, 326 insertions(+), 14 deletions(-)
create mode 100644 xen/common/earlycpio.c
create mode 100644 xen/include/xen/earlycpio.h
diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 708ffc2..34c679f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -760,6 +760,18 @@ only available on 32-bit x86 platforms.
`acpi` instructs Xen to reboot the host using RESET_REG in the ACPI FADT.
+### scan_ucode
+> `= <boolean>`
+
+> Default: `true`
+
+Flag to globally enable or disable support for scanning the multiboot images
+for an cpio image that contains microcode, which are:
+ on Intel: kernel/x86/microcode/GenuineIntel.bin
+ on AMD : kernel/x86/microcode/AuthenticAMD.bin
+
+Note that is ''ucode'' parameter is used this option becomes
disabled.
+
### sched
> `= credit | credit2 | sedf | arinc653`
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index fbbf95b..202162f 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -39,12 +39,33 @@
#include <asm/setup.h>
#include <asm/microcode.h>
+#include <xen/earlycpio.h>
+
static module_t __initdata ucode_mod;
static void *(*__initdata ucode_mod_map)(const module_t *);
static signed int __initdata ucode_mod_idx;
static bool_t __initdata ucode_mod_forced;
static cpumask_t __initdata init_mask;
+/*
+ * If we scan the initramfs.cpio for the early microcode code
+ * and find it, then ''ucode_blob'' will contain the pointer
+ * and the size of said blob. It is allocated from Xen''s heap
+ * memory.
+ */
+struct ucode_mod_blob {
+ void *data;
+ size_t size;
+};
+
+static struct ucode_mod_blob __initdata ucode_blob;
+/*
+ * By default we will parse the multiboot modules to see if there is
+ * cpio image with the microcode images.
+ */
+static bool_t __initdata opt_scan_ucode = 1;
+boolean_param("scan_ucode", opt_scan_ucode);
+
void __init microcode_set_module(unsigned int idx)
{
ucode_mod_idx = idx;
@@ -58,6 +79,88 @@ static void __init parse_ucode(char *s)
}
custom_param("ucode", parse_ucode);
+static __initdata const char * const ucodes[] =
{"kernel/x86/microcode/GenuineIntel.bin",
+
"kernel/x86/microcode/AuthenticAMD.bin"};
+/*
+ * 8MB ought to be enough.
+ */
+#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
+
+void __init microcode_scan_module(
+ unsigned long *module_map,
+ const multiboot_info_t *mbi,
+ void *(*bootmap)(const module_t *))
+{
+ module_t *mod = (module_t *)__va(mbi->mods_addr);
+ uint64_t *_blob_start;
+ unsigned long _blob_size;
+ struct cpio_data cd;
+ long offset;
+ const char *p = NULL;
+ int i;
+
+ ucode_blob.size = 0;
+ if ( opt_scan_ucode == 0 )
+ return;
+ /*
+ * If user used ucode=X parameter, let them have it.
+ */
+ if ( ucode_mod_idx )
+ return;
+
+ if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ p = ucodes[1];
+ else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+ p = ucodes[0];
+ else
+ return;
+
+ /*
+ * Try all modules and see whichever could be the microcode blob.
+ */
+ for ( i = 0; i < mbi->mods_count; i++ )
+ {
+ if ( !test_bit(i, module_map) )
+ continue;
+
+ _blob_start = bootmap(&mod[i]);
+ _blob_size = mod[i].mod_end;
+ if ( !_blob_start )
+ {
+ printk("Could not map multiboot module #%d (size:
%ld)\n",
+ i, _blob_size);
+ return;
+ }
+ cd.data = NULL;
+ cd.size = 0;
+ cd = find_cpio_data(p, _blob_start, _blob_size, &offset /*
don''t care */);
+ if ( cd.data )
+ {
+ /*
+ * This is an arbitrary check - it would be sad if the blob
+ * consumed most of the memory and did not allow guests
+ * to launch.
+ */
+ if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
+ {
+ printk("Multiboot %d microcode payload too big! (%ld,
we can do %d)\n",
+ i, cd.size, MAX_EARLY_CPIO_MICROCODE);
+ goto err;
+ }
+ ucode_blob.size = cd.size;
+ ucode_blob.data = xmalloc_bytes(cd.size);
+ if ( !ucode_blob.data )
+ goto err;
+ memcpy(ucode_blob.data, cd.data, cd.size);
+ }
+ bootmap(NULL);
+ if ( cd.data )
+ break;
+ }
+ return;
+err:
+ bootmap(NULL);
+}
void __init microcode_grab_module(
unsigned long *module_map,
const multiboot_info_t *mbi,
@@ -69,7 +172,10 @@ 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) )
+ {
+ microcode_scan_module(module_map, mbi, map);
return;
+ }
ucode_mod = mod[ucode_mod_idx];
ucode_mod_map = map;
}
@@ -236,7 +342,15 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void)
buf, unsigned long len)
static void __init _do_microcode_update(unsigned long data)
{
- microcode_update_cpu((void *)data, ucode_mod.mod_end);
+ void *_data = (void *)data;
+ size_t len = ucode_mod.mod_end;
+
+ if ( ucode_blob.size )
+ {
+ _data = (void *)ucode_blob.data;
+ len = ucode_blob.size;
+ }
+ microcode_update_cpu(_data, len);
cpumask_set_cpu(smp_processor_id(), &init_mask);
}
@@ -246,18 +360,22 @@ static int __init microcode_init(void)
static struct tasklet __initdata tasklet;
unsigned int cpu;
- if ( !microcode_ops || !ucode_mod.mod_end )
+ if ( !microcode_ops )
+ return 0;
+
+ if ( !ucode_mod.mod_end && !ucode_blob.size )
return 0;
- data = ucode_mod_map(&ucode_mod);
+ if ( ucode_blob.size )
+ data = ucode_blob.data;
+ else
+ data = ucode_mod_map(&ucode_mod);
+
if ( !data )
return -ENOMEM;
if ( microcode_ops->start_update &&
microcode_ops->start_update() != 0 )
- {
- ucode_mod_map(NULL);
- return 0;
- }
+ goto out;
softirq_tasklet_init(&tasklet, _do_microcode_update, (unsigned
long)data);
@@ -269,7 +387,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 +420,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(µcode_percpu_nfb);
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 0dc2050..f85d3f5 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -48,7 +48,7 @@ obj-y += radix-tree.o
obj-y += rbtree.o
obj-y += lzo.o
-obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma
unlzo,$(n).init.o)
+obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma
unlzo,$(n).init.o earlycpio.o)
obj-$(perfc) += perfc.o
obj-$(crash_debug) += gdbstub.o
diff --git a/xen/common/earlycpio.c b/xen/common/earlycpio.c
new file mode 100644
index 0000000..8b5abb6
--- /dev/null
+++ b/xen/common/earlycpio.c
@@ -0,0 +1,151 @@
+/* ----------------------------------------------------------------------- *
+ *
+ * Copyright 2012 Intel Corporation; author H. Peter Anvin
+ *
+ * This file is part of the Linux kernel, and is made available
+ * under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * ----------------------------------------------------------------------- */
+
+/*
+ * earlycpio.c
+ *
+ * Find a specific cpio member; must precede any compressed content.
+ * This is used to locate data items in the initramfs used by the
+ * kernel itself during early boot (before the main initramfs is
+ * decompressed.) It is the responsibility of the initramfs creator
+ * to ensure that these items are uncompressed at the head of the
+ * blob. Depending on the boot loader or package tool that may be a
+ * separate file or part of the same file.
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/string.h>
+#include <xen/earlycpio.h>
+
+#define ALIGN(x, a) ((x + (a) - 1) & ~((a) - 1))
+#define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a)))
+
+enum cpio_fields {
+ C_MAGIC,
+ C_INO,
+ C_MODE,
+ C_UID,
+ C_GID,
+ C_NLINK,
+ C_MTIME,
+ C_FILESIZE,
+ C_MAJ,
+ C_MIN,
+ C_RMAJ,
+ C_RMIN,
+ C_NAMESIZE,
+ C_CHKSUM,
+ C_NFIELDS
+};
+
+/**
+ * cpio_data find_cpio_data - Search for files in an uncompressed cpio
+ * @path: The directory to search for, including a slash at the end
+ * @data: Pointer to the the cpio archive or a header inside
+ * @len: Remaining length of the cpio based on data pointer
+ * @offset: When a matching file is found, this is the offset to the
+ * beginning of the cpio. It can be used to iterate through
+ * the cpio to find all files inside of a directory path
+ *
+ * @return: struct cpio_data containing the address, length and
+ * filename (with the directory path cut off) of the found file.
+ * If you search for a filename and not for files in a directory,
+ * pass the absolute path of the filename in the cpio and make sure
+ * the match returned an empty filename string.
+ */
+
+struct cpio_data __cpuinit find_cpio_data(const char *path, void *data,
+ size_t len, long *offset)
+{
+ const size_t cpio_header_len = 8*C_NFIELDS - 2;
+ struct cpio_data cd = { NULL, 0, "" };
+ const char *p, *dptr, *nptr;
+ unsigned int ch[C_NFIELDS], *chp, v;
+ unsigned char c, x;
+ size_t mypathsize = strlen(path);
+ int i, j;
+
+ p = data;
+
+ while (len > cpio_header_len) {
+ if (!*p) {
+ /* All cpio headers need to be 4-byte aligned */
+ p += 4;
+ len -= 4;
+ continue;
+ }
+
+ j = 6; /* The magic field is only 6 characters */
+ chp = ch;
+ for (i = C_NFIELDS; i; i--) {
+ v = 0;
+ while (j--) {
+ v <<= 4;
+ c = *p++;
+
+ x = c - ''0'';
+ if (x < 10) {
+ v += x;
+ continue;
+ }
+
+ x = (c | 0x20) - ''a'';
+ if (x < 6) {
+ v += x + 10;
+ continue;
+ }
+
+ goto quit; /* Invalid hexadecimal */
+ }
+ *chp++ = v;
+ j = 8; /* All other fields are 8 characters */
+ }
+
+ if ((ch[C_MAGIC] - 0x070701) > 1)
+ goto quit; /* Invalid magic */
+
+ len -= cpio_header_len;
+
+ dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
+ nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
+
+ if (nptr > p + len || dptr < p || nptr < dptr)
+ goto quit; /* Buffer overrun */
+
+ if ((ch[C_MODE] & 0170000) == 0100000 &&
+ ch[C_NAMESIZE] >= mypathsize &&
+ !memcmp(p, path, mypathsize)) {
+ *offset = (long)nptr - (long)data;
+ if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
+ printk(
+ "File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
+ p, MAX_CPIO_FILE_NAME);
+ }
+ strlcpy(cd.name, p + mypathsize, MAX_CPIO_FILE_NAME);
+
+ cd.data = (void *)dptr;
+ cd.size = ch[C_FILESIZE];
+ return cd; /* Found it! */
+ }
+ len -= (nptr - p);
+ p = nptr;
+ }
+
+quit:
+ return cd;
+}
+
diff --git a/xen/include/xen/earlycpio.h b/xen/include/xen/earlycpio.h
new file mode 100644
index 0000000..16d9404
--- /dev/null
+++ b/xen/include/xen/earlycpio.h
@@ -0,0 +1,15 @@
+#ifndef _EARLYCPIO_H
+#define _EARLYCPIO_H
+
+#define MAX_CPIO_FILE_NAME 18
+
+struct cpio_data {
+ void *data;
+ size_t size;
+ char name[MAX_CPIO_FILE_NAME];
+};
+
+struct cpio_data find_cpio_data(const char *path, void *data, size_t len,
+ long *offset);
+
+#endif /* _EARLYCPIO_H */
--
1.7.7.6
Jan Beulich
2013-Jul-15 07:18 UTC
Re: [RFC PATCH] Scan initramfs for microcode cpio archive. (v1)
>>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: > Greetings, > > Please see the following patch which implements a mechanism to scan > the initramfs for the format of an microcode files. This is a feature > that the Linux kernel has since v3.10 - where it searches in the > initramfs for an archive of the microcode blob. The format is documented > in the Linux tree and the commit description contains it. > > The tool to make this work is the initramfs creator. The author has > provided an RFC patch: > http://article.gmane.org/gmane.linux.kernel.initramfs/3318 > which does that.Looking at that script change I''m getting the impression that the representation is as uncompressed cpio containing the ucode blobs followed by compressed cpio containing all the "normal" stuff. Which iiuc means that an unaware kernel can''t deal with such an image (as the consumer needs to know that it has to skip the initial portion). Unless that understanding of mine is wrong - isn''t that a rather bad design? Jan
Jan Beulich
2013-Jul-15 07:23 UTC
Re: [RFC PATCH v1] microcode: Scan the initramfs payload for microcode blob.
>>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -760,6 +760,18 @@ only available on 32-bit x86 platforms. > > `acpi` instructs Xen to reboot the host using RESET_REG in the ACPI FADT. > > +### scan_ucode > +> `= <boolean>` > + > +> Default: `true` > + > +Flag to globally enable or disable support for scanning the multiboot images > +for an cpio image that contains microcode, which are: > + on Intel: kernel/x86/microcode/GenuineIntel.bin > + on AMD : kernel/x86/microcode/AuthenticAMD.bin > + > +Note that is ''ucode'' parameter is used this option becomes disabled. > +I would want this to be an extension of the "ucode=" option, not an independent new one. E.g. "ucode=initrd" or "ucode=cpio:<num>" or some such.> --- a/xen/arch/x86/microcode.c > +++ b/xen/arch/x86/microcode.c > @@ -39,12 +39,33 @@ > #include <asm/setup.h> > #include <asm/microcode.h> > > +#include <xen/earlycpio.h> > + > static module_t __initdata ucode_mod; > static void *(*__initdata ucode_mod_map)(const module_t *); > static signed int __initdata ucode_mod_idx; > static bool_t __initdata ucode_mod_forced; > static cpumask_t __initdata init_mask; > > +/* > + * If we scan the initramfs.cpio for the early microcode code > + * and find it, then ''ucode_blob'' will contain the pointer > + * and the size of said blob. It is allocated from Xen''s heap > + * memory. > + */ > +struct ucode_mod_blob { > + void *data; > + size_t size; > +}; > + > +static struct ucode_mod_blob __initdata ucode_blob; > +/* > + * By default we will parse the multiboot modules to see if there is > + * cpio image with the microcode images. > + */ > +static bool_t __initdata opt_scan_ucode = 1; > +boolean_param("scan_ucode", opt_scan_ucode);I''m also unsure we really want this on by default. "ucode=<num>" also isn''t having any "default on" effect. Reviewing the rest of the patch requires clarification on the concept first, as asked for in response to the "overview" mail. Jan
David Vrabel
2013-Jul-15 12:31 UTC
Re: [RFC PATCH] Scan initramfs for microcode cpio archive. (v1)
On 15/07/13 08:18, Jan Beulich wrote:>>>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: >> Greetings, >> >> Please see the following patch which implements a mechanism to scan >> the initramfs for the format of an microcode files. This is a feature >> that the Linux kernel has since v3.10 - where it searches in the >> initramfs for an archive of the microcode blob. The format is documented >> in the Linux tree and the commit description contains it. >> >> The tool to make this work is the initramfs creator. The author has >> provided an RFC patch: >> http://article.gmane.org/gmane.linux.kernel.initramfs/3318 >> which does that. > > Looking at that script change I''m getting the impression that the > representation is as uncompressed cpio containing the ucode > blobs followed by compressed cpio containing all the "normal" > stuff. Which iiuc means that an unaware kernel can''t deal with > such an image (as the consumer needs to know that it has to > skip the initial portion). Unless that understanding of mine is > wrong - isn''t that a rather bad design?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. David
Konrad Rzeszutek Wilk
2013-Jul-15 14:34 UTC
Re: [RFC PATCH] Scan initramfs for microcode cpio archive. (v1)
On Mon, Jul 15, 2013 at 08:18:51AM +0100, Jan Beulich wrote:> >>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: > > Greetings, > > > > Please see the following patch which implements a mechanism to scan > > the initramfs for the format of an microcode files. This is a feature > > that the Linux kernel has since v3.10 - where it searches in the > > initramfs for an archive of the microcode blob. The format is documented > > in the Linux tree and the commit description contains it. > > > > The tool to make this work is the initramfs creator. The author has > > provided an RFC patch: > > http://article.gmane.org/gmane.linux.kernel.initramfs/3318 > > which does that. > > Looking at that script change I''m getting the impression that the > representation is as uncompressed cpio containing the ucode > blobs followed by compressed cpio containing all the "normal" > stuff. Which iiuc means that an unaware kernel can''t deal with > such an image (as the consumer needs to know that it has to > skip the initial portion). Unless that understanding of mine is > wrong - isn''t that a rather bad design? >The kernel if compiled without EARLY_INITRAMFS.. and an initramfs _with_ the uncompressed cpio containing the ucode blob followed by compressed cpio containing the normal stuff boots. I tested this with a v3.9 and v3.11 kernel and both booted without trouble.> Jan > >
Konrad Rzeszutek Wilk
2013-Jul-15 14:36 UTC
Re: [RFC PATCH v1] microcode: Scan the initramfs payload for microcode blob.
On Mon, Jul 15, 2013 at 08:23:04AM +0100, Jan Beulich wrote:> >>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: > > --- a/docs/misc/xen-command-line.markdown > > +++ b/docs/misc/xen-command-line.markdown > > @@ -760,6 +760,18 @@ only available on 32-bit x86 platforms. > > > > `acpi` instructs Xen to reboot the host using RESET_REG in the ACPI FADT. > > > > +### scan_ucode > > +> `= <boolean>` > > + > > +> Default: `true` > > + > > +Flag to globally enable or disable support for scanning the multiboot images > > +for an cpio image that contains microcode, which are: > > + on Intel: kernel/x86/microcode/GenuineIntel.bin > > + on AMD : kernel/x86/microcode/AuthenticAMD.bin > > + > > +Note that is ''ucode'' parameter is used this option becomes disabled. > > + > > I would want this to be an extension of the "ucode=" option, not > an independent new one. E.g. "ucode=initrd" or > "ucode=cpio:<num>" or some such.I believe that it would be best suited for users if said feature was turned on by default - and then if needed - can be turned off. Similarly to how HAP is enabled by default. Using the over-ride in ''ucode'' parse code is fine with me. I would propose ''ucode=noscan'' or ''ucode=noauto'' as the parameters.> > > --- a/xen/arch/x86/microcode.c > > +++ b/xen/arch/x86/microcode.c > > @@ -39,12 +39,33 @@ > > #include <asm/setup.h> > > #include <asm/microcode.h> > > > > +#include <xen/earlycpio.h> > > + > > static module_t __initdata ucode_mod; > > static void *(*__initdata ucode_mod_map)(const module_t *); > > static signed int __initdata ucode_mod_idx; > > static bool_t __initdata ucode_mod_forced; > > static cpumask_t __initdata init_mask; > > > > +/* > > + * If we scan the initramfs.cpio for the early microcode code > > + * and find it, then ''ucode_blob'' will contain the pointer > > + * and the size of said blob. It is allocated from Xen''s heap > > + * memory. > > + */ > > +struct ucode_mod_blob { > > + void *data; > > + size_t size; > > +}; > > + > > +static struct ucode_mod_blob __initdata ucode_blob; > > +/* > > + * By default we will parse the multiboot modules to see if there is > > + * cpio image with the microcode images. > > + */ > > +static bool_t __initdata opt_scan_ucode = 1; > > +boolean_param("scan_ucode", opt_scan_ucode); > > I''m also unsure we really want this on by default. "ucode=<num>" > also isn''t having any "default on" effect.Correct. Because it (''ucode=<num>'') is unable to figure out which of the multiboot payloads has the ucode. It needs the help from GRUB2 or other tools that construct the bootloader configuration files. This code, similar to how XSM finds its policy, can find the microcode blob without the help of the bootloader. In the Linux code if this feature is turned on it will _always_ search the initramfs. I think that idea makes sense here as well - we want to enable X feature if we detect that it exists.> > Reviewing the rest of the patch requires clarification on the > concept first, as asked for in response to the "overview" mail. > > Jan > >
Jan Beulich
2013-Jul-15 14:52 UTC
Re: [RFC PATCH v1] microcode: Scan the initramfs payload for microcode blob.
>>> On 15.07.13 at 16:36, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Mon, Jul 15, 2013 at 08:23:04AM +0100, Jan Beulich wrote: >> >>> On 12.07.13 at 16:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote: >> > +static struct ucode_mod_blob __initdata ucode_blob; >> > +/* >> > + * By default we will parse the multiboot modules to see if there is >> > + * cpio image with the microcode images. >> > + */ >> > +static bool_t __initdata opt_scan_ucode = 1; >> > +boolean_param("scan_ucode", opt_scan_ucode); >> >> I''m also unsure we really want this on by default. "ucode=<num>" >> also isn''t having any "default on" effect. > > Correct. Because it (''ucode=<num>'') is unable to figure out which of > the multiboot payloads has the ucode. It needs the help from GRUB2 > or other tools that construct the bootloader configuration files.No - it could have been coded the same way as XSM. But when I put this together, the approach of looking just at the first few bytes and draw conclusions from that seemed to fragile to me. Admitted XSM at least has some kind signature there, but at least AMD ucode has too, so one could have used that. Still I think that having the user explicitly state what (s)he wants is better than this guesswork.> This code, similar to how XSM finds its policy, can find the microcode > blob without the help of the bootloader. > > In the Linux code if this feature is turned on it will _always_ > search the initramfs. I think that idea makes sense here as well - > we want to enable X feature if we detect that it exists.Linux can safely always do that, because it can make assumptions about the format of the initrd. 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 (now that the question regarding the concatenation was answered By David). Jan