2015-02-24 10:37:54

by Borislav Petkov

Subject: [PATCH 00/13] x86/microcode: Intel early loader cleanups

From: Borislav Petkov <[email protected]>

Hi,

so this is something which got started in the aftermath of a discussion
about some robustifying fixes to the microcode loader by Quentin.
Everyone agrees that current code needs a good rubbing so here's part
one of that. More to come later, let's not overwhelm people with huge
patchsets.

All patches are cleanups and simplifications in an attempt to make the
code more readable and simpler and enable follow-up improvements.

Thanks.

Borislav Petkov (13):
x86/microcode/intel: Check if microcode was found before applying
x86/microcode/intel: Do the mc_saved_src NULL check first
x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()
x86/microcode/intel: Simplify load_ucode_intel_bsp()
x86/microcode/intel: Make _save_mc() return the updated saved count
x86/microcode/intel: Sanitize _save_mc()
x86/microcode/intel: Rename update_match_revision()
x86/microcode: Consolidate family,model, ... code
x86/microcode/intel: Simplify generic_load_microcode_early()
x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}
x86/microcode/intel: Sanitize microcode_pointer()
x86/microcode/intel: Check scan_microcode()'s retval
x86/microcode/intel: Fix printing of microcode blobs in
show_saved_mc()

arch/x86/include/asm/microcode.h | 73 ++++++
arch/x86/include/asm/microcode_intel.h | 13 +-
arch/x86/kernel/cpu/microcode/core_early.c | 75 +-----
arch/x86/kernel/cpu/microcode/intel.c | 4 +-
arch/x86/kernel/cpu/microcode/intel_early.c | 341 +++++++++++++---------------
arch/x86/kernel/cpu/microcode/intel_lib.c | 22 +-
6 files changed, 258 insertions(+), 270 deletions(-)

--
2.2.0.33.gc18b867

2015-02-24 10:37:52

by Borislav Petkov

Subject: [PATCH 01/13] x86/microcode/intel: Check if microcode was found before applying

From: Borislav Petkov <[email protected]>

We should check the return value of the routines fishing out the proper
microcode and not try to apply if we haven't found a suitable blob.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 420eb933189c..b3cb1568cd9a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -729,9 +729,10 @@ _load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,

ret = load_microcode(mc_saved_data, mc_saved_in_initrd,
initrd_start_early, uci);
+ if (ret != UCODE_OK)
+ return;

- if (ret == UCODE_OK)
- apply_microcode_early(uci, true);
+ apply_microcode_early(uci, true);
}

void __init
@@ -771,6 +772,7 @@ void load_ucode_intel_ap(void)
struct ucode_cpu_info uci;
unsigned long *mc_saved_in_initrd_p;
unsigned long initrd_start_addr;
+ enum ucode_state ret;
#ifdef CONFIG_X86_32
unsigned long *initrd_start_p;

@@ -793,8 +795,12 @@ void load_ucode_intel_ap(void)
return;

collect_cpu_info_early(&uci);
- load_microcode(mc_saved_data_p, mc_saved_in_initrd_p,
- initrd_start_addr, &uci);
+ ret = load_microcode(mc_saved_data_p, mc_saved_in_initrd_p,
+ initrd_start_addr, &uci);
+
+ if (ret != UCODE_OK)
+ return;
+
apply_microcode_early(&uci, true);
}

--
2.2.0.33.gc18b867

2015-02-24 10:42:52

by Borislav Petkov

Subject: [PATCH 02/13] x86/microcode/intel: Do the mc_saved_src NULL check first

From: Borislav Petkov <[email protected]>

... and only then deref it. Also, shorten some variable names and rename
others so as to diminish the ubiquitous presence of the "mc_" prefix
everywhere and make it a bit more readable.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 37 +++++++++++++++++------------
1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b3cb1568cd9a..1676c927045b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -204,7 +204,7 @@ save_microcode(struct mc_saved_data *mc_saved_data,
unsigned int mc_saved_count)
{
int i, j;
- struct microcode_intel **mc_saved_p;
+ struct microcode_intel **saved_ptr;
int ret;

if (!mc_saved_count)
@@ -213,39 +213,46 @@ save_microcode(struct mc_saved_data *mc_saved_data,
/*
* Copy new microcode data.
*/
- mc_saved_p = kmalloc(mc_saved_count*sizeof(struct microcode_intel *),
+ saved_ptr = kmalloc(mc_saved_count * sizeof(struct microcode_intel *),
GFP_KERNEL);
- if (!mc_saved_p)
+ if (!saved_ptr)
return -ENOMEM;

for (i = 0; i < mc_saved_count; i++) {
- struct microcode_intel *mc = mc_saved_src[i];
- struct microcode_header_intel *mc_header = &mc->hdr;
- unsigned long mc_size = get_totalsize(mc_header);
- mc_saved_p[i] = kmalloc(mc_size, GFP_KERNEL);
- if (!mc_saved_p[i]) {
- ret = -ENOMEM;
- goto err;
- }
+ struct microcode_header_intel *mc_hdr;
+ struct microcode_intel *mc;
+ unsigned long size;
+
if (!mc_saved_src[i]) {
ret = -EINVAL;
goto err;
}
- memcpy(mc_saved_p[i], mc, mc_size);
+
+ mc = mc_saved_src[i];
+ mc_hdr = &mc->hdr;
+ size = get_totalsize(mc_hdr);
+
+ saved_ptr[i] = kmalloc(size, GFP_KERNEL);
+ if (!saved_ptr[i]) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ memcpy(saved_ptr[i], mc, size);
}

/*
* Point to newly saved microcode.
*/
- mc_saved_data->mc_saved = mc_saved_p;
+ mc_saved_data->mc_saved = saved_ptr;
mc_saved_data->mc_saved_count = mc_saved_count;

return 0;

err:
for (j = 0; j <= i; j++)
- kfree(mc_saved_p[j]);
- kfree(mc_saved_p);
+ kfree(saved_ptr[j]);
+ kfree(saved_ptr);

return ret;
}
--
2.2.0.33.gc18b867

2015-02-24 10:42:50

by Borislav Petkov

Subject: [PATCH 03/13] x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()

From: Borislav Petkov <[email protected]>

Allocate it on the helper's _load_ucode_intel_bsp() stack instead and do
not hand it down.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 1676c927045b..988ef348cb1b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -725,21 +725,21 @@ static void __init
_load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
unsigned long *mc_saved_in_initrd,
unsigned long initrd_start_early,
- unsigned long initrd_end_early,
- struct ucode_cpu_info *uci)
+ unsigned long initrd_end_early)
{
+ struct ucode_cpu_info uci;
enum ucode_state ret;

- collect_cpu_info_early(uci);
+ collect_cpu_info_early(&uci);
scan_microcode(initrd_start_early, initrd_end_early, mc_saved_data,
- mc_saved_in_initrd, uci);
+ mc_saved_in_initrd, &uci);

ret = load_microcode(mc_saved_data, mc_saved_in_initrd,
- initrd_start_early, uci);
+ initrd_start_early, &uci);
if (ret != UCODE_OK)
return;

- apply_microcode_early(uci, true);
+ apply_microcode_early(&uci, true);
}

void __init
@@ -747,7 +747,6 @@ load_ucode_intel_bsp(void)
{
u64 ramdisk_image, ramdisk_size;
unsigned long initrd_start_early, initrd_end_early;
- struct ucode_cpu_info uci;
#ifdef CONFIG_X86_32
struct boot_params *boot_params_p;

@@ -760,7 +759,7 @@ load_ucode_intel_bsp(void)
_load_ucode_intel_bsp(
(struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
(unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
- initrd_start_early, initrd_end_early, &uci);
+ initrd_start_early, initrd_end_early);
#else
ramdisk_image = boot_params.hdr.ramdisk_image;
ramdisk_size = boot_params.hdr.ramdisk_size;
@@ -768,8 +767,7 @@ load_ucode_intel_bsp(void)
initrd_end_early = initrd_start_early + ramdisk_size;

_load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd,
- initrd_start_early, initrd_end_early,
- &uci);
+ initrd_start_early, initrd_end_early);
#endif
}

--
2.2.0.33.gc18b867

2015-02-24 10:41:51

by Borislav Petkov

Subject: [PATCH 04/13] x86/microcode/intel: Simplify load_ucode_intel_bsp()

From: Borislav Petkov <[email protected]>

Don't compute start and end from start and size in order to compute size
again down the path in scan_microcode(). So pass size directly instead
and simplify a bunch. Shorten variable names and remove useless ones.

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 49 +++++++++++------------------
1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 988ef348cb1b..ffeac5d62eca 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -555,12 +555,10 @@ EXPORT_SYMBOL_GPL(save_mc_for_early);

