* [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD)
@ 2020-03-31 10:05 Andrew Cooper
2020-03-31 10:05 ` [PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing Andrew Cooper
` (10 more replies)
0 siblings, 11 replies; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
The first patch definitely needs backporting. Second is a good candidate as
well. Everything else probably not.
This follows similar cleanup on the Intel side, removing gratuitous memory
allocations (both interms of number, and indirection), and fixes several
things to be more uniform (handling of cpu_sig->sig, and parsing of multiple
containers.
Andrew Cooper (11):
x86/ucode/amd: Fix more potential buffer overruns with microcode parsing
x86/ucode/amd: Move check_final_patch_levels() to apply_microcode()
x86/ucode/amd: Don't use void * for microcode_patch->mpb
x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info()
x86/ucode/amd: Overhaul the equivalent cpu table handling completely
x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd()
x86/ucode/amd: Alter API for microcode_fits()
x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
x86/ucode/amd: Fold structures together
x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
xen/arch/x86/cpu/microcode/amd.c | 512 +++++++++++++--------------------------
xen/include/asm-x86/microcode.h | 2 +-
2 files changed, 176 insertions(+), 338 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 10:05 ` [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode() Andrew Cooper
` (9 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
cpu_request_microcode() doesn't know the buffer is at least 4 bytes long
before inspecting UCODE_MAGIC.
install_equiv_cpu_table() doesn't know the boundary of the buffer it is
interpreting as an equivalency table. This case was clearly observed at one
point in the past, given the subsequent overrun detection, but without
comprehending that the damage was already done.
Make the logic consistent with container_fast_forward() and pass size_left in
to install_equiv_cpu_table().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 6bf3a054d3..796745e928 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -303,11 +303,20 @@ static int get_ucode_from_buffer_amd(
static int install_equiv_cpu_table(
struct microcode_amd *mc_amd,
const void *data,
+ size_t size_left,
size_t *offset)
{
- const struct mpbhdr *mpbuf = data + *offset + 4;
+ const struct mpbhdr *mpbuf;
const struct equiv_cpu_entry *eq;
+ if ( size_left < (sizeof(*mpbuf) + 4) ||
+ (mpbuf = data + *offset + 4,
+ size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
+ {
+ printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
+ return -EINVAL;
+ }
+
*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
@@ -417,7 +426,8 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
current_cpu_id = cpuid_eax(0x00000001);
- if ( *(const uint32_t *)buf != UCODE_MAGIC )
+ if ( bufsize < 4 ||
+ *(const uint32_t *)buf != UCODE_MAGIC )
{
printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
error = -EINVAL;
@@ -447,24 +457,13 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
*/
while ( offset < bufsize )
{
- error = install_equiv_cpu_table(mc_amd, buf, &offset);
+ error = install_equiv_cpu_table(mc_amd, buf, bufsize - offset, &offset);
if ( error )
{
printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
break;
}
- /*
- * Could happen as we advance 'offset' early
- * in install_equiv_cpu_table
- */
- if ( offset > bufsize )
- {
- printk(KERN_ERR "microcode: Microcode buffer overrun\n");
- error = -EINVAL;
- break;
- }
-
if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
&equiv_cpu_id) )
break;
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
2020-03-31 10:05 ` [PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:27 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb Andrew Cooper
` (8 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
The microcode revision of whichever CPU runs cpu_request_microcode() is not
necessarily applicable to other CPUs.
If the BIOS left us with asymmetric microcode, rejecting updates in
cpu_request_microcode() would prevent us levelling the system even if only up
to the final level. Also, failing to cache microcode misses an opportunity to
get beyond the final level via the S3 path.
Move check_final_patch_levels() earlier and use it in apply_microcode().
Reword the error message to be more informative, and use -ENXIO as this corner
case has nothing to do with permissions.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 83 ++++++++++++++++++----------------------
1 file changed, 38 insertions(+), 45 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 796745e928..4245dc13bb 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -119,6 +119,36 @@ static bool_t verify_patch_size(uint32_t patch_size)
return (patch_size <= max_size);
}
+static bool check_final_patch_levels(const struct cpu_signature *sig)
+{
+ /*
+ * The 'final_levels' of patch ids have been obtained empirically.
+ * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
+ * for details of the issue. The short version is that people
+ * using certain Fam10h systems noticed system hang issues when
+ * trying to update microcode levels beyond the patch IDs below.
+ * From internal discussions, we gathered that OS/hypervisor
+ * cannot reliably perform microcode updates beyond these levels
+ * due to hardware issues. Therefore, we need to abort microcode
+ * update process if we hit any of these levels.
+ */
+ static const unsigned int final_levels[] = {
+ 0x01000098,
+ 0x0100009f,
+ 0x010000af,
+ };
+ unsigned int i;
+
+ if ( boot_cpu_data.x86 != 0x10 )
+ return false;
+
+ for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
+ if ( sig->rev == final_levels[i] )
+ return true;
+
+ return false;
+}
+
static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
unsigned int current_cpu_id,
unsigned int *equiv_cpu_id)
@@ -229,6 +259,14 @@ static int apply_microcode(const struct microcode_patch *patch)
if ( !match_cpu(patch) )
return -EINVAL;
+ if ( check_final_patch_levels(sig) )
+ {
+ printk(XENLOG_ERR
+ "microcode: CPU%u current rev %#x unsafe to update\n",
+ cpu, sig->rev);
+ return -ENXIO;
+ }
+
hdr = patch->mpb;
hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
@@ -374,43 +412,6 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
return 0;
}
-/*
- * The 'final_levels' of patch ids have been obtained empirically.
- * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
- * for details of the issue. The short version is that people
- * using certain Fam10h systems noticed system hang issues when
- * trying to update microcode levels beyond the patch IDs below.
- * From internal discussions, we gathered that OS/hypervisor
- * cannot reliably perform microcode updates beyond these levels
- * due to hardware issues. Therefore, we need to abort microcode
- * update process if we hit any of these levels.
- */
-static const unsigned int final_levels[] = {
- 0x01000098,
- 0x0100009f,
- 0x010000af
-};
-
-static bool_t check_final_patch_levels(unsigned int cpu)
-{
- /*
- * Check the current patch levels on the cpu. If they are equal to
- * any of the 'final_levels', then we should not update the microcode
- * patch on the cpu as system will hang otherwise.
- */
- const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- unsigned int i;
-
- if ( boot_cpu_data.x86 != 0x10 )
- return 0;
-
- for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
- if ( sig->rev == final_levels[i] )
- return 1;
-
- return 0;
-}
-
static struct microcode_patch *cpu_request_microcode(const void *buf,
size_t bufsize)
{
@@ -434,14 +435,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
goto out;
}
- if ( check_final_patch_levels(cpu) )
- {
- printk(XENLOG_INFO
- "microcode: Cannot update microcode patch on the cpu as we hit a final level\n");
- error = -EPERM;
- goto out;
- }
-
mc_amd = xzalloc(struct microcode_amd);
if ( !mc_amd )
{
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
2020-03-31 10:05 ` [PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing Andrew Cooper
2020-03-31 10:05 ` [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:28 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info() Andrew Cooper
` (7 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
All code works fine with it having its correct type, and it even allows us to
drop two casts in a printk().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 4245dc13bb..3f3a05fad2 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -59,7 +59,7 @@ struct __packed microcode_header_amd {
#define UCODE_UCODE_TYPE 0x00000001
struct microcode_patch {
- void *mpb;
+ struct microcode_header_amd *mpb;
size_t mpb_size;
struct equiv_cpu_entry *equiv_cpu_table;
size_t equiv_cpu_table_size;
@@ -330,8 +330,7 @@ static int get_ucode_from_buffer_amd(
pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
smp_processor_id(), bufsize, mpbuf->len, *offset,
- ((struct microcode_header_amd *)mc_amd->mpb)->processor_rev_id,
- ((struct microcode_header_amd *)mc_amd->mpb)->patch_id);
+ mc_amd->mpb->processor_rev_id, mc_amd->mpb->patch_id);
*offset += mpbuf->len + SECTION_HDR_SIZE;
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (2 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:29 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely Andrew Cooper
` (6 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
... rather than collecting it repeatedly in microcode_fits(). This brings the
behaviour in line with the Intel side.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 11 +++--------
xen/include/asm-x86/microcode.h | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 3f3a05fad2..d2ecc7ae87 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -79,6 +79,7 @@ static int collect_cpu_info(struct cpu_signature *csig)
{
memset(csig, 0, sizeof(*csig));
+ csig->sig = cpuid_eax(1);
rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n",
@@ -177,12 +178,9 @@ static enum microcode_match_result microcode_fits(
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
- unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
- current_cpu_id = cpuid_eax(0x00000001);
-
- if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) )
+ if ( !find_equiv_cpu_id(equiv_cpu_table, sig->sig, &equiv_cpu_id) )
return MIS_UCODE;
if ( (mc_header->processor_rev_id) != equiv_cpu_id )
@@ -419,13 +417,10 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
struct microcode_patch *patch = NULL;
size_t offset = 0, saved_size = 0;
int error = 0;
- unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- current_cpu_id = cpuid_eax(0x00000001);
-
if ( bufsize < 4 ||
*(const uint32_t *)buf != UCODE_MAGIC )
{
@@ -456,7 +451,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
break;
}
- if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
+ if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, sig->sig,
&equiv_cpu_id) )
break;
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 3a8e4e8221..cbbe28cb45 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -7,7 +7,7 @@
#include <public/xen.h>
struct cpu_signature {
- /* CPU signature (CPUID.1.EAX). Only written on Intel. */
+ /* CPU signature (CPUID.1.EAX). */
unsigned int sig;
/* Platform Flags. Only applicable to Intel. */
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (3 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:36 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd() Andrew Cooper
` (5 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
We currently copy the entire equivalency table, and the single correct
microcode. This is not safe to heterogeneous scenarios, and as Xen doesn't
support such situations to being with, can be used to simplify things further.
The CPUID.1.EAX => processor_rev_id mapping is fixed for an individual part.
We can cache the single appropriate entry on first discovery, and forgo
duplicating the entire table.
Alter install_equiv_cpu_table() to be scan_equiv_cpu_table() which is
responsible for checking the equivalency table and caching appropriate
details. It now has a check for finding a different mapping (which indicates
that one of the tables we've seen is definitely wrong).
A return value of -ESRCH is now used to signify "everything fine, but nothing
applicable for the current CPU", which is used to select the
container_fast_forward() path.
Drop the printk(), as each applicable error path in scan_equiv_cpu_table()
already prints diagnostics.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
Naming of 'equiv' subject to improvement.
An alternative would be to embed the full equivelancy table, but it is fairly
large, and would need adjusting every time a new model/stepping was released.
---
xen/arch/x86/cpu/microcode/amd.c | 112 ++++++++++++++++++++++-----------------
1 file changed, 64 insertions(+), 48 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d2ecc7ae87..d3439b0c6c 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -61,8 +61,6 @@ struct __packed microcode_header_amd {
struct microcode_patch {
struct microcode_header_amd *mpb;
size_t mpb_size;
- struct equiv_cpu_entry *equiv_cpu_table;
- size_t equiv_cpu_table_size;
};
/* Temporary, until the microcode_* structure are disentangled. */
@@ -74,6 +72,31 @@ struct mpbhdr {
uint8_t data[];
};
+/*
+ * Microcode updates for different CPUs are distinguished by their
+ * processor_rev_id in the header. This denotes the format of the internals
+ * of the microcode engine, and is fixed for an individual CPU.
+ *
+ * There is a mapping from the CPU signature (CPUID.1.EAX -
+ * family/model/stepping) to the "equivalent CPU identifier" which is
+ * similarly fixed. In some cases, multiple different CPU signatures map to
+ * the same equiv_id for processor lines which share identical microcode
+ * facilities.
+ *
+ * This mapping can't be calculated in the general case, but is provided in
+ * the microcode container, so the correct piece of microcode for the CPU can
+ * be identified. We cache it the first time we encounter the correct mapping
+ * for this system.
+ *
+ * Note: for now, we assume a fully homogeneous setup, meaning that there is
+ * exactly one equiv_id we need to worry about for microcode blob
+ * identification. This may need revisiting in due course.
+ */
+static struct {
+ uint32_t sig;
+ uint16_t id;
+} equiv __read_mostly;
+
/* See comment in start_update() for cases when this routine fails */
static int collect_cpu_info(struct cpu_signature *csig)
{
@@ -150,40 +173,15 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
return false;
}
-static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
- unsigned int current_cpu_id,
- unsigned int *equiv_cpu_id)
-{
- unsigned int i;
-
- if ( !equiv_cpu_table )
- return 0;
-
- for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ )
- {
- if ( current_cpu_id == equiv_cpu_table[i].installed_cpu )
- {
- *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
- return 1;
- }
- }
-
- return 0;
-}
-
static enum microcode_match_result microcode_fits(
const struct microcode_amd *mc_amd)
{
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
- const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
- unsigned int equiv_cpu_id;
- if ( !find_equiv_cpu_id(equiv_cpu_table, sig->sig, &equiv_cpu_id) )
- return MIS_UCODE;
-
- if ( (mc_header->processor_rev_id) != equiv_cpu_id )
+ if ( equiv.sig != sig->sig ||
+ equiv.id != mc_header->processor_rev_id )
return MIS_UCODE;
if ( !verify_patch_size(mc_amd->mpb_size) )
@@ -213,7 +211,6 @@ static void free_patch(struct microcode_patch *mc_amd)
{
if ( mc_amd )
{
- xfree(mc_amd->equiv_cpu_table);
xfree(mc_amd->mpb);
xfree(mc_amd);
}
@@ -335,14 +332,15 @@ static int get_ucode_from_buffer_amd(
return 0;
}
-static int install_equiv_cpu_table(
- struct microcode_amd *mc_amd,
+static int scan_equiv_cpu_table(
const void *data,
size_t size_left,
size_t *offset)
{
+ const struct cpu_signature *sig = &this_cpu(cpu_sig);
const struct mpbhdr *mpbuf;
const struct equiv_cpu_entry *eq;
+ unsigned int i, nr;
if ( size_left < (sizeof(*mpbuf) + 4) ||
(mpbuf = data + *offset + 4,
@@ -362,19 +360,45 @@ static int install_equiv_cpu_table(
if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
(eq = (const void *)mpbuf->data,
- eq[(mpbuf->len / sizeof(*eq)) - 1].installed_cpu) )
+ nr = mpbuf->len / sizeof(*eq),
+ eq[nr - 1].installed_cpu) )
{
printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table length\n");
return -EINVAL;
}
- mc_amd->equiv_cpu_table = xmemdup_bytes(mpbuf->data, mpbuf->len);
- if ( !mc_amd->equiv_cpu_table )
- return -ENOMEM;
+ /* Search the equiv_cpu_table for the current CPU. */
+ for ( i = 0; i < nr && eq[i].installed_cpu; ++i )
+ {
+ if ( eq[i].installed_cpu != sig->sig )
+ continue;
- mc_amd->equiv_cpu_table_size = mpbuf->len;
+ if ( !equiv.sig ) /* Cache details on first find. */
+ {
+ equiv.sig = sig->sig;
+ equiv.id = eq[i].equiv_cpu;
+ return 0;
+ }
- return 0;
+ if ( equiv.sig != sig->sig || equiv.id != eq[i].equiv_cpu )
+ {
+ /*
+ * This can only occur if two equiv tables have been seen with
+ * different mappings for the same CPU. The mapping is fixed, so
+ * one of the tables is wrong. As we can't calculate the mapping,
+ * we trusted the first table we saw.
+ */
+ printk(XENLOG_ERR
+ "microcode: Equiv mismatch: cpu %08x, got %04x, cached %04x\n",
+ sig->sig, eq[i].equiv_cpu, equiv.id);
+ return -EINVAL;
+ }
+
+ return 0;
+ }
+
+ /* equiv_cpu_table was fine, but nothing found for the current CPU. */
+ return -ESRCH;
}
static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
@@ -417,7 +441,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
struct microcode_patch *patch = NULL;
size_t offset = 0, saved_size = 0;
int error = 0;
- unsigned int equiv_cpu_id;
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
@@ -444,15 +467,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
*/
while ( offset < bufsize )
{
- error = install_equiv_cpu_table(mc_amd, buf, bufsize - offset, &offset);
- if ( error )
- {
- printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
- break;
- }
+ error = scan_equiv_cpu_table(buf, bufsize - offset, &offset);
- if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, sig->sig,
- &equiv_cpu_id) )
+ if ( !error || error != -ESRCH )
break;
error = container_fast_forward(buf, bufsize - offset, &offset);
@@ -479,7 +496,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
if ( error == -ENODATA )
error = 0;
- xfree(mc_amd->equiv_cpu_table);
xfree(mc_amd);
goto out;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (4 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:38 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits() Andrew Cooper
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
We only stash the microcode blob size so it can be audited in
microcode_fits(). However, the patch size check depends only on the CPU
family.
Move the check earlier to when we are parsing the container, which avoids
caching bad microcode in the first place, and allows us to avoid storing the
size at all.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d3439b0c6c..8318664f85 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -60,7 +60,6 @@ struct __packed microcode_header_amd {
struct microcode_patch {
struct microcode_header_amd *mpb;
- size_t mpb_size;
};
/* Temporary, until the microcode_* structure are disentangled. */
@@ -184,12 +183,6 @@ static enum microcode_match_result microcode_fits(
equiv.id != mc_header->processor_rev_id )
return MIS_UCODE;
- if ( !verify_patch_size(mc_amd->mpb_size) )
- {
- pr_debug("microcode: patch size mismatch\n");
- return MIS_UCODE;
- }
-
if ( mc_header->patch_id <= sig->rev )
{
pr_debug("microcode: patch is already at required level or greater.\n");
@@ -318,10 +311,15 @@ static int get_ucode_from_buffer_amd(
return -EINVAL;
}
+ if ( !verify_patch_size(mpbuf->len) )
+ {
+ printk(XENLOG_ERR "microcode: patch size mismatch\n");
+ return -EINVAL;
+ }
+
mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len);
if ( !mc_amd->mpb )
return -ENOMEM;
- mc_amd->mpb_size = mpbuf->len;
pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
smp_processor_id(), bufsize, mpbuf->len, *offset,
@@ -439,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
struct microcode_amd *mc_amd;
struct microcode_header_amd *saved = NULL;
struct microcode_patch *patch = NULL;
- size_t offset = 0, saved_size = 0;
+ size_t offset = 0;
int error = 0;
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
@@ -516,7 +514,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
{
xfree(saved);
saved = mc_amd->mpb;
- saved_size = mc_amd->mpb_size;
}
else
{
@@ -555,7 +552,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
if ( saved )
{
mc_amd->mpb = saved;
- mc_amd->mpb_size = saved_size;
patch = mc_amd;
}
else
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (5 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:39 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode() Andrew Cooper
` (3 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
Although it is logically a step in the wrong direction overall, it simplifies
the rearranging of cpu_request_microcode() substantially for microcode_fits()
to take struct microcode_header_amd directly, and not require an intermediate
struct microcode_amd pointing at it.
Make this change (taking time to rename 'mc_amd' to its eventual 'patch' to
reduce the churn in the series), and a later cleanup will make it uniformly
take a struct microcode_patch.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 8318664f85..254f3dd4d7 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -173,31 +173,30 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
}
static enum microcode_match_result microcode_fits(
- const struct microcode_amd *mc_amd)
+ const struct microcode_header_amd *patch)
{
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- const struct microcode_header_amd *mc_header = mc_amd->mpb;
if ( equiv.sig != sig->sig ||
- equiv.id != mc_header->processor_rev_id )
+ equiv.id != patch->processor_rev_id )
return MIS_UCODE;
- if ( mc_header->patch_id <= sig->rev )
+ if ( patch->patch_id <= sig->rev )
{
pr_debug("microcode: patch is already at required level or greater.\n");
return OLD_UCODE;
}
pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n",
- cpu, mc_header->patch_id, sig->rev);
+ cpu, patch->patch_id, sig->rev);
return NEW_UCODE;
}
static bool match_cpu(const struct microcode_patch *patch)
{
- return patch && (microcode_fits(patch) == NEW_UCODE);
+ return patch && (microcode_fits(patch->mpb) == NEW_UCODE);
}
static void free_patch(struct microcode_patch *mc_amd)
@@ -223,14 +222,11 @@ static enum microcode_match_result compare_header(
static enum microcode_match_result compare_patch(
const struct microcode_patch *new, const struct microcode_patch *old)
{
- const struct microcode_header_amd *new_header = new->mpb;
- const struct microcode_header_amd *old_header = old->mpb;
-
/* Both patches to compare are supposed to be applicable to local CPU. */
- ASSERT(microcode_fits(new) != MIS_UCODE);
- ASSERT(microcode_fits(old) != MIS_UCODE);
+ ASSERT(microcode_fits(new->mpb) != MIS_UCODE);
+ ASSERT(microcode_fits(old->mpb) != MIS_UCODE);
- return compare_header(new_header, old_header);
+ return compare_header(new->mpb, old->mpb);
}
static int apply_microcode(const struct microcode_patch *patch)
@@ -509,7 +505,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
* If the new ucode covers current CPU, compare ucodes and store the
* one with higher revision.
*/
- if ( (microcode_fits(mc_amd) != MIS_UCODE) &&
+ if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
(!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
{
xfree(saved);
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (6 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:41 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode() Andrew Cooper
` (2 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
To simplify future cleanup, rename this variable.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 254f3dd4d7..980e61c547 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -427,8 +427,7 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
return 0;
}
-static struct microcode_patch *cpu_request_microcode(const void *buf,
- size_t bufsize)
+static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
{
struct microcode_amd *mc_amd;
struct microcode_header_amd *saved = NULL;
@@ -438,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- if ( bufsize < 4 ||
+ if ( size < 4 ||
*(const uint32_t *)buf != UCODE_MAGIC )
{
printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
@@ -459,17 +458,17 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
* 1. check if this container file has equiv_cpu_id match
* 2. If not, fast-fwd to next container file
*/
- while ( offset < bufsize )
+ while ( offset < size )
{
- error = scan_equiv_cpu_table(buf, bufsize - offset, &offset);
+ error = scan_equiv_cpu_table(buf, size - offset, &offset);
if ( !error || error != -ESRCH )
break;
- error = container_fast_forward(buf, bufsize - offset, &offset);
+ error = container_fast_forward(buf, size - offset, &offset);
if ( error == -ENODATA )
{
- ASSERT(offset == bufsize);
+ ASSERT(offset == size);
break;
}
if ( error )
@@ -498,7 +497,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
* It's possible the data file has multiple matching ucode,
* lets keep searching till the latest version
*/
- while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
+ while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
&offset)) == 0 )
{
/*
@@ -517,7 +516,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
mc_amd->mpb = NULL;
}
- if ( offset >= bufsize )
+ if ( offset >= size )
break;
/*
@@ -527,7 +526,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
* earlier while() (On this case, matches on earlier container
* file and we break)
* 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
- * buf, bufsize,&offset)) == 0 )
+ * buf, size, &offset)) == 0 )
* 4. Find correct patch using microcode_fits() and apply the patch
* (Assume: apply_microcode() is successful)
* 5. The while() loop from (3) continues to parse the binary as
@@ -540,7 +539,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
* before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
* false and returns -EINVAL.
*/
- if ( offset + SECTION_HDR_SIZE <= bufsize &&
+ if ( offset + SECTION_HDR_SIZE <= size &&
*(const uint32_t *)(buf + offset) == UCODE_MAGIC )
break;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (7 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:51 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 10/11] x86/ucode/amd: Fold structures together Andrew Cooper
2020-03-31 10:05 ` [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode() Andrew Cooper
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
Just as on the Intel side, there is no point having
get_ucode_from_buffer_amd() make $N memory allocations and free $N-1 of them.
Delete get_ucode_from_buffer_amd() and rewrite the loop in
cpu_request_microcode() to have 'saved' point into 'buf' until we finally
decide to duplicate that blob and return it to our caller.
Introduce a new struct container_microcode to simplify interpreting the
container format. Doubly indent the logic to substantially reduce the churn
in a later change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 138 +++++++++++++--------------------------
1 file changed, 47 insertions(+), 91 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 980e61c547..ae1276988f 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -70,6 +70,11 @@ struct mpbhdr {
uint32_t len;
uint8_t data[];
};
+struct container_microcode {
+ uint32_t type; /* UCODE_UCODE_TYPE */
+ uint32_t len;
+ struct microcode_header_amd patch[];
+};
/*
* Microcode updates for different CPUs are distinguished by their
@@ -280,52 +285,6 @@ static int apply_microcode(const struct microcode_patch *patch)
return 0;
}
-static int get_ucode_from_buffer_amd(
- struct microcode_amd *mc_amd,
- const void *buf,
- size_t bufsize,
- size_t *offset)
-{
- const struct mpbhdr *mpbuf = buf + *offset;
-
- /* No more data */
- if ( *offset >= bufsize )
- {
- printk(KERN_ERR "microcode: Microcode buffer overrun\n");
- return -EINVAL;
- }
-
- if ( mpbuf->type != UCODE_UCODE_TYPE )
- {
- printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
- return -EINVAL;
- }
-
- if ( (*offset + mpbuf->len) > bufsize )
- {
- printk(KERN_ERR "microcode: Bad data in microcode data file\n");
- return -EINVAL;
- }
-
- if ( !verify_patch_size(mpbuf->len) )
- {
- printk(XENLOG_ERR "microcode: patch size mismatch\n");
- return -EINVAL;
- }
-
- mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len);
- if ( !mc_amd->mpb )
- return -ENOMEM;
-
- pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
- smp_processor_id(), bufsize, mpbuf->len, *offset,
- mc_amd->mpb->processor_rev_id, mc_amd->mpb->patch_id);
-
- *offset += mpbuf->len + SECTION_HDR_SIZE;
-
- return 0;
-}
-
static int scan_equiv_cpu_table(
const void *data,
size_t size_left,
@@ -430,9 +389,9 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
{
struct microcode_amd *mc_amd;
- struct microcode_header_amd *saved = NULL;
+ const struct microcode_header_amd *saved = NULL;
struct microcode_patch *patch = NULL;
- size_t offset = 0;
+ size_t offset = 0, saved_size = 0;
int error = 0;
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
@@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
* It's possible the data file has multiple matching ucode,
* lets keep searching till the latest version
*/
- while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
- &offset)) == 0 )
+ buf += offset;
+ size -= offset;
{
- /*
- * If the new ucode covers current CPU, compare ucodes and store the
- * one with higher revision.
- */
- if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
- (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
+ while ( size )
{
- xfree(saved);
- saved = mc_amd->mpb;
- }
- else
- {
- xfree(mc_amd->mpb);
- mc_amd->mpb = NULL;
- }
+ const struct container_microcode *mc;
+
+ if ( size < sizeof(*mc) ||
+ (mc = buf)->type != UCODE_UCODE_TYPE ||
+ size - sizeof(*mc) < mc->len ||
+ !verify_patch_size(mc->len) )
+ {
+ printk(XENLOG_ERR "microcode: Bad microcode data\n");
+ error = -EINVAL;
+ break;
+ }
- if ( offset >= size )
- break;
+ /*
+ * If the new ucode covers current CPU, compare ucodes and store the
+ * one with higher revision.
+ */
+ if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
+ (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
+ {
+ saved = mc->patch;
+ saved_size = mc->len;
+ }
- /*
- * 1. Given a situation where multiple containers exist and correct
- * patch lives on a container that is not the last container.
- * 2. We match equivalent ids using find_equiv_cpu_id() from the
- * earlier while() (On this case, matches on earlier container
- * file and we break)
- * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
- * buf, size, &offset)) == 0 )
- * 4. Find correct patch using microcode_fits() and apply the patch
- * (Assume: apply_microcode() is successful)
- * 5. The while() loop from (3) continues to parse the binary as
- * there is a subsequent container file, but...
- * 6. ...a correct patch can only be on one container and not on any
- * subsequent ones. (Refer docs for more info) Therefore, we
- * don't have to parse a subsequent container. So, we can abort
- * the process here.
- * 7. This ensures that we retain a success value (= 0) to 'error'
- * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
- * false and returns -EINVAL.
- */
- if ( offset + SECTION_HDR_SIZE <= size &&
- *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
- break;
+ /* Move over the microcode blob. */
+ buf += sizeof(*mc) + mc->len;
+ size -= sizeof(*mc) + mc->len;
+
+ /*
+ * Peek ahead. If we see the start of another container, we've
+ * exhaused all microcode blobs in this container. Exit cleanly.
+ */
+ if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
+ break;
+ }
}
if ( saved )
{
- mc_amd->mpb = saved;
- patch = mc_amd;
+ mc_amd->mpb = xmemdup_bytes(saved, saved_size);
+ if ( mc_amd->mpb )
+ patch = mc_amd;
+ else
+ error = -ENOMEM;
}
else
free_patch(mc_amd);
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 10/11] x86/ucode/amd: Fold structures together
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (8 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode() Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 14:53 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode() Andrew Cooper
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
With all the necessary cleanup now in place, fold struct microcode_header_amd
into struct microcode_patch and drop the struct microcode_amd temporary
ifdef-ary.
This removes the memory allocation of struct microcode_amd which is a single
pointer to a separately allocated object, and therefore a waste.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 70 ++++++++++++----------------------------
1 file changed, 20 insertions(+), 50 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index ae1276988f..f9c50b43bf 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -37,7 +37,7 @@ struct __packed equiv_cpu_entry {
uint16_t reserved;
};
-struct __packed microcode_header_amd {
+struct microcode_patch {
uint32_t data_code;
uint32_t patch_id;
uint8_t mc_patch_data_id[2];
@@ -58,13 +58,6 @@ struct __packed microcode_header_amd {
#define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
#define UCODE_UCODE_TYPE 0x00000001
-struct microcode_patch {
- struct microcode_header_amd *mpb;
-};
-
-/* Temporary, until the microcode_* structure are disentangled. */
-#define microcode_amd microcode_patch
-
struct mpbhdr {
uint32_t type;
uint32_t len;
@@ -73,7 +66,7 @@ struct mpbhdr {
struct container_microcode {
uint32_t type; /* UCODE_UCODE_TYPE */
uint32_t len;
- struct microcode_header_amd patch[];
+ struct microcode_patch patch[];
};
/*
@@ -178,7 +171,7 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
}
static enum microcode_match_result microcode_fits(
- const struct microcode_header_amd *patch)
+ const struct microcode_patch *patch)
{
unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
@@ -201,37 +194,31 @@ static enum microcode_match_result microcode_fits(
static bool match_cpu(const struct microcode_patch *patch)
{
- return patch && (microcode_fits(patch->mpb) == NEW_UCODE);
+ return patch && (microcode_fits(patch) == NEW_UCODE);
}
-static void free_patch(struct microcode_patch *mc_amd)
+static void free_patch(struct microcode_patch *patch)
{
- if ( mc_amd )
- {
- xfree(mc_amd->mpb);
- xfree(mc_amd);
- }
+ xfree(patch);
}
static enum microcode_match_result compare_header(
- const struct microcode_header_amd *new_header,
- const struct microcode_header_amd *old_header)
+ const struct microcode_patch *new, const struct microcode_patch *old)
{
- if ( new_header->processor_rev_id == old_header->processor_rev_id )
- return (new_header->patch_id > old_header->patch_id) ? NEW_UCODE
- : OLD_UCODE;
+ if ( new->processor_rev_id != old->processor_rev_id )
+ return MIS_UCODE;
- return MIS_UCODE;
+ return new->patch_id > old->patch_id ? NEW_UCODE : OLD_UCODE;
}
static enum microcode_match_result compare_patch(
const struct microcode_patch *new, const struct microcode_patch *old)
{
/* Both patches to compare are supposed to be applicable to local CPU. */
- ASSERT(microcode_fits(new->mpb) != MIS_UCODE);
- ASSERT(microcode_fits(old->mpb) != MIS_UCODE);
+ ASSERT(microcode_fits(new) != MIS_UCODE);
+ ASSERT(microcode_fits(old) != MIS_UCODE);
- return compare_header(new->mpb, old->mpb);
+ return compare_header(new, old);
}
static int apply_microcode(const struct microcode_patch *patch)
@@ -239,7 +226,6 @@ static int apply_microcode(const struct microcode_patch *patch)
int hw_err;
unsigned int cpu = smp_processor_id();
struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- const struct microcode_header_amd *hdr;
uint32_t rev, old_rev = sig->rev;
if ( !patch )
@@ -256,9 +242,7 @@ static int apply_microcode(const struct microcode_patch *patch)
return -ENXIO;
}
- hdr = patch->mpb;
-
- hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
+ hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)patch);
/* get patch id after patching */
rdmsrl(MSR_AMD_PATCHLEVEL, rev);
@@ -268,14 +252,14 @@ static int apply_microcode(const struct microcode_patch *patch)
* Some processors leave the ucode blob mapping as UC after the update.
* Flush the mapping to regain normal cacheability.
*/
- flush_area_local(hdr, FLUSH_TLB_GLOBAL | FLUSH_ORDER(0));
+ flush_area_local(patch, FLUSH_TLB_GLOBAL | FLUSH_ORDER(0));
/* check current patch id and patch's id for match */
- if ( hw_err || (rev != hdr->patch_id) )
+ if ( hw_err || (rev != patch->patch_id) )
{
printk(XENLOG_ERR
"microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
- cpu, old_rev, hdr->patch_id, rev);
+ cpu, old_rev, patch->patch_id, rev);
return -EIO;
}
@@ -388,8 +372,7 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
{
- struct microcode_amd *mc_amd;
- const struct microcode_header_amd *saved = NULL;
+ const struct microcode_patch *saved = NULL;
struct microcode_patch *patch = NULL;
size_t offset = 0, saved_size = 0;
int error = 0;
@@ -404,14 +387,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
goto out;
}
- mc_amd = xzalloc(struct microcode_amd);
- if ( !mc_amd )
- {
- printk(KERN_ERR "microcode: Cannot allocate memory for microcode patch\n");
- error = -ENOMEM;
- goto out;
- }
-
/*
* Multiple container file support:
* 1. check if this container file has equiv_cpu_id match
@@ -448,7 +423,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
if ( error == -ENODATA )
error = 0;
- xfree(mc_amd);
goto out;
}
@@ -499,14 +473,10 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
if ( saved )
{
- mc_amd->mpb = xmemdup_bytes(saved, saved_size);
- if ( mc_amd->mpb )
- patch = mc_amd;
- else
+ patch = xmemdup_bytes(saved, saved_size);
+ if ( !patch )
error = -ENOMEM;
}
- else
- free_patch(mc_amd);
out:
if ( error && !patch )
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
` (9 preceding siblings ...)
2020-03-31 10:05 ` [PATCH 10/11] x86/ucode/amd: Fold structures together Andrew Cooper
@ 2020-03-31 10:05 ` Andrew Cooper
2020-03-31 15:07 ` Jan Beulich
10 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 10:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
cpu_request_microcode() is still a confusing mess to follow, with sub
functions responsible for maintaining offset. Rewrite it so all container
structure handling is in this one function.
Rewrite struct mpbhdr as struct container_equiv_table to aid parsing. Drop
container_fast_forward() entirely, and shrink scan_equiv_cpu_table() to just
its searching/caching logic.
container_fast_forward() gets logically folded into the microcode blob
scanning loop, except that a skip path is inserted, which is conditional on
whether scan_equiv_cpu_table() thinks there is appropriate microcode to find.
With this change, we now scan to the end of all provided microcode containers,
and no longer give up at the first applicable one.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/cpu/microcode/amd.c | 169 ++++++++++-----------------------------
1 file changed, 44 insertions(+), 125 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index f9c50b43bf..0ada50797b 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -25,10 +25,6 @@
#define pr_debug(x...) ((void)0)
-#define CONT_HDR_SIZE 12
-#define SECTION_HDR_SIZE 8
-#define PATCH_HDR_SIZE 32
-
struct __packed equiv_cpu_entry {
uint32_t installed_cpu;
uint32_t fixed_errata_mask;
@@ -58,10 +54,10 @@ struct microcode_patch {
#define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
#define UCODE_UCODE_TYPE 0x00000001
-struct mpbhdr {
- uint32_t type;
+struct container_equiv_table {
+ uint32_t type; /* UCODE_EQUIV_CPU_TABLE_TYPE */
uint32_t len;
- uint8_t data[];
+ struct equiv_cpu_entry eq[];
};
struct container_microcode {
uint32_t type; /* UCODE_UCODE_TYPE */
@@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
return 0;
}
-static int scan_equiv_cpu_table(
- const void *data,
- size_t size_left,
- size_t *offset)
+static int scan_equiv_cpu_table(const struct container_equiv_table *et)
{
const struct cpu_signature *sig = &this_cpu(cpu_sig);
- const struct mpbhdr *mpbuf;
- const struct equiv_cpu_entry *eq;
- unsigned int i, nr;
-
- if ( size_left < (sizeof(*mpbuf) + 4) ||
- (mpbuf = data + *offset + 4,
- size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
- {
- printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
- return -EINVAL;
- }
-
- *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
-
- if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
- {
- printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
- return -EINVAL;
- }
-
- if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
- (eq = (const void *)mpbuf->data,
- nr = mpbuf->len / sizeof(*eq),
- eq[nr - 1].installed_cpu) )
- {
- printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table length\n");
- return -EINVAL;
- }
+ unsigned int i, nr = et->len / sizeof(et->eq[0]);
/* Search the equiv_cpu_table for the current CPU. */
- for ( i = 0; i < nr && eq[i].installed_cpu; ++i )
+ for ( i = 0; i < nr && et->eq[i].installed_cpu; ++i )
{
- if ( eq[i].installed_cpu != sig->sig )
+ if ( et->eq[i].installed_cpu != sig->sig )
continue;
if ( !equiv.sig ) /* Cache details on first find. */
{
equiv.sig = sig->sig;
- equiv.id = eq[i].equiv_cpu;
+ equiv.id = et->eq[i].equiv_cpu;
return 0;
}
- if ( equiv.sig != sig->sig || equiv.id != eq[i].equiv_cpu )
+ if ( equiv.sig != sig->sig || equiv.id != et->eq[i].equiv_cpu )
{
/*
* This can only occur if two equiv tables have been seen with
@@ -327,7 +293,7 @@ static int scan_equiv_cpu_table(
*/
printk(XENLOG_ERR
"microcode: Equiv mismatch: cpu %08x, got %04x, cached %04x\n",
- sig->sig, eq[i].equiv_cpu, equiv.id);
+ sig->sig, et->eq[i].equiv_cpu, equiv.id);
return -EINVAL;
}
@@ -338,101 +304,51 @@ static int scan_equiv_cpu_table(
return -ESRCH;
}
-static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
-{
- for ( ; ; )
- {
- size_t size;
- const uint32_t *header;
-
- if ( size_left < SECTION_HDR_SIZE )
- return -EINVAL;
-
- header = data + *offset;
-
- if ( header[0] == UCODE_MAGIC &&
- header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
- break;
-
- if ( header[0] != UCODE_UCODE_TYPE )
- return -EINVAL;
- size = header[1] + SECTION_HDR_SIZE;
- if ( size < PATCH_HDR_SIZE || size_left < size )
- return -EINVAL;
-
- size_left -= size;
- *offset += size;
-
- if ( !size_left )
- return -ENODATA;
- }
-
- return 0;
-}
-
static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
{
const struct microcode_patch *saved = NULL;
struct microcode_patch *patch = NULL;
- size_t offset = 0, saved_size = 0;
+ size_t saved_size = 0;
int error = 0;
- unsigned int cpu = smp_processor_id();
- const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- if ( size < 4 ||
- *(const uint32_t *)buf != UCODE_MAGIC )
+ while ( size )
{
- printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
- error = -EINVAL;
- goto out;
- }
-
- /*
- * Multiple container file support:
- * 1. check if this container file has equiv_cpu_id match
- * 2. If not, fast-fwd to next container file
- */
- while ( offset < size )
- {
- error = scan_equiv_cpu_table(buf, size - offset, &offset);
-
- if ( !error || error != -ESRCH )
- break;
+ const struct container_equiv_table *et;
+ bool skip_ucode;
- error = container_fast_forward(buf, size - offset, &offset);
- if ( error == -ENODATA )
+ if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
{
- ASSERT(offset == size);
+ printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
+ error = -EINVAL;
break;
}
- if ( error )
+
+ /* Move over UCODE_MAGIC. */
+ buf += 4;
+ size -= 4;
+
+ if ( size < sizeof(*et) ||
+ (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
+ size - sizeof(*et) < et->len ||
+ et->len % sizeof(et->eq[0]) )
{
- printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
- "microcode: Failed to update patch level. "
- "Current lvl:%#x\n", cpu, sig->rev);
+ printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
+ error = -EINVAL;
break;
}
- }
- if ( error )
- {
- /*
- * -ENODATA here means that the blob was parsed fine but no matching
- * ucode was found. Don't return it to the caller.
- */
- if ( error == -ENODATA )
- error = 0;
-
- goto out;
- }
+ /* Move over the Equiv table. */
+ buf += sizeof(*et) + et->len;
+ size -= sizeof(*et) + et->len;
+
+ error = scan_equiv_cpu_table(et);
+ if ( error && error != -ESRCH )
+ break;
+
+ /* -ESRCH means no applicable microcode in this container. */
+ skip_ucode = error == -ESRCH;
+ error = 0;
- /*
- * It's possible the data file has multiple matching ucode,
- * lets keep searching till the latest version
- */
- buf += offset;
- size -= offset;
- {
while ( size )
{
const struct container_microcode *mc;
@@ -440,13 +356,16 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
if ( size < sizeof(*mc) ||
(mc = buf)->type != UCODE_UCODE_TYPE ||
size - sizeof(*mc) < mc->len ||
- !verify_patch_size(mc->len) )
+ (!skip_ucode && !verify_patch_size(mc->len)) )
{
printk(XENLOG_ERR "microcode: Bad microcode data\n");
error = -EINVAL;
break;
}
+ if ( skip_ucode )
+ goto skip;
+
/*
* If the new ucode covers current CPU, compare ucodes and store the
* one with higher revision.
@@ -459,6 +378,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
}
/* Move over the microcode blob. */
+ skip:
buf += sizeof(*mc) + mc->len;
size -= sizeof(*mc) + mc->len;
@@ -478,7 +398,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
error = -ENOMEM;
}
- out:
if ( error && !patch )
patch = ERR_PTR(error);
--
2.11.0
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode()
2020-03-31 10:05 ` [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode() Andrew Cooper
@ 2020-03-31 14:27 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> The microcode revision of whichever CPU runs cpu_request_microcode() is not
> necessarily applicable to other CPUs.
>
> If the BIOS left us with asymmetric microcode, rejecting updates in
> cpu_request_microcode() would prevent us levelling the system even if only up
> to the final level. Also, failing to cache microcode misses an opportunity to
> get beyond the final level via the S3 path.
>
> Move check_final_patch_levels() earlier and use it in apply_microcode().
> Reword the error message to be more informative, and use -ENXIO as this corner
> case has nothing to do with permissions.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb
2020-03-31 10:05 ` [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb Andrew Cooper
@ 2020-03-31 14:28 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> All code works fine with it having its correct type, and it even allows us to
> drop two casts in a printk().
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info()
2020-03-31 10:05 ` [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info() Andrew Cooper
@ 2020-03-31 14:29 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:29 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> ... rather than collecting it repeatedly in microcode_fits(). This brings the
> behaviour in line with the Intel side.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely
2020-03-31 10:05 ` [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely Andrew Cooper
@ 2020-03-31 14:36 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:36 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> We currently copy the entire equivalency table, and the single correct
> microcode. This is not safe to heterogeneous scenarios, and as Xen doesn't
> support such situations to being with, can be used to simplify things further.
s/being/begin/ ?
> The CPUID.1.EAX => processor_rev_id mapping is fixed for an individual part.
> We can cache the single appropriate entry on first discovery, and forgo
> duplicating the entire table.
>
> Alter install_equiv_cpu_table() to be scan_equiv_cpu_table() which is
> responsible for checking the equivalency table and caching appropriate
> details. It now has a check for finding a different mapping (which indicates
> that one of the tables we've seen is definitely wrong).
>
> A return value of -ESRCH is now used to signify "everything fine, but nothing
> applicable for the current CPU", which is used to select the
> container_fast_forward() path.
>
> Drop the printk(), as each applicable error path in scan_equiv_cpu_table()
> already prints diagnostics.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd()
2020-03-31 10:05 ` [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd() Andrew Cooper
@ 2020-03-31 14:38 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> We only stash the microcode blob size so it can be audited in
> microcode_fits(). However, the patch size check depends only on the CPU
> family.
>
> Move the check earlier to when we are parsing the container, which avoids
> caching bad microcode in the first place, and allows us to avoid storing the
> size at all.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits()
2020-03-31 10:05 ` [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits() Andrew Cooper
@ 2020-03-31 14:39 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> Although it is logically a step in the wrong direction overall, it simplifies
> the rearranging of cpu_request_microcode() substantially for microcode_fits()
> to take struct microcode_header_amd directly, and not require an intermediate
> struct microcode_amd pointing at it.
>
> Make this change (taking time to rename 'mc_amd' to its eventual 'patch' to
> reduce the churn in the series), and a later cleanup will make it uniformly
> take a struct microcode_patch.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
2020-03-31 10:05 ` [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode() Andrew Cooper
@ 2020-03-31 14:41 ` Jan Beulich
2020-03-31 14:43 ` Andrew Cooper
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> To simplify future cleanup, rename this variable.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> @@ -438,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
> unsigned int cpu = smp_processor_id();
> const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>
> - if ( bufsize < 4 ||
> + if ( size < 4 ||
> *(const uint32_t *)buf != UCODE_MAGIC )
Take the opportunity and put this on a single line?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
2020-03-31 14:41 ` Jan Beulich
@ 2020-03-31 14:43 ` Andrew Cooper
0 siblings, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 14:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31/03/2020 15:41, Jan Beulich wrote:
> On 31.03.2020 12:05, Andrew Cooper wrote:
>> To simplify future cleanup, rename this variable.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>> @@ -438,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
>> unsigned int cpu = smp_processor_id();
>> const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>
>> - if ( bufsize < 4 ||
>> + if ( size < 4 ||
>> *(const uint32_t *)buf != UCODE_MAGIC )
> Take the opportunity and put this on a single line?
I have actually done that in a later patch.
This was introduced in patch 1, so I'll fix there and rebase it
throughout the series.
~Andrew
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 10:05 ` [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode() Andrew Cooper
@ 2020-03-31 14:51 ` Jan Beulich
2020-03-31 14:55 ` Andrew Cooper
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:51 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
> * It's possible the data file has multiple matching ucode,
> * lets keep searching till the latest version
> */
> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
> - &offset)) == 0 )
> + buf += offset;
> + size -= offset;
> {
> - /*
> - * If the new ucode covers current CPU, compare ucodes and store the
> - * one with higher revision.
> - */
> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
> + while ( size )
> {
> - xfree(saved);
> - saved = mc_amd->mpb;
> - }
> - else
> - {
> - xfree(mc_amd->mpb);
> - mc_amd->mpb = NULL;
> - }
> + const struct container_microcode *mc;
> +
> + if ( size < sizeof(*mc) ||
> + (mc = buf)->type != UCODE_UCODE_TYPE ||
> + size - sizeof(*mc) < mc->len ||
> + !verify_patch_size(mc->len) )
> + {
> + printk(XENLOG_ERR "microcode: Bad microcode data\n");
> + error = -EINVAL;
> + break;
> + }
>
> - if ( offset >= size )
> - break;
> + /*
> + * If the new ucode covers current CPU, compare ucodes and store the
> + * one with higher revision.
> + */
> + if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
> + {
> + saved = mc->patch;
> + saved_size = mc->len;
> + }
>
> - /*
> - * 1. Given a situation where multiple containers exist and correct
> - * patch lives on a container that is not the last container.
> - * 2. We match equivalent ids using find_equiv_cpu_id() from the
> - * earlier while() (On this case, matches on earlier container
> - * file and we break)
> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
> - * buf, size, &offset)) == 0 )
> - * 4. Find correct patch using microcode_fits() and apply the patch
> - * (Assume: apply_microcode() is successful)
> - * 5. The while() loop from (3) continues to parse the binary as
> - * there is a subsequent container file, but...
> - * 6. ...a correct patch can only be on one container and not on any
> - * subsequent ones. (Refer docs for more info) Therefore, we
> - * don't have to parse a subsequent container. So, we can abort
> - * the process here.
> - * 7. This ensures that we retain a success value (= 0) to 'error'
> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
> - * false and returns -EINVAL.
> - */
> - if ( offset + SECTION_HDR_SIZE <= size &&
> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
> - break;
> + /* Move over the microcode blob. */
> + buf += sizeof(*mc) + mc->len;
> + size -= sizeof(*mc) + mc->len;
> +
> + /*
> + * Peek ahead. If we see the start of another container, we've
> + * exhaused all microcode blobs in this container. Exit cleanly.
> + */
> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
> + break;
While, as already indicated, I agree with shrinking the big comment,
I think point 6 is what wants retaining in some form - it's not
obvious at all why a subsequent container couldn't contain a higher
rev ucode than what we've found. That comment refers us to docs, but
I couldn't find anything to this effect in PM Vol 2. Assuming this
indeed documented and true, with the comment extended accordingly
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 10/11] x86/ucode/amd: Fold structures together
2020-03-31 10:05 ` [PATCH 10/11] x86/ucode/amd: Fold structures together Andrew Cooper
@ 2020-03-31 14:53 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 14:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> With all the necessary cleanup now in place, fold struct microcode_header_amd
> into struct microcode_patch and drop the struct microcode_amd temporary
> ifdef-ary.
>
> This removes the memory allocation of struct microcode_amd which is a single
> pointer to a separately allocated object, and therefore a waste.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 14:51 ` Jan Beulich
@ 2020-03-31 14:55 ` Andrew Cooper
2020-03-31 15:13 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 14:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31/03/2020 15:51, Jan Beulich wrote:
> On 31.03.2020 12:05, Andrew Cooper wrote:
>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>> * It's possible the data file has multiple matching ucode,
>> * lets keep searching till the latest version
>> */
>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
>> - &offset)) == 0 )
>> + buf += offset;
>> + size -= offset;
>> {
>> - /*
>> - * If the new ucode covers current CPU, compare ucodes and store the
>> - * one with higher revision.
>> - */
>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
>> + while ( size )
>> {
>> - xfree(saved);
>> - saved = mc_amd->mpb;
>> - }
>> - else
>> - {
>> - xfree(mc_amd->mpb);
>> - mc_amd->mpb = NULL;
>> - }
>> + const struct container_microcode *mc;
>> +
>> + if ( size < sizeof(*mc) ||
>> + (mc = buf)->type != UCODE_UCODE_TYPE ||
>> + size - sizeof(*mc) < mc->len ||
>> + !verify_patch_size(mc->len) )
>> + {
>> + printk(XENLOG_ERR "microcode: Bad microcode data\n");
>> + error = -EINVAL;
>> + break;
>> + }
>>
>> - if ( offset >= size )
>> - break;
>> + /*
>> + * If the new ucode covers current CPU, compare ucodes and store the
>> + * one with higher revision.
>> + */
>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
>> + {
>> + saved = mc->patch;
>> + saved_size = mc->len;
>> + }
>>
>> - /*
>> - * 1. Given a situation where multiple containers exist and correct
>> - * patch lives on a container that is not the last container.
>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the
>> - * earlier while() (On this case, matches on earlier container
>> - * file and we break)
>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>> - * buf, size, &offset)) == 0 )
>> - * 4. Find correct patch using microcode_fits() and apply the patch
>> - * (Assume: apply_microcode() is successful)
>> - * 5. The while() loop from (3) continues to parse the binary as
>> - * there is a subsequent container file, but...
>> - * 6. ...a correct patch can only be on one container and not on any
>> - * subsequent ones. (Refer docs for more info) Therefore, we
>> - * don't have to parse a subsequent container. So, we can abort
>> - * the process here.
>> - * 7. This ensures that we retain a success value (= 0) to 'error'
>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
>> - * false and returns -EINVAL.
>> - */
>> - if ( offset + SECTION_HDR_SIZE <= size &&
>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>> - break;
>> + /* Move over the microcode blob. */
>> + buf += sizeof(*mc) + mc->len;
>> + size -= sizeof(*mc) + mc->len;
>> +
>> + /*
>> + * Peek ahead. If we see the start of another container, we've
>> + * exhaused all microcode blobs in this container. Exit cleanly.
>> + */
>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
>> + break;
> While, as already indicated, I agree with shrinking the big comment,
> I think point 6 is what wants retaining in some form - it's not
> obvious at all why a subsequent container couldn't contain a higher
> rev ucode than what we've found. That comment refers us to docs, but
> I couldn't find anything to this effect in PM Vol 2. Assuming this
> indeed documented and true, with the comment extended accordingly
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I think it is referring to the internal PPR, which isn't even the one we
have access to.
As to the multiple containers aspect, I've deliberately "fixed" that in
patch 11 so we do scan all the way to the end.
Its a much more obvious way to do things, even if the default case is to
only provide a single container applicable to a specific family.
~Andrew
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 10:05 ` [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode() Andrew Cooper
@ 2020-03-31 15:07 ` Jan Beulich
2020-03-31 15:19 ` Andrew Cooper
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 15:07 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 12:05, Andrew Cooper wrote:
> @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
> return 0;
> }
>
> -static int scan_equiv_cpu_table(
> - const void *data,
> - size_t size_left,
> - size_t *offset)
> +static int scan_equiv_cpu_table(const struct container_equiv_table *et)
> {
> const struct cpu_signature *sig = &this_cpu(cpu_sig);
> - const struct mpbhdr *mpbuf;
> - const struct equiv_cpu_entry *eq;
> - unsigned int i, nr;
> -
> - if ( size_left < (sizeof(*mpbuf) + 4) ||
> - (mpbuf = data + *offset + 4,
> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
> - {
> - printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
> - return -EINVAL;
> - }
> -
> - *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
> -
> - if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
> - {
> - printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
> - return -EINVAL;
> - }
> -
> - if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
> - (eq = (const void *)mpbuf->data,
> - nr = mpbuf->len / sizeof(*eq),
> - eq[nr - 1].installed_cpu) )
Did this last check get lost? I can't seem to be able to identify
any possible replacement.
> static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
> {
> const struct microcode_patch *saved = NULL;
> struct microcode_patch *patch = NULL;
> - size_t offset = 0, saved_size = 0;
> + size_t saved_size = 0;
> int error = 0;
> - unsigned int cpu = smp_processor_id();
> - const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>
> - if ( size < 4 ||
> - *(const uint32_t *)buf != UCODE_MAGIC )
> + while ( size )
> {
> - printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
> - error = -EINVAL;
> - goto out;
> - }
> -
> - /*
> - * Multiple container file support:
> - * 1. check if this container file has equiv_cpu_id match
> - * 2. If not, fast-fwd to next container file
> - */
> - while ( offset < size )
> - {
> - error = scan_equiv_cpu_table(buf, size - offset, &offset);
> -
> - if ( !error || error != -ESRCH )
> - break;
> + const struct container_equiv_table *et;
> + bool skip_ucode;
>
> - error = container_fast_forward(buf, size - offset, &offset);
> - if ( error == -ENODATA )
> + if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
> {
> - ASSERT(offset == size);
> + printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
> + error = -EINVAL;
> break;
> }
> - if ( error )
> +
> + /* Move over UCODE_MAGIC. */
> + buf += 4;
> + size -= 4;
> +
> + if ( size < sizeof(*et) ||
> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
> + size - sizeof(*et) < et->len ||
> + et->len % sizeof(et->eq[0]) )
> {
> - printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
> - "microcode: Failed to update patch level. "
> - "Current lvl:%#x\n", cpu, sig->rev);
> + printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
> + error = -EINVAL;
> break;
> }
> - }
>
> - if ( error )
> - {
> - /*
> - * -ENODATA here means that the blob was parsed fine but no matching
> - * ucode was found. Don't return it to the caller.
> - */
> - if ( error == -ENODATA )
> - error = 0;
> -
> - goto out;
> - }
> + /* Move over the Equiv table. */
> + buf += sizeof(*et) + et->len;
> + size -= sizeof(*et) + et->len;
> +
> + error = scan_equiv_cpu_table(et);
> + if ( error && error != -ESRCH )
> + break;
With this the only non-zero value left for error is -ESRCH.
Hence ...
> + /* -ESRCH means no applicable microcode in this container. */
> + skip_ucode = error == -ESRCH;
... perhaps omit the "== -ESRCH" here, moving the comment up
ahead of the if()?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 14:55 ` Andrew Cooper
@ 2020-03-31 15:13 ` Jan Beulich
2020-03-31 15:47 ` Andrew Cooper
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 15:13 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 16:55, Andrew Cooper wrote:
> On 31/03/2020 15:51, Jan Beulich wrote:
>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>>> * It's possible the data file has multiple matching ucode,
>>> * lets keep searching till the latest version
>>> */
>>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
>>> - &offset)) == 0 )
>>> + buf += offset;
>>> + size -= offset;
>>> {
>>> - /*
>>> - * If the new ucode covers current CPU, compare ucodes and store the
>>> - * one with higher revision.
>>> - */
>>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
>>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
>>> + while ( size )
>>> {
>>> - xfree(saved);
>>> - saved = mc_amd->mpb;
>>> - }
>>> - else
>>> - {
>>> - xfree(mc_amd->mpb);
>>> - mc_amd->mpb = NULL;
>>> - }
>>> + const struct container_microcode *mc;
>>> +
>>> + if ( size < sizeof(*mc) ||
>>> + (mc = buf)->type != UCODE_UCODE_TYPE ||
>>> + size - sizeof(*mc) < mc->len ||
>>> + !verify_patch_size(mc->len) )
>>> + {
>>> + printk(XENLOG_ERR "microcode: Bad microcode data\n");
>>> + error = -EINVAL;
>>> + break;
>>> + }
>>>
>>> - if ( offset >= size )
>>> - break;
>>> + /*
>>> + * If the new ucode covers current CPU, compare ucodes and store the
>>> + * one with higher revision.
>>> + */
>>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
>>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
>>> + {
>>> + saved = mc->patch;
>>> + saved_size = mc->len;
>>> + }
>>>
>>> - /*
>>> - * 1. Given a situation where multiple containers exist and correct
>>> - * patch lives on a container that is not the last container.
>>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the
>>> - * earlier while() (On this case, matches on earlier container
>>> - * file and we break)
>>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>>> - * buf, size, &offset)) == 0 )
>>> - * 4. Find correct patch using microcode_fits() and apply the patch
>>> - * (Assume: apply_microcode() is successful)
>>> - * 5. The while() loop from (3) continues to parse the binary as
>>> - * there is a subsequent container file, but...
>>> - * 6. ...a correct patch can only be on one container and not on any
>>> - * subsequent ones. (Refer docs for more info) Therefore, we
>>> - * don't have to parse a subsequent container. So, we can abort
>>> - * the process here.
>>> - * 7. This ensures that we retain a success value (= 0) to 'error'
>>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
>>> - * false and returns -EINVAL.
>>> - */
>>> - if ( offset + SECTION_HDR_SIZE <= size &&
>>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>>> - break;
>>> + /* Move over the microcode blob. */
>>> + buf += sizeof(*mc) + mc->len;
>>> + size -= sizeof(*mc) + mc->len;
>>> +
>>> + /*
>>> + * Peek ahead. If we see the start of another container, we've
>>> + * exhaused all microcode blobs in this container. Exit cleanly.
>>> + */
>>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
>>> + break;
>> While, as already indicated, I agree with shrinking the big comment,
>> I think point 6 is what wants retaining in some form - it's not
>> obvious at all why a subsequent container couldn't contain a higher
>> rev ucode than what we've found. That comment refers us to docs, but
>> I couldn't find anything to this effect in PM Vol 2. Assuming this
>> indeed documented and true, with the comment extended accordingly
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I think it is referring to the internal PPR, which isn't even the one we
> have access to.
>
> As to the multiple containers aspect, I've deliberately "fixed" that in
> patch 11 so we do scan all the way to the end.
Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at
least the "Exit cleanly" part of the comment? You're merely breaking
the inner loop then ...
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 15:07 ` Jan Beulich
@ 2020-03-31 15:19 ` Andrew Cooper
2020-03-31 15:27 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 15:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31/03/2020 16:07, Jan Beulich wrote:
> On 31.03.2020 12:05, Andrew Cooper wrote:
>> @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
>> return 0;
>> }
>>
>> -static int scan_equiv_cpu_table(
>> - const void *data,
>> - size_t size_left,
>> - size_t *offset)
>> +static int scan_equiv_cpu_table(const struct container_equiv_table *et)
>> {
>> const struct cpu_signature *sig = &this_cpu(cpu_sig);
>> - const struct mpbhdr *mpbuf;
>> - const struct equiv_cpu_entry *eq;
>> - unsigned int i, nr;
>> -
>> - if ( size_left < (sizeof(*mpbuf) + 4) ||
>> - (mpbuf = data + *offset + 4,
>> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
>> - {
>> - printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
>> - return -EINVAL;
>> - }
>> -
>> - *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
>> -
>> - if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>> - {
>> - printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
>> - return -EINVAL;
>> - }
>> -
>> - if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
>> - (eq = (const void *)mpbuf->data,
>> - nr = mpbuf->len / sizeof(*eq),
>> - eq[nr - 1].installed_cpu) )
> Did this last check get lost? I can't seem to be able to identify
> any possible replacement.
Given the lack of a spec, I'm unsure whether to keep it or not.
It is necessary in the backport of patch 1, because find_equiv_cpu_id()
doesn't have mpbuf->len to hand, and relies on the sentinel to find the
end of the table.
OTOH, the new logic will cope perfectly well without a sentinel.
>
>> static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
>> {
>> const struct microcode_patch *saved = NULL;
>> struct microcode_patch *patch = NULL;
>> - size_t offset = 0, saved_size = 0;
>> + size_t saved_size = 0;
>> int error = 0;
>> - unsigned int cpu = smp_processor_id();
>> - const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>
>> - if ( size < 4 ||
>> - *(const uint32_t *)buf != UCODE_MAGIC )
>> + while ( size )
>> {
>> - printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
>> - error = -EINVAL;
>> - goto out;
>> - }
>> -
>> - /*
>> - * Multiple container file support:
>> - * 1. check if this container file has equiv_cpu_id match
>> - * 2. If not, fast-fwd to next container file
>> - */
>> - while ( offset < size )
>> - {
>> - error = scan_equiv_cpu_table(buf, size - offset, &offset);
>> -
>> - if ( !error || error != -ESRCH )
>> - break;
>> + const struct container_equiv_table *et;
>> + bool skip_ucode;
>>
>> - error = container_fast_forward(buf, size - offset, &offset);
>> - if ( error == -ENODATA )
>> + if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
>> {
>> - ASSERT(offset == size);
>> + printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
>> + error = -EINVAL;
>> break;
>> }
>> - if ( error )
>> +
>> + /* Move over UCODE_MAGIC. */
>> + buf += 4;
>> + size -= 4;
>> +
>> + if ( size < sizeof(*et) ||
>> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> + size - sizeof(*et) < et->len ||
>> + et->len % sizeof(et->eq[0]) )
>> {
>> - printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
>> - "microcode: Failed to update patch level. "
>> - "Current lvl:%#x\n", cpu, sig->rev);
>> + printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
>> + error = -EINVAL;
>> break;
>> }
>> - }
>>
>> - if ( error )
>> - {
>> - /*
>> - * -ENODATA here means that the blob was parsed fine but no matching
>> - * ucode was found. Don't return it to the caller.
>> - */
>> - if ( error == -ENODATA )
>> - error = 0;
>> -
>> - goto out;
>> - }
>> + /* Move over the Equiv table. */
>> + buf += sizeof(*et) + et->len;
>> + size -= sizeof(*et) + et->len;
>> +
>> + error = scan_equiv_cpu_table(et);
>> + if ( error && error != -ESRCH )
>> + break;
> With this the only non-zero value left for error is -ESRCH.
> Hence ...
>
>> + /* -ESRCH means no applicable microcode in this container. */
>> + skip_ucode = error == -ESRCH;
> ... perhaps omit the "== -ESRCH" here, moving the comment up
> ahead of the if()?
That doesn't work, because you've got to reset error to 0 somewhere (to
avoid it leaking out if you don't find suitable microcode), and it can't
be before checking for errors in general. It can't easily become a
conditional because skip_ucode needs setting unconditionally.
The only other correct way I can see of arranging this code would be
skip_ucode = error == -ESRCH;
if ( error == -ESRCH )
error = 0;
else if ( error )
break;
which doesn't look to be better.
I have been debating quite heavily whether -ESRCH is best here, or using
-ve, 0 and 1. However, this doesn't lead to prettier code AFAICT, and
gains an ambiguous use for a variable named "error".
~Andrew
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 15:19 ` Andrew Cooper
@ 2020-03-31 15:27 ` Jan Beulich
2020-03-31 15:55 ` Andrew Cooper
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 15:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 17:19, Andrew Cooper wrote:
> On 31/03/2020 16:07, Jan Beulich wrote:
>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>> @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
>>> return 0;
>>> }
>>>
>>> -static int scan_equiv_cpu_table(
>>> - const void *data,
>>> - size_t size_left,
>>> - size_t *offset)
>>> +static int scan_equiv_cpu_table(const struct container_equiv_table *et)
>>> {
>>> const struct cpu_signature *sig = &this_cpu(cpu_sig);
>>> - const struct mpbhdr *mpbuf;
>>> - const struct equiv_cpu_entry *eq;
>>> - unsigned int i, nr;
>>> -
>>> - if ( size_left < (sizeof(*mpbuf) + 4) ||
>>> - (mpbuf = data + *offset + 4,
>>> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
>>> - {
>>> - printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
>>> - return -EINVAL;
>>> - }
>>> -
>>> - *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
>>> -
>>> - if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>>> - {
>>> - printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
>>> - return -EINVAL;
>>> - }
>>> -
>>> - if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
>>> - (eq = (const void *)mpbuf->data,
>>> - nr = mpbuf->len / sizeof(*eq),
>>> - eq[nr - 1].installed_cpu) )
>> Did this last check get lost? I can't seem to be able to identify
>> any possible replacement.
>
> Given the lack of a spec, I'm unsure whether to keep it or not.
>
> It is necessary in the backport of patch 1, because find_equiv_cpu_id()
> doesn't have mpbuf->len to hand, and relies on the sentinel to find the
> end of the table.
>
> OTOH, the new logic will cope perfectly well without a sentinel.
Okay.
>>> static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
>>> {
>>> const struct microcode_patch *saved = NULL;
>>> struct microcode_patch *patch = NULL;
>>> - size_t offset = 0, saved_size = 0;
>>> + size_t saved_size = 0;
>>> int error = 0;
>>> - unsigned int cpu = smp_processor_id();
>>> - const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>>
>>> - if ( size < 4 ||
>>> - *(const uint32_t *)buf != UCODE_MAGIC )
>>> + while ( size )
>>> {
>>> - printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
>>> - error = -EINVAL;
>>> - goto out;
>>> - }
>>> -
>>> - /*
>>> - * Multiple container file support:
>>> - * 1. check if this container file has equiv_cpu_id match
>>> - * 2. If not, fast-fwd to next container file
>>> - */
>>> - while ( offset < size )
>>> - {
>>> - error = scan_equiv_cpu_table(buf, size - offset, &offset);
>>> -
>>> - if ( !error || error != -ESRCH )
>>> - break;
>>> + const struct container_equiv_table *et;
>>> + bool skip_ucode;
>>>
>>> - error = container_fast_forward(buf, size - offset, &offset);
>>> - if ( error == -ENODATA )
>>> + if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
>>> {
>>> - ASSERT(offset == size);
>>> + printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
>>> + error = -EINVAL;
>>> break;
>>> }
>>> - if ( error )
>>> +
>>> + /* Move over UCODE_MAGIC. */
>>> + buf += 4;
>>> + size -= 4;
>>> +
>>> + if ( size < sizeof(*et) ||
>>> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>>> + size - sizeof(*et) < et->len ||
>>> + et->len % sizeof(et->eq[0]) )
>>> {
>>> - printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
>>> - "microcode: Failed to update patch level. "
>>> - "Current lvl:%#x\n", cpu, sig->rev);
>>> + printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
>>> + error = -EINVAL;
>>> break;
>>> }
>>> - }
>>>
>>> - if ( error )
>>> - {
>>> - /*
>>> - * -ENODATA here means that the blob was parsed fine but no matching
>>> - * ucode was found. Don't return it to the caller.
>>> - */
>>> - if ( error == -ENODATA )
>>> - error = 0;
>>> -
>>> - goto out;
>>> - }
>>> + /* Move over the Equiv table. */
>>> + buf += sizeof(*et) + et->len;
>>> + size -= sizeof(*et) + et->len;
>>> +
>>> + error = scan_equiv_cpu_table(et);
>>> + if ( error && error != -ESRCH )
>>> + break;
>> With this the only non-zero value left for error is -ESRCH.
>> Hence ...
>>
>>> + /* -ESRCH means no applicable microcode in this container. */
>>> + skip_ucode = error == -ESRCH;
>> ... perhaps omit the "== -ESRCH" here, moving the comment up
>> ahead of the if()?
>
> That doesn't work, because you've got to reset error to 0 somewhere (to
> avoid it leaking out if you don't find suitable microcode), and it can't
> be before checking for errors in general. It can't easily become a
> conditional because skip_ucode needs setting unconditionally.
I don't follow - what's wrong with
/* -ESRCH means no applicable microcode in this container. */
if ( error && error != -ESRCH )
break;
skip_ucode = error;
error = 0;
?
> I have been debating quite heavily whether -ESRCH is best here, or using
> -ve, 0 and 1. However, this doesn't lead to prettier code AFAICT, and
> gains an ambiguous use for a variable named "error".
I'm fine with that choice of yours.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 15:13 ` Jan Beulich
@ 2020-03-31 15:47 ` Andrew Cooper
2020-03-31 15:52 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 15:47 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31/03/2020 16:13, Jan Beulich wrote:
> On 31.03.2020 16:55, Andrew Cooper wrote:
>> On 31/03/2020 15:51, Jan Beulich wrote:
>>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>>>> * It's possible the data file has multiple matching ucode,
>>>> * lets keep searching till the latest version
>>>> */
>>>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
>>>> - &offset)) == 0 )
>>>> + buf += offset;
>>>> + size -= offset;
>>>> {
>>>> - /*
>>>> - * If the new ucode covers current CPU, compare ucodes and store the
>>>> - * one with higher revision.
>>>> - */
>>>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
>>>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
>>>> + while ( size )
>>>> {
>>>> - xfree(saved);
>>>> - saved = mc_amd->mpb;
>>>> - }
>>>> - else
>>>> - {
>>>> - xfree(mc_amd->mpb);
>>>> - mc_amd->mpb = NULL;
>>>> - }
>>>> + const struct container_microcode *mc;
>>>> +
>>>> + if ( size < sizeof(*mc) ||
>>>> + (mc = buf)->type != UCODE_UCODE_TYPE ||
>>>> + size - sizeof(*mc) < mc->len ||
>>>> + !verify_patch_size(mc->len) )
>>>> + {
>>>> + printk(XENLOG_ERR "microcode: Bad microcode data\n");
>>>> + error = -EINVAL;
>>>> + break;
>>>> + }
>>>>
>>>> - if ( offset >= size )
>>>> - break;
>>>> + /*
>>>> + * If the new ucode covers current CPU, compare ucodes and store the
>>>> + * one with higher revision.
>>>> + */
>>>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
>>>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
>>>> + {
>>>> + saved = mc->patch;
>>>> + saved_size = mc->len;
>>>> + }
>>>>
>>>> - /*
>>>> - * 1. Given a situation where multiple containers exist and correct
>>>> - * patch lives on a container that is not the last container.
>>>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the
>>>> - * earlier while() (On this case, matches on earlier container
>>>> - * file and we break)
>>>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>>>> - * buf, size, &offset)) == 0 )
>>>> - * 4. Find correct patch using microcode_fits() and apply the patch
>>>> - * (Assume: apply_microcode() is successful)
>>>> - * 5. The while() loop from (3) continues to parse the binary as
>>>> - * there is a subsequent container file, but...
>>>> - * 6. ...a correct patch can only be on one container and not on any
>>>> - * subsequent ones. (Refer docs for more info) Therefore, we
>>>> - * don't have to parse a subsequent container. So, we can abort
>>>> - * the process here.
>>>> - * 7. This ensures that we retain a success value (= 0) to 'error'
>>>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
>>>> - * false and returns -EINVAL.
>>>> - */
>>>> - if ( offset + SECTION_HDR_SIZE <= size &&
>>>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>>>> - break;
>>>> + /* Move over the microcode blob. */
>>>> + buf += sizeof(*mc) + mc->len;
>>>> + size -= sizeof(*mc) + mc->len;
>>>> +
>>>> + /*
>>>> + * Peek ahead. If we see the start of another container, we've
>>>> + * exhaused all microcode blobs in this container. Exit cleanly.
>>>> + */
>>>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
>>>> + break;
>>> While, as already indicated, I agree with shrinking the big comment,
>>> I think point 6 is what wants retaining in some form - it's not
>>> obvious at all why a subsequent container couldn't contain a higher
>>> rev ucode than what we've found. That comment refers us to docs, but
>>> I couldn't find anything to this effect in PM Vol 2. Assuming this
>>> indeed documented and true, with the comment extended accordingly
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> I think it is referring to the internal PPR, which isn't even the one we
>> have access to.
>>
>> As to the multiple containers aspect, I've deliberately "fixed" that in
>> patch 11 so we do scan all the way to the end.
> Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at
> least the "Exit cleanly" part of the comment? You're merely breaking
> the inner loop then ...
I'd still argue that "exit cleanly" is fine in context.
Without it, the end of buffer case happens fine as size becomes 0 and
terminates both loops, but in the case that there is a following
container, without it we fail because of the "!= UCODE_UCODE_TYPE" check.
~Andrew
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
2020-03-31 15:47 ` Andrew Cooper
@ 2020-03-31 15:52 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 15:52 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 17:47, Andrew Cooper wrote:
> On 31/03/2020 16:13, Jan Beulich wrote:
>> On 31.03.2020 16:55, Andrew Cooper wrote:
>>> On 31/03/2020 15:51, Jan Beulich wrote:
>>>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>>>>> * It's possible the data file has multiple matching ucode,
>>>>> * lets keep searching till the latest version
>>>>> */
>>>>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
>>>>> - &offset)) == 0 )
>>>>> + buf += offset;
>>>>> + size -= offset;
>>>>> {
>>>>> - /*
>>>>> - * If the new ucode covers current CPU, compare ucodes and store the
>>>>> - * one with higher revision.
>>>>> - */
>>>>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
>>>>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
>>>>> + while ( size )
>>>>> {
>>>>> - xfree(saved);
>>>>> - saved = mc_amd->mpb;
>>>>> - }
>>>>> - else
>>>>> - {
>>>>> - xfree(mc_amd->mpb);
>>>>> - mc_amd->mpb = NULL;
>>>>> - }
>>>>> + const struct container_microcode *mc;
>>>>> +
>>>>> + if ( size < sizeof(*mc) ||
>>>>> + (mc = buf)->type != UCODE_UCODE_TYPE ||
>>>>> + size - sizeof(*mc) < mc->len ||
>>>>> + !verify_patch_size(mc->len) )
>>>>> + {
>>>>> + printk(XENLOG_ERR "microcode: Bad microcode data\n");
>>>>> + error = -EINVAL;
>>>>> + break;
>>>>> + }
>>>>>
>>>>> - if ( offset >= size )
>>>>> - break;
>>>>> + /*
>>>>> + * If the new ucode covers current CPU, compare ucodes and store the
>>>>> + * one with higher revision.
>>>>> + */
>>>>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
>>>>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
>>>>> + {
>>>>> + saved = mc->patch;
>>>>> + saved_size = mc->len;
>>>>> + }
>>>>>
>>>>> - /*
>>>>> - * 1. Given a situation where multiple containers exist and correct
>>>>> - * patch lives on a container that is not the last container.
>>>>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the
>>>>> - * earlier while() (On this case, matches on earlier container
>>>>> - * file and we break)
>>>>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>>>>> - * buf, size, &offset)) == 0 )
>>>>> - * 4. Find correct patch using microcode_fits() and apply the patch
>>>>> - * (Assume: apply_microcode() is successful)
>>>>> - * 5. The while() loop from (3) continues to parse the binary as
>>>>> - * there is a subsequent container file, but...
>>>>> - * 6. ...a correct patch can only be on one container and not on any
>>>>> - * subsequent ones. (Refer docs for more info) Therefore, we
>>>>> - * don't have to parse a subsequent container. So, we can abort
>>>>> - * the process here.
>>>>> - * 7. This ensures that we retain a success value (= 0) to 'error'
>>>>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
>>>>> - * false and returns -EINVAL.
>>>>> - */
>>>>> - if ( offset + SECTION_HDR_SIZE <= size &&
>>>>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>>>>> - break;
>>>>> + /* Move over the microcode blob. */
>>>>> + buf += sizeof(*mc) + mc->len;
>>>>> + size -= sizeof(*mc) + mc->len;
>>>>> +
>>>>> + /*
>>>>> + * Peek ahead. If we see the start of another container, we've
>>>>> + * exhaused all microcode blobs in this container. Exit cleanly.
>>>>> + */
>>>>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
>>>>> + break;
>>>> While, as already indicated, I agree with shrinking the big comment,
>>>> I think point 6 is what wants retaining in some form - it's not
>>>> obvious at all why a subsequent container couldn't contain a higher
>>>> rev ucode than what we've found. That comment refers us to docs, but
>>>> I couldn't find anything to this effect in PM Vol 2. Assuming this
>>>> indeed documented and true, with the comment extended accordingly
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> I think it is referring to the internal PPR, which isn't even the one we
>>> have access to.
>>>
>>> As to the multiple containers aspect, I've deliberately "fixed" that in
>>> patch 11 so we do scan all the way to the end.
>> Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at
>> least the "Exit cleanly" part of the comment? You're merely breaking
>> the inner loop then ...
>
> I'd still argue that "exit cleanly" is fine in context.
Maybe; to me "exit" suggests more like being done with all processing /
looping. I'm not going to insist - you're the native speaker.
> Without it, the end of buffer case happens fine as size becomes 0 and
> terminates both loops, but in the case that there is a following
> container, without it we fail because of the "!= UCODE_UCODE_TYPE" check.
Of course.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 15:27 ` Jan Beulich
@ 2020-03-31 15:55 ` Andrew Cooper
2020-03-31 16:00 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Andrew Cooper @ 2020-03-31 15:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31/03/2020 16:27, Jan Beulich wrote:
> On 31.03.2020 17:19, Andrew Cooper wrote:
>> On 31/03/2020 16:07, Jan Beulich wrote:
>>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>>> @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
>>>> return 0;
>>>> }
>>>>
>>>> -static int scan_equiv_cpu_table(
>>>> - const void *data,
>>>> - size_t size_left,
>>>> - size_t *offset)
>>>> +static int scan_equiv_cpu_table(const struct container_equiv_table *et)
>>>> {
>>>> const struct cpu_signature *sig = &this_cpu(cpu_sig);
>>>> - const struct mpbhdr *mpbuf;
>>>> - const struct equiv_cpu_entry *eq;
>>>> - unsigned int i, nr;
>>>> -
>>>> - if ( size_left < (sizeof(*mpbuf) + 4) ||
>>>> - (mpbuf = data + *offset + 4,
>>>> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
>>>> - {
>>>> - printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
>>>> - return -EINVAL;
>>>> - }
>>>> -
>>>> - *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
>>>> -
>>>> - if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>>>> - {
>>>> - printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
>>>> - return -EINVAL;
>>>> - }
>>>> -
>>>> - if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
>>>> - (eq = (const void *)mpbuf->data,
>>>> - nr = mpbuf->len / sizeof(*eq),
>>>> - eq[nr - 1].installed_cpu) )
>>> Did this last check get lost? I can't seem to be able to identify
>>> any possible replacement.
>> Given the lack of a spec, I'm unsure whether to keep it or not.
>>
>> It is necessary in the backport of patch 1, because find_equiv_cpu_id()
>> doesn't have mpbuf->len to hand, and relies on the sentinel to find the
>> end of the table.
>>
>> OTOH, the new logic will cope perfectly well without a sentinel.
> Okay.
>
>>>> static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
>>>> {
>>>> const struct microcode_patch *saved = NULL;
>>>> struct microcode_patch *patch = NULL;
>>>> - size_t offset = 0, saved_size = 0;
>>>> + size_t saved_size = 0;
>>>> int error = 0;
>>>> - unsigned int cpu = smp_processor_id();
>>>> - const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>>>
>>>> - if ( size < 4 ||
>>>> - *(const uint32_t *)buf != UCODE_MAGIC )
>>>> + while ( size )
>>>> {
>>>> - printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
>>>> - error = -EINVAL;
>>>> - goto out;
>>>> - }
>>>> -
>>>> - /*
>>>> - * Multiple container file support:
>>>> - * 1. check if this container file has equiv_cpu_id match
>>>> - * 2. If not, fast-fwd to next container file
>>>> - */
>>>> - while ( offset < size )
>>>> - {
>>>> - error = scan_equiv_cpu_table(buf, size - offset, &offset);
>>>> -
>>>> - if ( !error || error != -ESRCH )
>>>> - break;
>>>> + const struct container_equiv_table *et;
>>>> + bool skip_ucode;
>>>>
>>>> - error = container_fast_forward(buf, size - offset, &offset);
>>>> - if ( error == -ENODATA )
>>>> + if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
>>>> {
>>>> - ASSERT(offset == size);
>>>> + printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
>>>> + error = -EINVAL;
>>>> break;
>>>> }
>>>> - if ( error )
>>>> +
>>>> + /* Move over UCODE_MAGIC. */
>>>> + buf += 4;
>>>> + size -= 4;
>>>> +
>>>> + if ( size < sizeof(*et) ||
>>>> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>>>> + size - sizeof(*et) < et->len ||
>>>> + et->len % sizeof(et->eq[0]) )
>>>> {
>>>> - printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
>>>> - "microcode: Failed to update patch level. "
>>>> - "Current lvl:%#x\n", cpu, sig->rev);
>>>> + printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
>>>> + error = -EINVAL;
>>>> break;
>>>> }
>>>> - }
>>>>
>>>> - if ( error )
>>>> - {
>>>> - /*
>>>> - * -ENODATA here means that the blob was parsed fine but no matching
>>>> - * ucode was found. Don't return it to the caller.
>>>> - */
>>>> - if ( error == -ENODATA )
>>>> - error = 0;
>>>> -
>>>> - goto out;
>>>> - }
>>>> + /* Move over the Equiv table. */
>>>> + buf += sizeof(*et) + et->len;
>>>> + size -= sizeof(*et) + et->len;
>>>> +
>>>> + error = scan_equiv_cpu_table(et);
>>>> + if ( error && error != -ESRCH )
>>>> + break;
>>> With this the only non-zero value left for error is -ESRCH.
>>> Hence ...
>>>
>>>> + /* -ESRCH means no applicable microcode in this container. */
>>>> + skip_ucode = error == -ESRCH;
>>> ... perhaps omit the "== -ESRCH" here, moving the comment up
>>> ahead of the if()?
>> That doesn't work, because you've got to reset error to 0 somewhere (to
>> avoid it leaking out if you don't find suitable microcode), and it can't
>> be before checking for errors in general. It can't easily become a
>> conditional because skip_ucode needs setting unconditionally.
> I don't follow - what's wrong with
>
> /* -ESRCH means no applicable microcode in this container. */
> if ( error && error != -ESRCH )
> break;
> skip_ucode = error;
> error = 0;
>
> ?
Oh - I misinterpreted your suggestion. That looks ok.
Are you happy overall with this change?
~Andrew
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
2020-03-31 15:55 ` Andrew Cooper
@ 2020-03-31 16:00 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2020-03-31 16:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 31.03.2020 17:55, Andrew Cooper wrote:
> On 31/03/2020 16:27, Jan Beulich wrote:
>> On 31.03.2020 17:19, Andrew Cooper wrote:
>>> On 31/03/2020 16:07, Jan Beulich wrote:
>>>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>>>> @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int scan_equiv_cpu_table(
>>>>> - const void *data,
>>>>> - size_t size_left,
>>>>> - size_t *offset)
>>>>> +static int scan_equiv_cpu_table(const struct container_equiv_table *et)
>>>>> {
>>>>> const struct cpu_signature *sig = &this_cpu(cpu_sig);
>>>>> - const struct mpbhdr *mpbuf;
>>>>> - const struct equiv_cpu_entry *eq;
>>>>> - unsigned int i, nr;
>>>>> -
>>>>> - if ( size_left < (sizeof(*mpbuf) + 4) ||
>>>>> - (mpbuf = data + *offset + 4,
>>>>> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) )
>>>>> - {
>>>>> - printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n");
>>>>> - return -EINVAL;
>>>>> - }
>>>>> -
>>>>> - *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */
>>>>> -
>>>>> - if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>>>>> - {
>>>>> - printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n");
>>>>> - return -EINVAL;
>>>>> - }
>>>>> -
>>>>> - if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) ||
>>>>> - (eq = (const void *)mpbuf->data,
>>>>> - nr = mpbuf->len / sizeof(*eq),
>>>>> - eq[nr - 1].installed_cpu) )
>>>> Did this last check get lost? I can't seem to be able to identify
>>>> any possible replacement.
>>> Given the lack of a spec, I'm unsure whether to keep it or not.
>>>
>>> It is necessary in the backport of patch 1, because find_equiv_cpu_id()
>>> doesn't have mpbuf->len to hand, and relies on the sentinel to find the
>>> end of the table.
>>>
>>> OTOH, the new logic will cope perfectly well without a sentinel.
>> Okay.
>>
>>>>> static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size)
>>>>> {
>>>>> const struct microcode_patch *saved = NULL;
>>>>> struct microcode_patch *patch = NULL;
>>>>> - size_t offset = 0, saved_size = 0;
>>>>> + size_t saved_size = 0;
>>>>> int error = 0;
>>>>> - unsigned int cpu = smp_processor_id();
>>>>> - const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>>>>
>>>>> - if ( size < 4 ||
>>>>> - *(const uint32_t *)buf != UCODE_MAGIC )
>>>>> + while ( size )
>>>>> {
>>>>> - printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
>>>>> - error = -EINVAL;
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - /*
>>>>> - * Multiple container file support:
>>>>> - * 1. check if this container file has equiv_cpu_id match
>>>>> - * 2. If not, fast-fwd to next container file
>>>>> - */
>>>>> - while ( offset < size )
>>>>> - {
>>>>> - error = scan_equiv_cpu_table(buf, size - offset, &offset);
>>>>> -
>>>>> - if ( !error || error != -ESRCH )
>>>>> - break;
>>>>> + const struct container_equiv_table *et;
>>>>> + bool skip_ucode;
>>>>>
>>>>> - error = container_fast_forward(buf, size - offset, &offset);
>>>>> - if ( error == -ENODATA )
>>>>> + if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC )
>>>>> {
>>>>> - ASSERT(offset == size);
>>>>> + printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n");
>>>>> + error = -EINVAL;
>>>>> break;
>>>>> }
>>>>> - if ( error )
>>>>> +
>>>>> + /* Move over UCODE_MAGIC. */
>>>>> + buf += 4;
>>>>> + size -= 4;
>>>>> +
>>>>> + if ( size < sizeof(*et) ||
>>>>> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>>>>> + size - sizeof(*et) < et->len ||
>>>>> + et->len % sizeof(et->eq[0]) )
>>>>> {
>>>>> - printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
>>>>> - "microcode: Failed to update patch level. "
>>>>> - "Current lvl:%#x\n", cpu, sig->rev);
>>>>> + printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
>>>>> + error = -EINVAL;
>>>>> break;
>>>>> }
>>>>> - }
>>>>>
>>>>> - if ( error )
>>>>> - {
>>>>> - /*
>>>>> - * -ENODATA here means that the blob was parsed fine but no matching
>>>>> - * ucode was found. Don't return it to the caller.
>>>>> - */
>>>>> - if ( error == -ENODATA )
>>>>> - error = 0;
>>>>> -
>>>>> - goto out;
>>>>> - }
>>>>> + /* Move over the Equiv table. */
>>>>> + buf += sizeof(*et) + et->len;
>>>>> + size -= sizeof(*et) + et->len;
>>>>> +
>>>>> + error = scan_equiv_cpu_table(et);
>>>>> + if ( error && error != -ESRCH )
>>>>> + break;
>>>> With this the only non-zero value left for error is -ESRCH.
>>>> Hence ...
>>>>
>>>>> + /* -ESRCH means no applicable microcode in this container. */
>>>>> + skip_ucode = error == -ESRCH;
>>>> ... perhaps omit the "== -ESRCH" here, moving the comment up
>>>> ahead of the if()?
>>> That doesn't work, because you've got to reset error to 0 somewhere (to
>>> avoid it leaking out if you don't find suitable microcode), and it can't
>>> be before checking for errors in general. It can't easily become a
>>> conditional because skip_ucode needs setting unconditionally.
>> I don't follow - what's wrong with
>>
>> /* -ESRCH means no applicable microcode in this container. */
>> if ( error && error != -ESRCH )
>> break;
>> skip_ucode = error;
>> error = 0;
>>
>> ?
>
> Oh - I misinterpreted your suggestion. That looks ok.
>
> Are you happy overall with this change?
Yes, you did address the other question I had:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-03-31 16:00 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 10:05 [PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD) Andrew Cooper
2020-03-31 10:05 ` [PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing Andrew Cooper
2020-03-31 10:05 ` [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode() Andrew Cooper
2020-03-31 14:27 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb Andrew Cooper
2020-03-31 14:28 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info() Andrew Cooper
2020-03-31 14:29 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely Andrew Cooper
2020-03-31 14:36 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd() Andrew Cooper
2020-03-31 14:38 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits() Andrew Cooper
2020-03-31 14:39 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode() Andrew Cooper
2020-03-31 14:41 ` Jan Beulich
2020-03-31 14:43 ` Andrew Cooper
2020-03-31 10:05 ` [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode() Andrew Cooper
2020-03-31 14:51 ` Jan Beulich
2020-03-31 14:55 ` Andrew Cooper
2020-03-31 15:13 ` Jan Beulich
2020-03-31 15:47 ` Andrew Cooper
2020-03-31 15:52 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 10/11] x86/ucode/amd: Fold structures together Andrew Cooper
2020-03-31 14:53 ` Jan Beulich
2020-03-31 10:05 ` [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode() Andrew Cooper
2020-03-31 15:07 ` Jan Beulich
2020-03-31 15:19 ` Andrew Cooper
2020-03-31 15:27 ` Jan Beulich
2020-03-31 15:55 ` Andrew Cooper
2020-03-31 16:00 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.