[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] x86, amd_ucode: Support multiple container files appended together
On 6/20/2014 10:57 AM, Boris Ostrovsky wrote:
On 06/19/2014 11:14 AM, Aravind Gopalakrishnan wrote:
This patch adds support for parsing through multiple AMD container
binaries concatenated together. It is a feature already present in
Linux.
Link to linux patch:
http://lkml.kernel.org/r/1370463236-2115-3-git-send-email-jacob.shin@xxxxxxx
While at it, define HDR_SIZES and use these for clarity.
Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>
Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
---
xen/arch/x86/microcode_amd.c | 120
++++++++++++++++++++++++++++++++++++------
1 file changed, 103 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e83f4b6..6478dda 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -29,6 +29,9 @@
#define pr_debug(x...) ((void)0)
+#define CONT_HDR_SIZE 12
+#define SECTION_HDR_SIZE 8
+
struct __packed equiv_cpu_entry {
uint32_t installed_cpu;
uint32_t fixed_errata_mask;
@@ -124,6 +127,25 @@ static bool_t verify_patch_size(uint32_t
patch_size)
return (patch_size <= max_size);
}
+static void 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;
+
+ 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;
+ break;
+ }
+ }
+}
+
static bool_t microcode_fits(const struct microcode_amd *mc_amd,
int cpu)
{
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
@@ -131,21 +153,13 @@ static bool_t microcode_fits(const struct
microcode_amd *mc_amd, int cpu)
const struct equiv_cpu_entry *equiv_cpu_table =
mc_amd->equiv_cpu_table;
unsigned int current_cpu_id;
unsigned int equiv_cpu_id = 0x0;
- unsigned int i;
/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
current_cpu_id = cpuid_eax(0x00000001);
- 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;
- break;
- }
- }
+ find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id);
if ( !equiv_cpu_id )
return 0;
@@ -236,7 +250,15 @@ static int get_ucode_from_buffer_amd(
mpbuf = (const struct mpbhdr *)&bufp[off];
if ( mpbuf->type != UCODE_UCODE_TYPE )
{
- printk(KERN_ERR "microcode: Wrong microcode payload type
field\n");
+ /*
+ * We will hit this condition only if an update has succeeded
+ * but there is still another container file being parsed.
+ * In that case, there is no need of this ERR message to be
+ * printed.
+ */
+ if ( mpbuf->type != UCODE_MAGIC )
+ printk(KERN_ERR "microcode: Wrong microcode payload type
field\n");
+
return -EINVAL;
}
@@ -260,7 +282,7 @@ static int get_ucode_from_buffer_amd(
}
memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
- *offset = off + mpbuf->len + 8;
+ *offset = off + mpbuf->len + SECTION_HDR_SIZE;
pr_debug("microcode: CPU%d size %zu, block size %u offset %zu
equivID %#x rev %#x\n",
raw_smp_processor_id(), bufsize, mpbuf->len, off,
@@ -272,13 +294,16 @@ static int get_ucode_from_buffer_amd(
static int install_equiv_cpu_table(
struct microcode_amd *mc_amd,
- const uint32_t *buf,
- size_t *offset)
+ const uint8_t *data,
+ size_t *tot_size,
Why is this a pointer? This routine does not modify the value.
Yeah. No point. I'll fix it.
Thanks for the pointer :)
+ size_t *curr_offset)
{
+ size_t off = *curr_offset;
+ uint32_t *buf = (uint32_t *) &data[off];
const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
/* No more data */
- if ( mpbuf->len + 12 >= *offset )
+ if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size )
return -EINVAL;
if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
@@ -303,7 +328,32 @@ static int install_equiv_cpu_table(
memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len);
mc_amd->equiv_cpu_table_size = mpbuf->len;
- *offset = mpbuf->len + 12; /* add header length */
+ *curr_offset += mpbuf->len + CONT_HDR_SIZE; /* add header
length */
+
+ return 0;
+}
+
+static int container_fast_forward(const uint8_t *data, size_t
size_left, size_t *offset)
+{
+ size_t size, off;
+ uint32_t *header;
+
+ while ( size_left )
+ {
+ off = *offset;
+ header = (uint32_t *) &data[off];
+ if ( header[0] == UCODE_MAGIC &&
+ header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
+ break;
+
+ size = header[1] + SECTION_HDR_SIZE;
+ *offset += size;
+ size_left -= size;
+
+ }
+
+ if ( !size_left )
+ return -EINVAL;
return 0;
}
@@ -311,14 +361,19 @@ static int install_equiv_cpu_table(
static int cpu_request_microcode(int cpu, const void *buf, size_t
bufsize)
{
struct microcode_amd *mc_amd, *mc_old;
- size_t offset = bufsize;
+ size_t offset = 0;
+ size_t tot_size = bufsize;
size_t last_offset, applied_offset = 0;
int error = 0, save_error = 1;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ unsigned int current_cpu_id;
+ unsigned int equiv_cpu_id = 0x0;
/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
+ current_cpu_id = cpuid_eax(0x00000001);
+
if ( *(const uint32_t *)buf != UCODE_MAGIC )
{
printk(KERN_ERR "microcode: Wrong microcode patch file
magic\n");
@@ -334,7 +389,30 @@ static int cpu_request_microcode(int cpu, const
void *buf, size_t bufsize)
goto out;
}
- error = install_equiv_cpu_table(mc_amd, buf, &offset);
+ /*
+ * 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 ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size,
+ &offset)) == 0 )
install_equiv_cpu_table() checks whether container size is larger than
the buffer size.
Not really.
It is basically checking if we have data at all to parse (like the
comment says:/* No more data */)
If we have multiple containers merged I think tot_size is the size of
the merged file, not of an individual container.
That's right.
And this makes the first check in install_equiv_cpu_table() not
especially meaningful.
Theoretically-
If it so happens that we don't have any patch files and just the
container header,
then mpbuf->len would be zero and
if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size )
will fail in this case- (which gives some meaning to the check being there)
1. If containers are concatenated, but there is no matching patch on the
first one
2. So, we fast-fwd to next container file but this one has mpbuf->len=0
Which leads me to another scenario though-
1. If we do have multiple containers and if right patch is not on the
first one, but on some subsequent container
2. If the first container has mpbuf->len=0
Then we'd just error out right there. Hmm.. I'll have to re-work the
logic then.
Come to think, if the first container is corrupted for some reason and
returns error (or there's an -ENOMEM), then we are going to return
pre-maturely
Instead, we should probably (try to) forward to next container and look
for patches there as well.
Shall fix this logic in a v2..
Thanks,
-Aravind
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