static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
static __init enum ucode_state
-scan_microcode(unsigned long start, unsigned long end,
- struct mc_saved_data *mc_saved_data,
- unsigned long *mc_saved_in_initrd,
- struct ucode_cpu_info *uci)
+scan_microcode(unsigned long start, unsigned long size,
+ struct mc_saved_data *mc_saved_data,
+ unsigned long *mc_saved_in_initrd, struct ucode_cpu_info *uci)
{
- unsigned int size = end - start + 1;
struct cpio_data cd;
long offset = 0;
#ifdef CONFIG_X86_32
@@ -576,7 +574,6 @@ scan_microcode(unsigned long start, unsigned long end,
if (!cd.data)
return UCODE_ERROR;

-
return get_matching_model_microcode(0, start, cd.data, cd.size,
mc_saved_data, mc_saved_in_initrd,
uci);
@@ -724,50 +721,40 @@ int __init save_microcode_in_initrd_intel(void)
static void __init
_load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
unsigned long *mc_saved_in_initrd,
- unsigned long initrd_start_early,
- unsigned long initrd_end_early)
+ unsigned long start, unsigned long size)
{
struct ucode_cpu_info uci;
enum ucode_state ret;

collect_cpu_info_early(&uci);
- scan_microcode(initrd_start_early, initrd_end_early, mc_saved_data,
- mc_saved_in_initrd, &uci);
+ scan_microcode(start, size, mc_saved_data, mc_saved_in_initrd, &uci);

- ret = load_microcode(mc_saved_data, mc_saved_in_initrd,
- initrd_start_early, &uci);
+ ret = load_microcode(mc_saved_data, mc_saved_in_initrd, start, &uci);
if (ret != UCODE_OK)
return;

apply_microcode_early(&uci, true);
}

-void __init
-load_ucode_intel_bsp(void)
+void __init load_ucode_intel_bsp(void)
{
- u64 ramdisk_image, ramdisk_size;
- unsigned long initrd_start_early, initrd_end_early;
+ u64 start, size;
#ifdef CONFIG_X86_32
- struct boot_params *boot_params_p;
+ struct boot_params *p;

- boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
- ramdisk_image = boot_params_p->hdr.ramdisk_image;
- ramdisk_size = boot_params_p->hdr.ramdisk_size;
- initrd_start_early = ramdisk_image;
- initrd_end_early = initrd_start_early + ramdisk_size;
+ p = (struct boot_params *)__pa_nodebug(&boot_params);
+ start = p->hdr.ramdisk_image;
+ size = p->hdr.ramdisk_size;

_load_ucode_intel_bsp(
- (struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
- (unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
- initrd_start_early, initrd_end_early);
+ (struct mc_saved_data *)__pa_nodebug(&mc_saved_data),
+ (unsigned long *)__pa_nodebug(&mc_saved_in_initrd),
+ start, size);
#else
- ramdisk_image = boot_params.hdr.ramdisk_image;
- ramdisk_size = boot_params.hdr.ramdisk_size;
- initrd_start_early = ramdisk_image + PAGE_OFFSET;
- initrd_end_early = initrd_start_early + ramdisk_size;
+ start = boot_params.hdr.ramdisk_image + PAGE_OFFSET;
+ size = boot_params.hdr.ramdisk_size;

- _load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd,
- initrd_start_early, initrd_end_early);
+ _load_ucode_intel_bsp(&mc_saved_data, mc_saved_in_initrd, start, size);
#endif
}

--
2.2.0.33.gc18b867

2015-02-24 10:42:23

by Borislav Petkov

Subject: [PATCH 05/13] x86/microcode/intel: Make _save_mc() return the updated saved count

From: Borislav Petkov <[email protected]>

... of microcode patches instead of handing in a pointer which is used
for I/O in an otherwise void function.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index ffeac5d62eca..ee74e7726c33 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -264,17 +264,18 @@ err:
* - or if it is a newly discovered microcode patch.
*
* The microcode patch should have matching model with CPU.
+ *
+ * Returns: The updated number @num_saved of saved microcode patches.
*/
-static void _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr,
- unsigned int *mc_saved_count_p)
+static unsigned int _save_mc(struct microcode_intel **mc_saved,
+ u8 *ucode_ptr, unsigned int num_saved)
{
- int i;
- int found = 0;
- unsigned int mc_saved_count = *mc_saved_count_p;
struct microcode_header_intel *mc_header;
+ int found = 0, i;

mc_header = (struct microcode_header_intel *)ucode_ptr;
- for (i = 0; i < mc_saved_count; i++) {
+
+ for (i = 0; i < num_saved; i++) {
unsigned int sig, pf;
unsigned int new_rev;
struct microcode_header_intel *mc_saved_header =
@@ -291,21 +292,20 @@ static void _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr,
* Replace the older one with this newer
* one.
*/
- mc_saved[i] =
- (struct microcode_intel *)ucode_ptr;
+ mc_saved[i] = (struct microcode_intel *)ucode_ptr;
break;
}
}
}
- if (i >= mc_saved_count && !found)
+
+ if (i >= num_saved && !found)
/*
* This ucode is first time discovered in ucode file.
* Save it to memory.
*/
- mc_saved[mc_saved_count++] =
- (struct microcode_intel *)ucode_ptr;
+ mc_saved[num_saved++] = (struct microcode_intel *)ucode_ptr;

- *mc_saved_count_p = mc_saved_count;
+ return num_saved;
}

/*
@@ -353,7 +353,7 @@ get_matching_model_microcode(int cpu, unsigned long start,
continue;
}

- _save_mc(mc_saved_tmp, ucode_ptr, &mc_saved_count);
+ mc_saved_count = _save_mc(mc_saved_tmp, ucode_ptr, mc_saved_count);

ucode_ptr += mc_size;
}
@@ -522,8 +522,7 @@ int save_mc_for_early(u8 *mc)
* Save the microcode patch mc in mc_save_tmp structure if it's a newer
* version.
*/
-
- _save_mc(mc_saved_tmp, mc, &mc_saved_count);
+ mc_saved_count = _save_mc(mc_saved_tmp, mc, mc_saved_count);

/*
* Save the mc_save_tmp in global mc_saved_data.
--
2.2.0.33.gc18b867

2015-02-24 10:41:49

by Borislav Petkov

Subject: [PATCH 06/13] x86/microcode/intel: Sanitize _save_mc()

From: Borislav Petkov <[email protected]>

Shorten local variable names for better readability and flatten loop
indentation levels.

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 49 ++++++++++++++---------------
1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index ee74e7726c33..1bd46690499c 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -270,39 +270,36 @@ err:
static unsigned int _save_mc(struct microcode_intel **mc_saved,
u8 *ucode_ptr, unsigned int num_saved)
{
- struct microcode_header_intel *mc_header;
+ struct microcode_header_intel *mc_hdr, *mc_saved_hdr;
+ unsigned int sig, pf, new_rev;
int found = 0, i;

- mc_header = (struct microcode_header_intel *)ucode_ptr;
+ mc_hdr = (struct microcode_header_intel *)ucode_ptr;

for (i = 0; i < num_saved; i++) {
- unsigned int sig, pf;
- unsigned int new_rev;
- struct microcode_header_intel *mc_saved_header =
- (struct microcode_header_intel *)mc_saved[i];
- sig = mc_saved_header->sig;
- pf = mc_saved_header->pf;
- new_rev = mc_header->rev;
-
- if (get_matching_sig(sig, pf, ucode_ptr, new_rev)) {
- found = 1;
- if (update_match_revision(mc_header, new_rev)) {
- /*
- * Found an older ucode saved before.
- * Replace the older one with this newer
- * one.
- */
- mc_saved[i] = (struct microcode_intel *)ucode_ptr;
- break;
- }
- }
- }
+ mc_saved_hdr = (struct microcode_header_intel *)mc_saved[i];
+ sig = mc_saved_hdr->sig;
+ pf = mc_saved_hdr->pf;
+ new_rev = mc_hdr->rev;
+
+ if (!get_matching_sig(sig, pf, ucode_ptr, new_rev))
+ continue;
+
+ found = 1;
+
+ if (!update_match_revision(mc_hdr, new_rev))
+ continue;

- if (i >= num_saved && !found)
/*
- * This ucode is first time discovered in ucode file.
- * Save it to memory.
+ * Found an older ucode saved earlier. Replace it with
+ * this newer one.
*/
+ mc_saved[i] = (struct microcode_intel *)ucode_ptr;
+ break;
+ }
+
+ /* Newly detected microcode, save it to memory. */
+ if (i >= num_saved && !found)
mc_saved[num_saved++] = (struct microcode_intel *)ucode_ptr;

return num_saved;
--
2.2.0.33.gc18b867

2015-02-24 10:39:55

by Borislav Petkov

Subject: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

From: Borislav Petkov <[email protected]>

... to revision_is_newer() and push it up into the header and make it an
inline function.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 8 ++++++--
arch/x86/kernel/cpu/microcode/intel_early.c | 2 +-
arch/x86/kernel/cpu/microcode/intel_lib.c | 8 +-------
3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index dd4c20043ce7..a6b753a131cd 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -60,8 +60,12 @@ extern int
get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev);
extern int microcode_sanity_check(void *mc, int print_err);
extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
-extern int
-update_match_revision(struct microcode_header_intel *mc_header, int rev);
+
+static inline int
+revision_is_newer(struct microcode_header_intel *mc_header, int rev)
+{
+ return (mc_header->rev <= rev) ? 0 : 1;
+}

#ifdef CONFIG_MICROCODE_INTEL_EARLY
extern void __init load_ucode_intel_bsp(void);
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 1bd46690499c..372ffc7d929b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -287,7 +287,7 @@ static unsigned int _save_mc(struct microcode_intel **mc_saved,

found = 1;

- if (!update_match_revision(mc_hdr, new_rev))
+ if (!revision_is_newer(mc_hdr, new_rev))
continue;

/*
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320d0179..7e259d99b0aa 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -38,12 +38,6 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
return (!sigmatch(sig, csig, pf, cpf)) ? 0 : 1;
}

-int
-update_match_revision(struct microcode_header_intel *mc_header, int rev)
-{
- return (mc_header->rev <= rev) ? 0 : 1;
-}
-
int microcode_sanity_check(void *mc, int print_err)
{
unsigned long total_size, data_size, ext_table_size;
@@ -166,7 +160,7 @@ int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev)
{
struct microcode_header_intel *mc_header = mc;

- if (!update_match_revision(mc_header, rev))
+ if (!revision_is_newer(mc_header, rev))
return 0;

return get_matching_sig(csig, cpf, mc, rev);
--
2.2.0.33.gc18b867

2015-02-24 10:39:52

by Borislav Petkov

Subject: [PATCH 08/13] x86/microcode: Consolidate family,model, ... code

From: Borislav Petkov <[email protected]>

... to the header. Split the family acquiring function into a
main one, doing CPUID and a helper which computes the extended
family and is used in multiple places. Get rid of the locally-grown
get_x86_{family,model}().

While at it, rename local variables to something more descriptive and
vertically align assignments for better readability.

There should be no functionality change resulting from this patch.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/microcode.h | 73 ++++++++++++++++++++++++++++
arch/x86/kernel/cpu/microcode/core_early.c | 75 +++++------------------------
arch/x86/kernel/cpu/microcode/intel_early.c | 59 ++++++-----------------
3 files changed, 101 insertions(+), 106 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 201b520521ed..2fb20d6f7e23 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -75,6 +75,79 @@ static inline void __exit exit_amd_microcode(void) {}

#ifdef CONFIG_MICROCODE_EARLY
#define MAX_UCODE_COUNT 128
+
+#define QCHAR(a, b, c, d) ((a) + ((b) << 8) + ((c) << 16) + ((d) << 24))
+#define CPUID_INTEL1 QCHAR('G', 'e', 'n', 'u')
+#define CPUID_INTEL2 QCHAR('i', 'n', 'e', 'I')
+#define CPUID_INTEL3 QCHAR('n', 't', 'e', 'l')
+#define CPUID_AMD1 QCHAR('A', 'u', 't', 'h')
+#define CPUID_AMD2 QCHAR('e', 'n', 't', 'i')
+#define CPUID_AMD3 QCHAR('c', 'A', 'M', 'D')
+
+#define CPUID_IS(a, b, c, ebx, ecx, edx) \
+ (!((ebx ^ (a))|(edx ^ (b))|(ecx ^ (c))))
+
+/*
+ * In early loading microcode phase on BSP, boot_cpu_data is not set up yet.
+ * x86_vendor() gets vendor id for BSP.
+ *
+ * In 32 bit AP case, accessing boot_cpu_data needs linear address. To simplify
+ * coding, we still use x86_vendor() to get vendor id for AP.
+ *
+ * x86_vendor() gets vendor information directly from CPUID.
+ */
+static inline int x86_vendor(void)
+{
+ u32 eax = 0x00000000;
+ u32 ebx, ecx = 0, edx;
+
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (CPUID_IS(CPUID_INTEL1, CPUID_INTEL2, CPUID_INTEL3, ebx, ecx, edx))
+ return X86_VENDOR_INTEL;
+
+ if (CPUID_IS(CPUID_AMD1, CPUID_AMD2, CPUID_AMD3, ebx, ecx, edx))
+ return X86_VENDOR_AMD;
+
+ return X86_VENDOR_UNKNOWN;
+}
+
+static inline unsigned int __x86_family(unsigned int sig)
+{
+ unsigned int x86;
+
+ x86 = (sig >> 8) & 0xf;
+
+ if (x86 == 0xf)
+ x86 += (sig >> 20) & 0xff;
+
+ return x86;
+}
+
+static inline unsigned int x86_family(void)
+{
+ u32 eax = 0x00000001;
+ u32 ebx, ecx = 0, edx;
+
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ return __x86_family(eax);
+}
+
+static inline unsigned int x86_model(unsigned int sig)
+{
+ unsigned int x86, model;
+
+ x86 = __x86_family(sig);
+
+ model = (sig >> 4) & 0xf;
+
+ if (x86 == 0x6 || x86 == 0xf)
+ model += ((sig >> 16) & 0xf) << 4;
+
+ return model;
+}
+
extern void __init load_ucode_bsp(void);
extern void load_ucode_ap(void);
extern int __init save_microcode_in_initrd(void);
diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
index d45df4bd16ab..a413a69cbd74 100644
--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -23,57 +23,6 @@
#include <asm/processor.h>
#include <asm/cmdline.h>

-#define QCHAR(a, b, c, d) ((a) + ((b) << 8) + ((c) << 16) + ((d) << 24))
-#define CPUID_INTEL1 QCHAR('G', 'e', 'n', 'u')
-#define CPUID_INTEL2 QCHAR('i', 'n', 'e', 'I')
-#define CPUID_INTEL3 QCHAR('n', 't', 'e', 'l')
-#define CPUID_AMD1 QCHAR('A', 'u', 't', 'h')
-#define CPUID_AMD2 QCHAR('e', 'n', 't', 'i')
-#define CPUID_AMD3 QCHAR('c', 'A', 'M', 'D')
-
-#define CPUID_IS(a, b, c, ebx, ecx, edx) \
- (!((ebx ^ (a))|(edx ^ (b))|(ecx ^ (c))))
-
-/*
- * In early loading microcode phase on BSP, boot_cpu_data is not set up yet.
- * x86_vendor() gets vendor id for BSP.
- *
- * In 32 bit AP case, accessing boot_cpu_data needs linear address. To simplify
- * coding, we still use x86_vendor() to get vendor id for AP.
- *
- * x86_vendor() gets vendor information directly through cpuid.
- */
-static int x86_vendor(void)
-{
- u32 eax = 0x00000000;
- u32 ebx, ecx = 0, edx;
-
- native_cpuid(&eax, &ebx, &ecx, &edx);
-
- if (CPUID_IS(CPUID_INTEL1, CPUID_INTEL2, CPUID_INTEL3, ebx, ecx, edx))
- return X86_VENDOR_INTEL;
-
- if (CPUID_IS(CPUID_AMD1, CPUID_AMD2, CPUID_AMD3, ebx, ecx, edx))
- return X86_VENDOR_AMD;
-
- return X86_VENDOR_UNKNOWN;
-}
-
-static int x86_family(void)
-{
- u32 eax = 0x00000001;
- u32 ebx, ecx = 0, edx;
- int x86;
-
- native_cpuid(&eax, &ebx, &ecx, &edx);
-
- x86 = (eax >> 8) & 0xf;
- if (x86 == 15)
- x86 += (eax >> 20) & 0xff;
-
- return x86;
-}
-
static bool __init check_loader_disabled_bsp(void)
{
#ifdef CONFIG_X86_32
@@ -96,7 +45,7 @@ static bool __init check_loader_disabled_bsp(void)

void __init load_ucode_bsp(void)
{
- int vendor, x86;
+ int vendor, family;

if (check_loader_disabled_bsp())
return;
@@ -105,15 +54,15 @@ void __init load_ucode_bsp(void)
return;

vendor = x86_vendor();
- x86 = x86_family();
+ family = x86_family();

switch (vendor) {
case X86_VENDOR_INTEL:
- if (x86 >= 6)
+ if (family >= 6)
load_ucode_intel_bsp();
break;
case X86_VENDOR_AMD:
- if (x86 >= 0x10)
+ if (family >= 0x10)
load_ucode_amd_bsp();
break;
default:
@@ -132,7 +81,7 @@ static bool check_loader_disabled_ap(void)

void load_ucode_ap(void)
{
- int vendor, x86;
+ int vendor, family;

if (check_loader_disabled_ap())
return;
@@ -141,15 +90,15 @@ void load_ucode_ap(void)
return;

vendor = x86_vendor();
- x86 = x86_family();
+ family = x86_family();

switch (vendor) {
case X86_VENDOR_INTEL:
- if (x86 >= 6)
+ if (family >= 6)
load_ucode_intel_ap();
break;
case X86_VENDOR_AMD:
- if (x86 >= 0x10)
+ if (family >= 0x10)
load_ucode_amd_ap();
break;
default:
@@ -179,18 +128,18 @@ int __init save_microcode_in_initrd(void)

void reload_early_microcode(void)
{
- int vendor, x86;
+ int vendor, family;

vendor = x86_vendor();
- x86 = x86_family();
+ family = x86_family();

switch (vendor) {
case X86_VENDOR_INTEL:
- if (x86 >= 6)
+ if (family >= 6)
reload_ucode_intel();
break;
case X86_VENDOR_AMD:
- if (x86 >= 0x10)
+ if (family >= 0x10)
reload_ucode_amd();
break;
default:
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 372ffc7d929b..c3892244fe3e 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -126,31 +126,6 @@ load_microcode(struct mc_saved_data *mc_saved_data,
}
}

-static u8 get_x86_family(unsigned long sig)
-{
- u8 x86;
-
- x86 = (sig >> 8) & 0xf;
-
- if (x86 == 0xf)
- x86 += (sig >> 20) & 0xff;
-
- return x86;
-}
-
-static u8 get_x86_model(unsigned long sig)
-{
- u8 x86, x86_model;
-
- x86 = get_x86_family(sig);
- x86_model = (sig >> 4) & 0xf;
-
- if (x86 == 0x6 || x86 == 0xf)
- x86_model += ((sig >> 16) & 0xf) << 4;
-
- return x86_model;
-}
-
/*
* Given CPU signature and a microcode patch, this function finds if the
* microcode patch has matching family and model with the CPU.
@@ -159,42 +134,40 @@ static enum ucode_state
matching_model_microcode(struct microcode_header_intel *mc_header,
unsigned long sig)
{
- u8 x86, x86_model;
- u8 x86_ucode, x86_model_ucode;
+ unsigned int fam, model;
+ unsigned int fam_ucode, model_ucode;
struct extended_sigtable *ext_header;
unsigned long total_size = get_totalsize(mc_header);
unsigned long data_size = get_datasize(mc_header);
int ext_sigcount, i;
struct extended_signature *ext_sig;

- x86 = get_x86_family(sig);
- x86_model = get_x86_model(sig);
+ fam = __x86_family(sig);
+ model = x86_model(sig);

- x86_ucode = get_x86_family(mc_header->sig);
- x86_model_ucode = get_x86_model(mc_header->sig);
+ fam_ucode = __x86_family(mc_header->sig);
+ model_ucode = x86_model(mc_header->sig);

- if (x86 == x86_ucode && x86_model == x86_model_ucode)
+ if (fam == fam_ucode && model == model_ucode)
return UCODE_OK;

/* Look for ext. headers: */
if (total_size <= data_size + MC_HEADER_SIZE)
return UCODE_NFOUND;

- ext_header = (struct extended_sigtable *)
- mc_header + data_size + MC_HEADER_SIZE;
+ ext_header = (struct extended_sigtable *)mc_header + data_size + MC_HEADER_SIZE;
+ ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
ext_sigcount = ext_header->count;
- ext_sig = (void *)ext_header + EXT_HEADER_SIZE;

for (i = 0; i < ext_sigcount; i++) {
- x86_ucode = get_x86_family(ext_sig->sig);
- x86_model_ucode = get_x86_model(ext_sig->sig);
+ fam_ucode = __x86_family(ext_sig->sig);
+ model_ucode = x86_model(ext_sig->sig);

- if (x86 == x86_ucode && x86_model == x86_model_ucode)
+ if (fam == fam_ucode && model == model_ucode)
return UCODE_OK;

ext_sig++;
}
-
return UCODE_NFOUND;
}

@@ -376,7 +349,7 @@ out:
static int collect_cpu_info_early(struct ucode_cpu_info *uci)
{
unsigned int val[2];
- u8 x86, x86_model;
+ unsigned int family, model;
struct cpu_signature csig;
unsigned int eax, ebx, ecx, edx;

@@ -391,10 +364,10 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
native_cpuid(&eax, &ebx, &ecx, &edx);
csig.sig = eax;

- x86 = get_x86_family(csig.sig);
- x86_model = get_x86_model(csig.sig);
+ family = __x86_family(csig.sig);
+ model = x86_model(csig.sig);

- if ((x86_model >= 5) || (x86 > 6)) {
+ if ((model >= 5) || (family > 6)) {
/* get processor flags from MSR 0x17 */
native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
csig.pf = 1 << ((val[1] >> 18) & 7);
--
2.2.0.33.gc18b867

2015-02-24 10:39:54

by Borislav Petkov

Subject: [PATCH 09/13] x86/microcode/intel: Simplify generic_load_microcode_early()

From: Borislav Petkov <[email protected]>

* remove state variable and out label
* get rid of completely unused mc_size
* shorten variable names
* get rid of local variables
* don't do assignments in local var declarations for less cluttered code
* finally rename it to the shorter and perfectly fine load_microcode_early()

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 55 ++++++++++++++---------------
1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index c3892244fe3e..2dd8121c979d 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -35,38 +35,35 @@ static struct mc_saved_data {
} mc_saved_data;

static enum ucode_state
-generic_load_microcode_early(struct microcode_intel **mc_saved_p,
- unsigned int mc_saved_count,
- struct ucode_cpu_info *uci)
+load_microcode_early(struct microcode_intel **saved,
+ unsigned int num_saved, struct ucode_cpu_info *uci)
{
struct microcode_intel *ucode_ptr, *new_mc = NULL;
- int new_rev = uci->cpu_sig.rev;
- enum ucode_state state = UCODE_OK;
- unsigned int mc_size;
- struct microcode_header_intel *mc_header;
- unsigned int csig = uci->cpu_sig.sig;
- unsigned int cpf = uci->cpu_sig.pf;
- int i;
+ struct microcode_header_intel *mc_hdr;
+ int new_rev, ret, i;

- for (i = 0; i < mc_saved_count; i++) {
- ucode_ptr = mc_saved_p[i];
+ new_rev = uci->cpu_sig.rev;

- mc_header = (struct microcode_header_intel *)ucode_ptr;
- mc_size = get_totalsize(mc_header);
- if (get_matching_microcode(csig, cpf, ucode_ptr, new_rev)) {
- new_rev = mc_header->rev;
- new_mc = ucode_ptr;
- }
- }
+ for (i = 0; i < num_saved; i++) {
+ ucode_ptr = saved[i];
+ mc_hdr = (struct microcode_header_intel *)ucode_ptr;
+
+ ret = get_matching_microcode(uci->cpu_sig.sig,
+ uci->cpu_sig.pf,
+ ucode_ptr,
+ new_rev);
+ if (!ret)
+ continue;

- if (!new_mc) {
- state = UCODE_NFOUND;
- goto out;
+ new_rev = mc_hdr->rev;
+ new_mc = ucode_ptr;
}

+ if (!new_mc)
+ return UCODE_NFOUND;
+
uci->mc = (struct microcode_intel *)new_mc;
-out:
- return state;
+ return UCODE_OK;
}

static void
@@ -114,13 +111,13 @@ load_microcode(struct mc_saved_data *mc_saved_data,
microcode_pointer(mc_saved_tmp, mc_saved_in_initrd,
initrd_start, count);

- return generic_load_microcode_early(mc_saved_tmp, count, uci);
+ return load_microcode_early(mc_saved_tmp, count, uci);
} else {
#ifdef CONFIG_X86_32
microcode_phys(mc_saved_tmp, mc_saved_data);
- return generic_load_microcode_early(mc_saved_tmp, count, uci);
+ return load_microcode_early(mc_saved_tmp, count, uci);
#else
- return generic_load_microcode_early(mc_saved_data->mc_saved,
+ return load_microcode_early(mc_saved_data->mc_saved,
count, uci);
#endif
}
@@ -775,8 +772,8 @@ void reload_ucode_intel(void)

collect_cpu_info_early(&uci);

- ret = generic_load_microcode_early(mc_saved_data.mc_saved,
- mc_saved_data.mc_saved_count, &uci);
+ ret = load_microcode_early(mc_saved_data.mc_saved,
+ mc_saved_data.mc_saved_count, &uci);
if (ret != UCODE_OK)
return;

--
2.2.0.33.gc18b867

2015-02-24 10:39:34

by Borislav Petkov

Subject: [PATCH 10/13] x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}

From: Borislav Petkov <[email protected]>

... arguments list so that it comes more natural for those functions to
have the signature, processor flags and revision together, before the
rest of the args.

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 5 ++---
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
arch/x86/kernel/cpu/microcode/intel_early.c | 6 +++---
arch/x86/kernel/cpu/microcode/intel_lib.c | 16 +++++++---------
4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index a6b753a131cd..2b9209c46ca9 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -56,10 +56,9 @@ struct extended_sigtable {

#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-extern int
-get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev);
+extern int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc);
extern int microcode_sanity_check(void *mc, int print_err);
-extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
+extern int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc);

static inline int
revision_is_newer(struct microcode_header_intel *mc_header, int rev)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 746e7fd08aad..a41beadb3db9 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -124,7 +124,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
cpf = cpu_sig.pf;
crev = cpu_sig.rev;

- return get_matching_microcode(csig, cpf, mc_intel, crev);
+ return get_matching_microcode(csig, cpf, crev, mc_intel);
}

static int apply_microcode_intel(int cpu)
@@ -226,7 +226,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,

csig = uci->cpu_sig.sig;
cpf = uci->cpu_sig.pf;
- if (get_matching_microcode(csig, cpf, mc, new_rev)) {
+ if (get_matching_microcode(csig, cpf, new_rev, mc)) {
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 2dd8121c979d..51c0b8093c1b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -50,8 +50,8 @@ load_microcode_early(struct microcode_intel **saved,

ret = get_matching_microcode(uci->cpu_sig.sig,
uci->cpu_sig.pf,
- ucode_ptr,
- new_rev);
+ new_rev,
+ ucode_ptr);
if (!ret)
continue;

@@ -252,7 +252,7 @@ static unsigned int _save_mc(struct microcode_intel **mc_saved,
pf = mc_saved_hdr->pf;
new_rev = mc_hdr->rev;

- if (!get_matching_sig(sig, pf, ucode_ptr, new_rev))
+ if (!get_matching_sig(sig, pf, new_rev, ucode_ptr))
continue;

found = 1;
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 7e259d99b0aa..cd47a510a3f1 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -122,10 +122,9 @@ int microcode_sanity_check(void *mc, int print_err)
EXPORT_SYMBOL_GPL(microcode_sanity_check);

/*
- * return 0 - no update found
- * return 1 - found update
+ * Returns 1 if update has been found, 0 otherwise.
*/
-int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev)
+int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc)
{
struct microcode_header_intel *mc_header = mc;
struct extended_sigtable *ext_header;
@@ -153,16 +152,15 @@ int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev)
}

/*
- * return 0 - no update found
- * return 1 - found update
+ * Returns 1 if update has been found, 0 otherwise.
*/
-int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev)
+int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
{
- struct microcode_header_intel *mc_header = mc;
+ struct microcode_header_intel *mc_hdr = mc;

- if (!revision_is_newer(mc_header, rev))
+ if (!revision_is_newer(mc_hdr, rev))
return 0;

- return get_matching_sig(csig, cpf, mc, rev);
+ return get_matching_sig(csig, cpf, rev, mc);
}
EXPORT_SYMBOL_GPL(get_matching_microcode);
--
2.2.0.33.gc18b867

2015-02-24 10:37:57

by Borislav Petkov

Subject: [PATCH 11/13] x86/microcode/intel: Sanitize microcode_pointer()

From: Borislav Petkov <[email protected]>

Shorten variable names and rename it to what it does.

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 51c0b8093c1b..d14ceae8a83d 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -66,16 +66,14 @@ load_microcode_early(struct microcode_intel **saved,
return UCODE_OK;
}

-static void
-microcode_pointer(struct microcode_intel **mc_saved,
- unsigned long *mc_saved_in_initrd,
- unsigned long initrd_start, int mc_saved_count)
+static inline void
+copy_initrd_ptrs(struct microcode_intel **mc_saved, unsigned long *initrd,
+ unsigned long off, int num_saved)
{
int i;

- for (i = 0; i < mc_saved_count; i++)
- mc_saved[i] = (struct microcode_intel *)
- (mc_saved_in_initrd[i] + initrd_start);
+ for (i = 0; i < num_saved; i++)
+ mc_saved[i] = (struct microcode_intel *)(initrd[i] + off);
}

#ifdef CONFIG_X86_32
@@ -99,17 +97,14 @@ microcode_phys(struct microcode_intel **mc_saved_tmp,
#endif

static enum ucode_state
-load_microcode(struct mc_saved_data *mc_saved_data,
- unsigned long *mc_saved_in_initrd,
- unsigned long initrd_start,
- struct ucode_cpu_info *uci)
+load_microcode(struct mc_saved_data *mc_saved_data, unsigned long *initrd,
+ unsigned long initrd_start, struct ucode_cpu_info *uci)
{
struct microcode_intel *mc_saved_tmp[MAX_UCODE_COUNT];
unsigned int count = mc_saved_data->mc_saved_count;

if (!mc_saved_data->mc_saved) {
- microcode_pointer(mc_saved_tmp, mc_saved_in_initrd,
- initrd_start, count);
+ copy_initrd_ptrs(mc_saved_tmp, initrd, initrd_start, count);

return load_microcode_early(mc_saved_tmp, count, uci);
} else {
@@ -674,7 +669,7 @@ int __init save_microcode_in_initrd_intel(void)
if (count == 0)
return ret;

- microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
+ copy_initrd_ptrs(mc_saved, mc_saved_in_initrd, initrd_start, count);
ret = save_microcode(&mc_saved_data, mc_saved, count);
if (ret)
pr_err("Cannot save microcode patches from initrd.\n");
--
2.2.0.33.gc18b867

2015-02-24 10:38:47

by Borislav Petkov

Subject: [PATCH 12/13] x86/microcode/intel: Check scan_microcode()'s retval

From: Borislav Petkov <[email protected]>

... and do not attempt to load anything in case of error.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index d14ceae8a83d..2fa9ad39e0e5 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -516,9 +516,9 @@ EXPORT_SYMBOL_GPL(save_mc_for_early);

static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
static __init enum ucode_state
-scan_microcode(unsigned long start, unsigned long size,
- struct mc_saved_data *mc_saved_data,
- unsigned long *mc_saved_in_initrd, struct ucode_cpu_info *uci)
+scan_microcode(struct mc_saved_data *mc_saved_data, unsigned long *initrd,
+ unsigned long start, unsigned long size,
+ struct ucode_cpu_info *uci)
{
struct cpio_data cd;
long offset = 0;
@@ -536,8 +536,7 @@ scan_microcode(unsigned long start, unsigned long size,
return UCODE_ERROR;

return get_matching_model_microcode(0, start, cd.data, cd.size,
- mc_saved_data, mc_saved_in_initrd,
- uci);
+ mc_saved_data, initrd, uci);
}

/*
@@ -681,16 +680,19 @@ int __init save_microcode_in_initrd_intel(void)

static void __init
_load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
- unsigned long *mc_saved_in_initrd,
+ unsigned long *initrd,
unsigned long start, unsigned long size)
{
struct ucode_cpu_info uci;
enum ucode_state ret;

collect_cpu_info_early(&uci);
- scan_microcode(start, size, mc_saved_data, mc_saved_in_initrd, &uci);

- ret = load_microcode(mc_saved_data, mc_saved_in_initrd, start, &uci);
+ ret = scan_microcode(mc_saved_data, initrd, start, size, &uci);
+ if (ret != UCODE_OK)
+ return;
+
+ ret = load_microcode(mc_saved_data, initrd, start, &uci);
if (ret != UCODE_OK)
return;

--
2.2.0.33.gc18b867

2015-02-24 10:38:23

by Borislav Petkov

Subject: [PATCH 13/13] x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

From: Borislav Petkov <[email protected]>

When doing

echo 1 > /sys/devices/system/cpu/microcode/reload

in order to reload microcode, I get:

microcode: Total microcode saved: 1
BUG: using smp_processor_id() in preemptible [00000000] code: bash/2606
caller is debug_smp_processor_id+0x17/0x20
CPU: 1 PID: 2606 Comm: bash Not tainted 3.19.0-rc7+ #9
Hardware name: LENOVO 2320CTO/2320CTO, BIOS G2ET86WW (2.06 ) 11/13/2012
ffffffff81a4266d ffff8802131db808 ffffffff81666588 0000000000000007
0000000000000001 ffff8802131db838 ffffffff812e6eef ffff8802131db868
00000000000306a9 0000000000000010 0000000000000015 ffff8802131db848
Call Trace:
dump_stack
check_preemption_disabled
debug_smp_processor_id
show_saved_mc
? save_microcode.constprop.8
save_mc_for_early
? print_context_stack
? dump_trace
? __bfs
? mark_held_locks
? get_page_from_freelist
? trace_hardirqs_on_caller
? trace_hardirqs_on
? __alloc_pages_nodemask
? __get_vm_area_node
? map_vm_area
? __vmalloc_node_range
? generic_load_microcode
generic_load_microcode
? microcode_fini_cpu
request_microcode_fw
reload_store
dev_attr_store
sysfs_kf_write
kernfs_fop_write
vfs_write
? sysret_check
SyS_write
system_call_fastpath
microcode: CPU1: sig=0x306a9, pf=0x10, rev=0x15
microcode: mc_saved[0]: sig=0x306a9, pf=0x12, rev=0x1b, toal size=0x3000, date = 2014-05-29

because we're using smp_processor_id() in preemtible context. And we
don't really need to use it there because the microcode container we're
dumping is global and CPU-specific info is irrelevant.

While at it, make pr_* stuff use "microcode: " prefix for easier
grepping and document how to enable the DEBUG build.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 2fa9ad39e0e5..079d8d41fba3 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -16,6 +16,14 @@
* as published by the Free Software Foundation; either version
* 2 of the License, or (at your option) any later version.
*/
+
+/*
+ * This needs to be before all headers so that pr_debug in printk.h doesn't turn
+ * printk calls into no_printk().
+ *
+ *#define DEBUG
+ */
+
#include <linux/module.h>
#include <linux/mm.h>
#include <linux/slab.h>
@@ -28,6 +36,9 @@
#include <asm/tlbflush.h>
#include <asm/setup.h>

+#undef pr_fmt
+#define pr_fmt(fmt) "microcode: " fmt
+
static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
static struct mc_saved_data {
unsigned int mc_saved_count;
@@ -398,8 +409,7 @@ static void __ref show_saved_mc(void)
sig = uci.cpu_sig.sig;
pf = uci.cpu_sig.pf;
rev = uci.cpu_sig.rev;
- pr_debug("CPU%d: sig=0x%x, pf=0x%x, rev=0x%x\n",
- smp_processor_id(), sig, pf, rev);
+ pr_debug("CPU: sig=0x%x, pf=0x%x, rev=0x%x\n", sig, pf, rev);

for (i = 0; i < mc_saved_data.mc_saved_count; i++) {
struct microcode_header_intel *mc_saved_header;
--
2.2.0.33.gc18b867

2015-02-24 16:18:28

by Quentin Casasnovas

Subject: Re: [PATCH 02/13] x86/microcode/intel: Do the mc_saved_src NULL check first

On Tue, Feb 24, 2015 at 11:37:01AM +0100, Borislav Petkov wrote:
> @@ -213,39 +213,46 @@ save_microcode(struct mc_saved_data *mc_saved_data,
> /*
> * Copy new microcode data.
> */
> - mc_saved_p = kmalloc(mc_saved_count*sizeof(struct microcode_intel *),
> + saved_ptr = kmalloc(mc_saved_count * sizeof(struct microcode_intel *),
> GFP_KERNEL);

I'd be tempted to use kcalloc() in these cases but I suppose it's just
personnal preference - it avoids having to make sure your multiplication
cannot overflow when reviewing.

> - if (!mc_saved_p)
> + if (!saved_ptr)
> return -ENOMEM;
>
> for (i = 0; i < mc_saved_count; i++) {
> - struct microcode_intel *mc = mc_saved_src[i];
> - struct microcode_header_intel *mc_header = &mc->hdr;
> - unsigned long mc_size = get_totalsize(mc_header);
> - mc_saved_p[i] = kmalloc(mc_size, GFP_KERNEL);
> - if (!mc_saved_p[i]) {
> - ret = -ENOMEM;
> - goto err;
> - }
> + struct microcode_header_intel *mc_hdr;
> + struct microcode_intel *mc;
> + unsigned long size;
> +
> if (!mc_saved_src[i]) {
> ret = -EINVAL;
> goto err;
> }

... though in this particular case, I think using kcalloc() above would
also prevent the introduction of a kfree() on random junk if
!mc_saved_src[0] since you'll jump straight to err which will
kfree(saved_ptr[0]), which isn't initialized yet. So it might be worth the
change :)

>
> err:
> for (j = 0; j <= i; j++)
> - kfree(mc_saved_p[j]);
> - kfree(mc_saved_p);
> + kfree(saved_ptr[j]);
> + kfree(saved_ptr);
>

Quentin

2015-02-24 16:19:22

by Quentin Casasnovas

Subject: Re: [PATCH 03/13] x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()

On Tue, Feb 24, 2015 at 11:37:02AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Allocate it on the helper's _load_ucode_intel_bsp() stack instead and do
> not hand it down.
>

Going further, could you not even make uci a static global variable and
have collect_cpu_info_early() called only _once_ to fill the information
in, then you can remove the uci argument from all the other functions as
well? Just a suggestion anyway.

Quentin

2015-02-24 16:20:06

by Quentin Casasnovas

Subject: Re: [PATCH 04/13] x86/microcode/intel: Simplify load_ucode_intel_bsp()

On Tue, Feb 24, 2015 at 11:37:03AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Don't compute start and end from start and size in order to compute size
> again down the path in scan_microcode(). So pass size directly instead
> and simplify a bunch. Shorten variable names and remove useless ones.
>
> No functionality change.
>

Unless I can't read, which is likely to be true, I think I might have
spotted one.

> @@ -555,12 +555,10 @@ EXPORT_SYMBOL_GPL(save_mc_for_early);
>
> static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
> static __init enum ucode_state
> -scan_microcode(unsigned long start, unsigned long end,
> - struct mc_saved_data *mc_saved_data,
> - unsigned long *mc_saved_in_initrd,
> - struct ucode_cpu_info *uci)
> +scan_microcode(unsigned long start, unsigned long size,
> + struct mc_saved_data *mc_saved_data,
> + unsigned long *mc_saved_in_initrd, struct ucode_cpu_info *uci)
> {
> - unsigned int size = end - start + 1;

... here `size = end - start + 1`, so basically the original size in
`hdr.ramdisk_image` *plus one*, whereas the `size` you're passing around
does not include it. I'm not saying it's wrong, quite the opposite I think
you're fixing an off-by-one error present in the original code at the same
time :)

> + u64 start, size;
> #ifdef CONFIG_X86_32
> - struct boot_params *boot_params_p;
> + struct boot_params *p;
>
> - boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
> - ramdisk_image = boot_params_p->hdr.ramdisk_image;
> - ramdisk_size = boot_params_p->hdr.ramdisk_size;
> - initrd_start_early = ramdisk_image;
> - initrd_end_early = initrd_start_early + ramdisk_size;
> + p = (struct boot_params *)__pa_nodebug(&boot_params);
> + start = p->hdr.ramdisk_image;
> + size = p->hdr.ramdisk_size;

Here is your size.

Quentin

2015-02-24 16:21:10

by Quentin Casasnovas

Subject: Re: [PATCH 05/13] x86/microcode/intel: Make _save_mc() return the updated saved count

On Tue, Feb 24, 2015 at 11:37:04AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> ... of microcode patches instead of handing in a pointer which is used
> for I/O in an otherwise void function.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel_early.c | 29 ++++++++++++++---------------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index ffeac5d62eca..ee74e7726c33 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -264,17 +264,18 @@ err:
> * - or if it is a newly discovered microcode patch.
> *
> * The microcode patch should have matching model with CPU.
> + *
> + * Returns: The updated number @num_saved of saved microcode patches.
> */
> -static void _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr,
> - unsigned int *mc_saved_count_p)
> +static unsigned int _save_mc(struct microcode_intel **mc_saved,
> + u8 *ucode_ptr, unsigned int num_saved)
> {
> - int i;
> - int found = 0;
> - unsigned int mc_saved_count = *mc_saved_count_p;
> struct microcode_header_intel *mc_header;
> + int found = 0, i;
>
> mc_header = (struct microcode_header_intel *)ucode_ptr;
> - for (i = 0; i < mc_saved_count; i++) {
> +
> + for (i = 0; i < num_saved; i++) {

Minor comment: since num_saved is unsigned, I think it would be better to
just use an unsigned int for `i` as well.

> unsigned int sig, pf;
> unsigned int new_rev;
> struct microcode_header_intel *mc_saved_header =
> @@ -291,21 +292,20 @@ static void _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr,
> * Replace the older one with this newer
> * one.
> */
> - mc_saved[i] =
> - (struct microcode_intel *)ucode_ptr;
> + mc_saved[i] = (struct microcode_intel *)ucode_ptr;
> break;
> }
> }
> }
> - if (i >= mc_saved_count && !found)
> +
> + if (i >= num_saved && !found)

While at it, I could not find that `i` would ever be bigger than
`num_saved` so maybe you can just test for equality instead? It just makes
it clearer what the code does.

Quentin

2015-02-24 16:21:32

by Quentin Casasnovas

Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

On Tue, Feb 24, 2015 at 11:37:06AM +0100, Borislav Petkov wrote:
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -60,8 +60,12 @@ extern int
> get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev);
> extern int microcode_sanity_check(void *mc, int print_err);
> extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
> -extern int
> -update_match_revision(struct microcode_header_intel *mc_header, int rev);
> +
> +static inline int
> +revision_is_newer(struct microcode_header_intel *mc_header, int rev)
> +{
> + return (mc_header->rev <= rev) ? 0 : 1;
> +}
>

Minor nit-pick, if you reverse your inequality, you don't need for the
ternary operator.

Quentin

2015-02-24 16:21:58

by Quentin Casasnovas

Subject: Re: [PATCH 08/13] x86/microcode: Consolidate family,model, ... code

On Tue, Feb 24, 2015 at 11:37:07AM +0100, Borislav Petkov wrote:
> @@ -159,42 +134,40 @@ static enum ucode_state
> matching_model_microcode(struct microcode_header_intel *mc_header,
> unsigned long sig)
> {
> - u8 x86, x86_model;
> - u8 x86_ucode, x86_model_ucode;
> + unsigned int fam, model;
> + unsigned int fam_ucode, model_ucode;
> struct extended_sigtable *ext_header;
> unsigned long total_size = get_totalsize(mc_header);
> unsigned long data_size = get_datasize(mc_header);
> int ext_sigcount, i;
> struct extended_signature *ext_sig;
>
> - x86 = get_x86_family(sig);
> - x86_model = get_x86_model(sig);
> + fam = __x86_family(sig);
> + model = x86_model(sig);
>
> - x86_ucode = get_x86_family(mc_header->sig);
> - x86_model_ucode = get_x86_model(mc_header->sig);
> + fam_ucode = __x86_family(mc_header->sig);
> + model_ucode = x86_model(mc_header->sig);
>
> - if (x86 == x86_ucode && x86_model == x86_model_ucode)
> + if (fam == fam_ucode && model == model_ucode)
> return UCODE_OK;
>
> /* Look for ext. headers: */
> if (total_size <= data_size + MC_HEADER_SIZE)
> return UCODE_NFOUND;
>
> - ext_header = (struct extended_sigtable *)
> - mc_header + data_size + MC_HEADER_SIZE;
> + ext_header = (struct extended_sigtable *)mc_header + data_size + MC_HEADER_SIZE;
> + ext_sig = (void *)ext_header + EXT_HEADER_SIZE;

I think we have another serious problem here, both in the original code and
in your patch - mc_header will first be casted to (unsigned long*) then
we'll add data_size and MC_HEADER_SIZE, potentially going way further than
intended. Same remark for ext_sig.

Quentin

2015-02-24 16:22:15

by Quentin Casasnovas

Subject: Re: [PATCH 10/13] x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}

On Tue, Feb 24, 2015 at 11:37:09AM +0100, Borislav Petkov wrote:
> @@ -153,16 +152,15 @@ int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev)
> }
>
> /*
> - * return 0 - no update found
> - * return 1 - found update
> + * Returns 1 if update has been found, 0 otherwise.
> */
> -int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev)
> +int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)

Why not rename the function to is_microcode_matching() while at it? :)

Quentin

2015-02-24 16:22:37

by Quentin Casasnovas

Subject: Re: [PATCH 13/13] x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

On Tue, Feb 24, 2015 at 11:37:12AM +0100, Borislav Petkov wrote:
>
> While at it, make pr_* stuff use "microcode: " prefix for easier
> grepping and document how to enable the DEBUG build.
>
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -16,6 +16,14 @@
> * as published by the Free Software Foundation; either version
> * 2 of the License, or (at your option) any later version.
> */
> +
> +/*
> + * This needs to be before all headers so that pr_debug in printk.h doesn't turn
> + * printk calls into no_printk().
> + *
> + *#define DEBUG
> + */
> +

Hmm I might be completely wrong but I thought the whole point of pr_debug()
was to have something dynamic at runtime as opposed to compiled in? What
am I missing?

Quentin

2015-02-24 16:38:50

by Quentin Casasnovas

Subject: Re: [PATCH 00/13] x86/microcode: Intel early loader cleanups

On Tue, Feb 24, 2015 at 11:36:59AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Hi,
>
> so this is something which got started in the aftermath of a discussion
> about some robustifying fixes to the microcode loader by Quentin.
> Everyone agrees that current code needs a good rubbing so here's part
> one of that. More to come later, let's not overwhelm people with huge
> patchsets.
>
> All patches are cleanups and simplifications in an attempt to make the
> code more readable and simpler and enable follow-up improvements.
>

I've sent my initial review answers but adding Fenghua to the CC list since
he's the original author and I can't test your code.

FWIW I think it's a nice start!

Quentin

2015-02-24 16:47:42

by Borislav Petkov

Subject: Re: [PATCH 13/13] x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

On Tue, Feb 24, 2015 at 05:24:27PM +0100, Quentin Casasnovas wrote:
> Hmm I might be completely wrong but I thought the whole point of pr_debug()
> was to have something dynamic at runtime as opposed to compiled in? What
> am I missing?

That's the CONFIG_DYNAMIC_DEBUG case. But the functionality here is
ifdeffed off anyway, which makes it really clumsy to use as it is now.
So it will need more work.

Which leads me to say what I actually wanted to say:

Thanks for the review, very good points. I had spotted some of them
myself but had to restrain myself not to do them now for the very
simple reason: we want this code first cleaned up nicely, in small and
self-contained pieces so that no regressions get introduced from more
involved patches (debugging early microcode loader issues is nasty).

Then we can start improving it, once enough rust is shaken off.

Now, I have limited time so, if, in case you wanted to do small and
clean patchsets cleaning up this more or improving some aspects of it
and tested them and sent them to me, I'll gladly give them a good look
and test them here too. :-)

But you don't have to, this is just a suggestion anyway - I just get the
feeling that you like looking at it and wanted to mention that in case
you'd like to help out cleaning it up, you're welcome to do so.

:-)

Thanks again.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-02-24 18:29:29

by Borislav Petkov

Subject: Re: [PATCH 03/13] x86/microcode/intel: Get rid of last arg to load_ucode_intel_bsp()

On Tue, Feb 24, 2015 at 05:21:04PM +0100, Quentin Casasnovas wrote:
> Going further, could you not even make uci a static global variable and
> have collect_cpu_info_early() called only _once_ to fill the information
> in, then you can remove the uci argument from all the other functions as
> well? Just a suggestion anyway.

Yeah, I could but this is not on a fast path and we want to,
theoretically at least, support mixed revision microcode loading
where the BSP and APs have different revisions. Thus the separate
collect_cpu_info_early() calls.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-02-24 18:31:31

by Borislav Petkov

Subject: Re: [PATCH 04/13] x86/microcode/intel: Simplify load_ucode_intel_bsp()

On Tue, Feb 24, 2015 at 05:21:51PM +0100, Quentin Casasnovas wrote:
> ... here `size = end - start + 1`, so basically the original size in
> `hdr.ramdisk_image` *plus one*, whereas the `size` you're passing around
> does not include it. I'm not saying it's wrong, quite the opposite I think
> you're fixing an off-by-one error present in the original code at the same
> time :)

I know, right :-)

I was scratching my head about it and if using the size directly is
wrong then something's genuinely b0rked with the design of this thing.

I'll correct the commit message.

Tanks.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-02-25 09:40:06

by Quentin Casasnovas

Subject: Re: [PATCH 13/13] x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

On Tue, Feb 24, 2015 at 05:48:17PM +0100, Borislav Petkov wrote:
>
> Thanks for the review, very good points. I had spotted some of them
> myself but had to restrain myself not to do them now for the very
> simple reason: we want this code first cleaned up nicely, in small and
> self-contained pieces so that no regressions get introduced from more
> involved patches (debugging early microcode loader issues is nasty).
>
> Then we can start improving it, once enough rust is shaken off.

Yup that's how I understood your patchset :) If I may though, I think the
issue I raised on your patch 8 is serious enough to get a fix before you
merge this patchset - it should just be a matter of adding some parentheses
at the correct place, and should be a good candidate for -stable.

>
> Now, I have limited time so, if, in case you wanted to do small and
> clean patchsets cleaning up this more or improving some aspects of it
> and tested them and sent them to me, I'll gladly give them a good look
> and test them here too. :-)
>
> But you don't have to, this is just a suggestion anyway - I just get the
> feeling that you like looking at it and wanted to mention that in case
> you'd like to help out cleaning it up, you're welcome to do so.
>

I have also very limited time to allocate for this, but I can surely help
with reviewing. If I manage to get some spare time, I'll make sure to try
to contribute as well, no guarantee for now though!

Quentin

2015-02-25 17:57:00

by Borislav Petkov

Subject: Re: [PATCH 13/13] x86/microcode/intel: Fix printing of microcode blobs in show_saved_mc()

On Wed, Feb 25, 2015 at 10:41:25AM +0100, Quentin Casasnovas wrote:
> Yup that's how I understood your patchset :) If I may though, I think the
> issue I raised on your patch 8 is serious enough to get a fix before you
> merge this patchset - it should just be a matter of adding some parentheses
> at the correct place, and should be a good candidate for -stable.

Right, so ext_sig is already correct because (void *) arithmetic is a
gnu extension and works as expected when doing char ptr arithmetic. So
the minimal fix should be this:

---
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 079d8d41fba3..860006e3b5c1 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -158,7 +158,7 @@ matching_model_microcode(struct microcode_header_intel *mc_header,
if (total_size <= data_size + MC_HEADER_SIZE)
return UCODE_NFOUND;

- ext_header = (struct extended_sigtable *)mc_header + data_size + MC_HEADER_SIZE;
+ ext_header = (void *)mc_header + data_size + MC_HEADER_SIZE;
ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
ext_sigcount = ext_header->count;

---

Now, since you spotted it, if you sent me a patch *not* ontop of my
patchset but ontop of 4.0-rc1, I'll gladly take it and prepend my
patchset with it.

> I have also very limited time to allocate for this, but I can surely
> help with reviewing. If I manage to get some spare time, I'll make
> sure to try to contribute as well, no guarantee for now though!

Ok, sure, absolutely. I'm already happy with people even looking at it
and poking holes. :-)

Thanks.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-04-10 11:14:46

by Borislav Petkov

Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

On Tue, Feb 24, 2015 at 05:23:18PM +0100, Quentin Casasnovas wrote:
> Minor nit-pick, if you reverse your inequality, you don't need for the
> ternary operator.

Yeah, so I started looking at that and it seems the rabbit hole goes
deeper.

Let's look at the call to revision_is_newer() in _save_mc():

save_mc:

new_rev = mc_hdr->rev;

...

if (!revision_is_newer(mc_hdr, new_rev))
->
if (!((mc_hdr->rev <= new_rev) ? 0 : 1))
->
if (!((mc_hdr->rev <= mc_hdr->rev) ? 0 : 1))
->
if (!0)
->
if (1)
continue;

So basically @new_rev was wrong to use there in the first place. And it
is there since it got committed in 3.13. If anything, it should've been
old_rev FAIK, or

if (!revision_is_newer(mc_saved_hdr, new_rev))

... whateva...

And to confirm this and so I can stop rubbing my eyes, let's look at the
asm:

*
* Returns: The updated number @num_saved of saved microcode patches.
*/
static unsigned int _save_mc(struct microcode_intel **mc_saved,
u8 *ucode_ptr, unsigned int num_saved)
{
ffffffff81033f25: 4c 89 65 e0 mov %r12,-0x20(%rbp)
ffffffff81033f29: 4c 89 6d e8 mov %r13,-0x18(%rbp)
ffffffff81033f2d: 49 89 f4 mov %rsi,%r12


ucode_ptr lands in %r12

...

new_rev = mc_hdr->rev;
ffffffff81033f4f: 45 8b 74 24 04 mov 0x4(%r12),%r14d


new_rev is the second unsigned int in the struct thus new_rev = %r14d = *(%r12 + 4)

...

if (!revision_is_newer(mc_hdr, new_rev))
ffffffff81033f70: 45 3b 74 24 04 cmp 0x4(%r12),%r14d
ffffffff81033f75: 73 21 jae ffffffff81033f98 <_save_mc+0x88>


So we practically end up doing

cmpl 0x4(%r12), 0x4(%r12)

and gcc doesn't optimize it away even.


Oh well, let's kill this function completely:


---
From: Borislav Petkov <[email protected]>
Date: Fri, 10 Apr 2015 12:50:57 +0200
Subject: [PATCH] x86/microcode/intel: Get rid of revision_is_newer()

It is a one-liner for checking microcode header revisions. On top of
that, it can be used wrong as it was the case in _save_mc(). Get rid of
it.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 6 ------
arch/x86/kernel/cpu/microcode/intel_early.c | 2 +-
arch/x86/kernel/cpu/microcode/intel_lib.c | 6 +++---
3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 2b9209c46ca9..a4df6d292228 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -60,12 +60,6 @@ extern int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
extern int microcode_sanity_check(void *mc, int print_err);
extern int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc);

-static inline int
-revision_is_newer(struct microcode_header_intel *mc_header, int rev)
-{
- return (mc_header->rev <= rev) ? 0 : 1;
-}
-
#ifdef CONFIG_MICROCODE_INTEL_EARLY
extern void __init load_ucode_intel_bsp(void);
extern void load_ucode_intel_ap(void);
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 98d320c25dff..edae46ebdf32 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -262,7 +262,7 @@ static unsigned int _save_mc(struct microcode_intel **mc_saved,

found = 1;

- if (!revision_is_newer(mc_hdr, new_rev))
+ if (mc_hdr->rev <= mc_saved_hdr->rev)
continue;

/*
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index cd47a510a3f1..63b0a2e059ee 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -154,13 +154,13 @@ int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc)
/*
* Returns 1 if update has been found, 0 otherwise.
*/
-int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
+int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)
{
struct microcode_header_intel *mc_hdr = mc;

- if (!revision_is_newer(mc_hdr, rev))
+ if (mc_hdr->rev <= new_rev)
return 0;

- return get_matching_sig(csig, cpf, rev, mc);
+ return get_matching_sig(csig, cpf, new_rev, mc);
}
EXPORT_SYMBOL_GPL(get_matching_microcode);
--
2.3.5

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-04-10 11:52:57

by Quentin Casasnovas

Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

On Fri, Apr 10, 2015 at 01:12:18PM +0200, Borislav Petkov wrote:
> On Tue, Feb 24, 2015 at 05:23:18PM +0100, Quentin Casasnovas wrote:
> > Minor nit-pick, if you reverse your inequality, you don't need for the
> > ternary operator.
>
> Yeah, so I started looking at that and it seems the rabbit hole goes
> deeper.
>
> Let's look at the call to revision_is_newer() in _save_mc():
>
> save_mc:
>
> new_rev = mc_hdr->rev;
>
> ...
>
> if (!revision_is_newer(mc_hdr, new_rev))
> ->

Ha good catch!

BTW, I could not find that the 'rev' argument to get_matching_sig() was
used at all..

>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Fri, 10 Apr 2015 12:50:57 +0200
> Subject: [PATCH] x86/microcode/intel: Get rid of revision_is_newer()
>
> ...
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index cd47a510a3f1..63b0a2e059ee 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -154,13 +154,13 @@ int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc)
> /*
> * Returns 1 if update has been found, 0 otherwise.
> */
> -int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
> +int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)

If we're to rename 'rev', maybe calling it 'cpu_rev' would make it more
obvious where this variable is coming from?

> {
> struct microcode_header_intel *mc_hdr = mc;
>
> - if (!revision_is_newer(mc_hdr, rev))
> + if (mc_hdr->rev <= new_rev)
> return 0;
>
> - return get_matching_sig(csig, cpf, rev, mc);
> + return get_matching_sig(csig, cpf, new_rev, mc);
> }
> EXPORT_SYMBOL_GPL(get_matching_microcode);

Anyway you patch looks good to me!

Quentin

2015-04-10 12:11:20

by Borislav Petkov

Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

On Fri, Apr 10, 2015 at 01:54:56PM +0200, Quentin Casasnovas wrote:
> BTW, I could not find that the 'rev' argument to get_matching_sig() was
> used at all..

That's the next patch in the queue for 4.2 :-)

> If we're to rename 'rev', maybe calling it 'cpu_rev' would make it more
> obvious where this variable is coming from?

It is actually a microcode revision. I.e., microcode patch version
number. I'll add doc comments after the cleanup is done.

> Anyway you patch looks good to me!

Thanks, I'll add your ACK.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-05 09:14:16

by Borislav Petkov

Subject: Re: [PATCH 10/13] x86/microcode/intel: Move mc arg last in get_matching_{microcode|sig}

On Tue, Feb 24, 2015 at 05:24:07PM +0100, Quentin Casasnovas wrote:
> Why not rename the function to is_microcode_matching() while at it? :)

Even better, see below. :-)

---
From: Borislav Petkov <[email protected]>
Date: Tue, 5 May 2015 11:10:44 +0200
Subject: [PATCH] x86/microcode/intel: Rename get_matching_microcode

... to has_newer_microcode() as it does exactly that: checks whether
binary data @mc has newer microcode patch than the applied one. Move @mc
to be the first function arg too.

Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 2 +-
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
arch/x86/kernel/cpu/microcode/intel_early.c | 8 ++++----
arch/x86/kernel/cpu/microcode/intel_lib.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 3564299863f0..8e87e6fe98b5 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -56,7 +56,7 @@ struct extended_sigtable {

#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-extern int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc);
+extern int has_newer_microcode(void *mc, unsigned int csig, int cpf, int rev);
extern int microcode_sanity_check(void *mc, int print_err);
extern int get_matching_sig(unsigned int csig, int cpf, void *mc);

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index a41beadb3db9..c82fd1d57bca 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -124,7 +124,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
cpf = cpu_sig.pf;
crev = cpu_sig.rev;

- return get_matching_microcode(csig, cpf, crev, mc_intel);
+ return has_newer_microcode(mc_intel, csig, cpf, crev);
}

static int apply_microcode_intel(int cpu)
@@ -226,7 +226,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,

csig = uci->cpu_sig.sig;
cpf = uci->cpu_sig.pf;
- if (get_matching_microcode(csig, cpf, new_rev, mc)) {
+ if (has_newer_microcode(mc, csig, cpf, new_rev)) {
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index ccd474a7a59e..5f828268357d 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -59,10 +59,10 @@ load_microcode_early(struct microcode_intel **saved,
ucode_ptr = saved[i];
mc_hdr = (struct microcode_header_intel *)ucode_ptr;

- ret = get_matching_microcode(uci->cpu_sig.sig,
- uci->cpu_sig.pf,
- new_rev,
- ucode_ptr);
+ ret = has_newer_microcode(ucode_ptr,
+ uci->cpu_sig.sig,
+ uci->cpu_sig.pf,
+ new_rev);
if (!ret)
continue;

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 7de293726923..425f8e29b795 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -154,7 +154,7 @@ int get_matching_sig(unsigned int csig, int cpf, void *mc)
/*
* Returns 1 if update has been found, 0 otherwise.
*/
-int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)
+int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev)
{
struct microcode_header_intel *mc_hdr = mc;

@@ -163,4 +163,4 @@ int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)

return get_matching_sig(csig, cpf, mc);
}
-EXPORT_SYMBOL_GPL(get_matching_microcode);
+EXPORT_SYMBOL_GPL(has_newer_microcode);
--
2.3.5

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--