Mailing List Archive

# HG changeset patch
# User Keir Fraser <keir.fraser@citrix.com>
# Date 1232458028 0
# Node ID fb0dc8143932c3beeda160585597a42b80549b0d
# Parent 0ab2b283e89230d485ded93acea58da5105df802
x86: update microcode support

- Container header file holding the patches changed. Update to new
format.
- in cpu_request_microcode() move heap re-allocation & copy out of the
loop.
Side-effect: Remove limitation in only supporting fixed sized
microcode patches. Also simplifies code a lot.
- cleanup: use rdmsr and wrmsrl instead of inlined assembler
- pass ucode_cpu_info as arguments. Improves reentrancy.
- cleanup: simplify struct ucode_cpu_info and remove
get_matching_microcode hook. Side-effect: reduces kernel size.
- bugfix: fix xen kernel memory leak in error path. equiv_cpu_table
was not freed.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
---
xen/arch/x86/microcode.c | 61 +++------
xen/arch/x86/microcode_amd.c | 271 +++++++++++++++++-----------------------
xen/arch/x86/microcode_intel.c | 9 -
xen/include/asm-x86/microcode.h | 53 +++----
4 files changed, 171 insertions(+), 223 deletions(-)

diff -r 0ab2b283e892 -r fb0dc8143932 xen/arch/x86/microcode.c
--- a/xen/arch/x86/microcode.c Tue Jan 20 13:22:28 2009 +0000
+++ b/xen/arch/x86/microcode.c Tue Jan 20 13:27:08 2009 +0000
@@ -49,39 +49,28 @@ struct microcode_info {
char buffer[1];
};

-static void microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(struct ucode_cpu_info *uci, int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
spin_lock(&microcode_mutex);
- xfree(uci->mc.valid_mc);
- uci->mc.valid_mc = NULL;
- uci->valid = 0;
+ xfree(uci->mc.mc_valid);
+ uci->mc.mc_valid = NULL;
spin_unlock(&microcode_mutex);
}

-static int collect_cpu_info(int cpu)
+static int collect_cpu_info(struct ucode_cpu_info *uci, int cpu)
+{
+ memset(uci, 0, sizeof(*uci));
+ return microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+}
+
+static int microcode_resume_cpu(struct ucode_cpu_info *uci, int cpu)
{
int err = 0;
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- memset(uci, 0, sizeof(*uci));
- err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
- if ( !err )
- uci->valid = 1;
-
- return err;
-}
-
-static int microcode_resume_cpu(int cpu)
-{
- int err = 0;
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
struct cpu_signature nsig;

gdprintk(XENLOG_INFO, "microcode: CPU%d resumed\n", cpu);

- if ( !uci->mc.valid_mc )
+ if ( !uci->mc.mc_valid )
return -EIO;

/*
@@ -91,43 +80,38 @@ static int microcode_resume_cpu(int cpu)
err = microcode_ops->collect_cpu_info(cpu, &nsig);
if ( err )
{
- microcode_fini_cpu(cpu);
+ microcode_fini_cpu(uci, cpu);
return err;
}

if ( memcmp(&nsig, &uci->cpu_sig, sizeof(nsig)) )
{
- microcode_fini_cpu(cpu);
+ microcode_fini_cpu(uci, cpu);
/* Should we look for a new ucode here? */
return -EIO;
}

- err = microcode_ops->apply_microcode(cpu);
-
- return err;
+ return microcode_ops->apply_microcode(uci, cpu);
}

static int microcode_update_cpu(const void *buf, size_t size)
{
int err;
unsigned int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ struct ucode_cpu_info *uci = &ucode_cpu_info[cpu];

spin_lock(&microcode_mutex);

/*
- * Check if the system resume is in progress (uci->valid != NULL),
+ * Check if the system resume is in progress (uci->mc.mc_valid != NULL),
* otherwise just request a firmware:
*/
- if ( uci->valid )
- {
- err = microcode_resume_cpu(cpu);
- }
- else
- {
- err = collect_cpu_info(cpu);
- if ( !err && uci->valid )
- err = microcode_ops->cpu_request_microcode(cpu, buf, size);
+ if ( uci->mc.mc_valid ) {
+ err = microcode_resume_cpu(uci, cpu);
+ } else {
+ err = collect_cpu_info(uci, cpu);
+ if ( !err )
+ err = microcode_ops->cpu_request_microcode(uci, cpu, buf, size);
}

spin_unlock(&microcode_mutex);
@@ -153,7 +137,6 @@ static long do_microcode_update(void *_i
error = info->error;
xfree(info);
return error;
-
}

int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
diff -r 0ab2b283e892 -r fb0dc8143932 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c Tue Jan 20 13:22:28 2009 +0000
+++ b/xen/arch/x86/microcode_amd.c Tue Jan 20 13:27:08 2009 +0000
@@ -38,21 +38,16 @@
#define MC_HEADER_SIZE (sizeof(struct microcode_header_amd))
#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
#define DWSIZE (sizeof(uint32_t))
-/* For now we support a fixed ucode total size only */
-#define get_totalsize(mc) \
- ((((struct microcode_amd *)mc)->hdr.mc_patch_data_len * 28) \
- + MC_HEADER_SIZE)

/* serialize access to the physical write */
static DEFINE_SPINLOCK(microcode_update_lock);

struct equiv_cpu_entry *equiv_cpu_table;

-static long install_equiv_cpu_table(const void *, uint32_t, long);
-
static int collect_cpu_info(int cpu, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data[cpu];
+ uint32_t dummy;

memset(csig, 0, sizeof(*csig));

@@ -60,13 +55,10 @@ static int collect_cpu_info(int cpu, str
{
printk(KERN_ERR "microcode: CPU%d not a capable AMD processor\n",
cpu);
- return -1;
- }
-
- asm volatile (
- "movl %1, %%ecx; rdmsr"
- : "=a" (csig->rev)
- : "i" (MSR_AMD_PATCHLEVEL) : "ecx" );
+ return -EINVAL;
+ }
+
+ rdmsr(MSR_AMD_PATCHLEVEL, csig->rev, dummy);

printk(KERN_INFO "microcode: collect_cpu_info: patch_id=0x%x\n",
csig->rev);
@@ -74,29 +66,17 @@ static int collect_cpu_info(int cpu, str
return 0;
}

-static int get_matching_microcode(void *mc, int cpu)
+static int microcode_fits(void *mc, int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
struct microcode_header_amd *mc_header = mc;
- unsigned long total_size = get_totalsize(mc_header);
- void *new_mc;
unsigned int current_cpu_id;
- unsigned int equiv_cpu_id = 0x00;
+ unsigned int equiv_cpu_id = 0x0;
unsigned int i;

/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());

- /* This is a tricky part. We might be called from a write operation
- * to the device file instead of the usual process of firmware
- * loading. This routine needs to be able to distinguish both
- * cases. This is done by checking if there already is a equivalent
- * CPU table installed. If not, we're written through
- * /dev/cpu/microcode.
- * Since we ignore all checks. The error case in which going through
- * firmware loading and that table is not loaded has already been
- * checked earlier.
- */
if ( equiv_cpu_table == NULL )
{
printk(KERN_INFO "microcode: CPU%d microcode update with "
@@ -111,7 +91,7 @@ static int get_matching_microcode(void *
{
if ( current_cpu_id == equiv_cpu_table[i].installed_cpu )
{
- equiv_cpu_id = equiv_cpu_table[i].equiv_cpu;
+ equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
break;
}
}
@@ -119,171 +99,135 @@ static int get_matching_microcode(void *
if ( !equiv_cpu_id )
{
printk(KERN_ERR "microcode: CPU%d cpu_id "
- "not found in equivalent cpu table \n", cpu);
- return 0;
- }
-
- if ( (mc_header->processor_rev_id[0]) != (equiv_cpu_id & 0xff) )
- {
- printk(KERN_INFO
- "microcode: CPU%d patch does not match "
- "(patch is %x, cpu extended is %x) \n",
- cpu, mc_header->processor_rev_id[0],
- (equiv_cpu_id & 0xff));
- return 0;
- }
-
- if ( (mc_header->processor_rev_id[1]) != ((equiv_cpu_id >> 16) & 0xff) )
+ "not found in equivalent cpu table\n", cpu);
+ return -EINVAL;
+ }
+
+ if ( (mc_header->processor_rev_id) != equiv_cpu_id )
{
printk(KERN_INFO "microcode: CPU%d patch does not match "
"(patch is %x, cpu base id is %x) \n",
- cpu, mc_header->processor_rev_id[1],
- ((equiv_cpu_id >> 16) & 0xff));
- return 0;
+ cpu, mc_header->processor_rev_id, equiv_cpu_id);
+ return -EINVAL;
}

if ( mc_header->patch_id <= uci->cpu_sig.rev )
- return 0;
+ return -EINVAL;

printk(KERN_INFO "microcode: CPU%d found a matching microcode "
"update with version 0x%x (current=0x%x)\n",
cpu, mc_header->patch_id, uci->cpu_sig.rev);

- out:
- new_mc = xmalloc_bytes(UCODE_MAX_SIZE);
- if ( new_mc == NULL )
- {
- printk(KERN_ERR "microcode: error, can't allocate memory\n");
- return -ENOMEM;
- }
- memset(new_mc, 0, UCODE_MAX_SIZE);
-
- /* free previous update file */
- xfree(uci->mc.mc_amd);
-
- memcpy(new_mc, mc, total_size);
-
- uci->mc.mc_amd = new_mc;
- return 1;
-}
-
-static int apply_microcode(int cpu)
+out:
+ return 0;
+}
+
+static int apply_microcode(struct ucode_cpu_info *uci, int cpu)
{
unsigned long flags;
- uint32_t eax, edx, rev;
- int cpu_num = raw_smp_processor_id();
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
- uint64_t addr;
+ uint32_t rev, dummy;
+ struct microcode_amd *mc_amd = uci->mc.mc_amd;

/* We should bind the task to the CPU */
- BUG_ON(cpu_num != cpu);
-
- if ( uci->mc.mc_amd == NULL )
+ BUG_ON(raw_smp_processor_id() != cpu);
+
+ if ( mc_amd == NULL )
return -EINVAL;

spin_lock_irqsave(&microcode_update_lock, flags);

- addr = (unsigned long)&uci->mc.mc_amd->hdr.data_code;
- edx = (uint32_t)(addr >> 32);
- eax = (uint32_t)addr;
-
- asm volatile (
- "movl %0, %%ecx; wrmsr" :
- : "i" (MSR_AMD_PATCHLOADER), "a" (eax), "d" (edx) : "ecx" );
+ wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);

/* get patch id after patching */
- asm volatile (
- "movl %1, %%ecx; rdmsr"
- : "=a" (rev)
- : "i" (MSR_AMD_PATCHLEVEL) : "ecx");
+ rdmsr(MSR_AMD_PATCHLEVEL, rev, dummy);

spin_unlock_irqrestore(&microcode_update_lock, flags);

/* check current patch id and patch's id for match */
- if ( rev != uci->mc.mc_amd->hdr.patch_id )
+ if ( rev != mc_amd->hdr.patch_id )
{
printk(KERN_ERR "microcode: CPU%d update from revision "
- "0x%x to 0x%x failed\n", cpu_num,
- uci->mc.mc_amd->hdr.patch_id, rev);
+ "0x%x to 0x%x failed\n", cpu,
+ mc_amd->hdr.patch_id, rev);
return -EIO;
}

printk("microcode: CPU%d updated from revision "
"0x%x to 0x%x \n",
- cpu_num, uci->cpu_sig.rev, uci->mc.mc_amd->hdr.patch_id);
+ cpu, uci->cpu_sig.rev, mc_amd->hdr.patch_id);

uci->cpu_sig.rev = rev;

return 0;
}

-static long get_next_ucode_from_buffer_amd(void **mc, const void *buf,
- unsigned long size, long offset)
+static int get_next_ucode_from_buffer_amd(void *mc, const void *buf,
+ size_t size, unsigned long *offset)
{
struct microcode_header_amd *mc_header;
- unsigned long total_size;
- const uint8_t *buf_pos = buf;
+ size_t total_size;
+ const uint8_t *bufp = buf;
+ unsigned long off;
+
+ off = *offset;

/* No more data */
- if ( offset >= size )
- return 0;
-
- if ( buf_pos[offset] != UCODE_UCODE_TYPE )
+ if ( off >= size )
+ return 1;
+
+ if ( bufp[off] != UCODE_UCODE_TYPE )
{
printk(KERN_ERR "microcode: error! "
"Wrong microcode payload type field\n");
return -EINVAL;
}

- mc_header = (struct microcode_header_amd *)(&buf_pos[offset+8]);
-
- total_size = (unsigned long) (buf_pos[offset+4] +
- (buf_pos[offset+5] << 8));
+ mc_header = (struct microcode_header_amd *)(&bufp[off+8]);
+
+ total_size = (unsigned long) (bufp[off+4] + (bufp[off+5] << 8));

printk(KERN_INFO "microcode: size %lu, total_size %lu, offset %ld\n",
- size, total_size, offset);
-
- if ( (offset + total_size) > size )
+ (unsigned long)size, total_size, off);
+
+ if ( (off + total_size) > size )
{
printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
return -EINVAL;
}

- *mc = xmalloc_bytes(UCODE_MAX_SIZE);
- if ( *mc == NULL )
- {
- printk(KERN_ERR "microcode: error! "
- "Can not allocate memory for microcode patch\n");
- return -ENOMEM;
- }
-
- memset(*mc, 0, UCODE_MAX_SIZE);
- memcpy(*mc, (const void *)(buf + offset + 8), total_size);
-
- return offset + total_size + 8;
-}
-
-static long install_equiv_cpu_table(const void *buf,
- uint32_t size, long offset)
+ memset(mc, 0, UCODE_MAX_SIZE);
+ memcpy(mc, (const void *)(&bufp[off + 8]), total_size);
+
+ *offset = off + total_size + 8;
+
+ return 0;
+}
+
+static int install_equiv_cpu_table(const void *buf, uint32_t size,
+ unsigned long *offset)
{
const uint32_t *buf_pos = buf;
+ unsigned long off;
+
+ off = *offset;
+ *offset = 0;

/* No more data */
- if ( offset >= size )
- return 0;
+ if ( off >= size )
+ return -EINVAL;

if ( buf_pos[1] != UCODE_EQUIV_CPU_TABLE_TYPE )
{
printk(KERN_ERR "microcode: error! "
- "Wrong microcode equivalnet cpu table type field\n");
- return 0;
+ "Wrong microcode equivalent cpu table type field\n");
+ return -EINVAL;
}

if ( size == 0 )
{
printk(KERN_ERR "microcode: error! "
"Wrong microcode equivalnet cpu table length\n");
- return 0;
+ return -EINVAL;
}

equiv_cpu_table = xmalloc_bytes(size);
@@ -291,24 +235,29 @@ static long install_equiv_cpu_table(cons
{
printk(KERN_ERR "microcode: error, can't allocate "
"memory for equiv CPU table\n");
- return 0;
+ return -ENOMEM;
}

memset(equiv_cpu_table, 0, size);
memcpy(equiv_cpu_table, (const void *)&buf_pos[3], size);

- return size + 12; /* add header length */
-}
-
-static int cpu_request_microcode(int cpu, const void *buf, size_t size)
+ *offset = size + 12; /* add header length */
+
+ return 0;
+}
+
+static int cpu_request_microcode(struct ucode_cpu_info *uci,
+ int cpu, const void *buf, size_t size)
{
const uint32_t *buf_pos;
- long offset = 0;
+ unsigned long offset = 0;
int error = 0;
+ int ret;
void *mc;

/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
+ BUG_ON(uci != &ucode_cpu_info[cpu]);

buf_pos = (const uint32_t *)buf;

@@ -319,41 +268,57 @@ static int cpu_request_microcode(int cpu
return -EINVAL;
}

- offset = install_equiv_cpu_table(buf, (uint32_t)(buf_pos[2]), offset);
- if ( !offset )
+ error = install_equiv_cpu_table(buf, (uint32_t)(buf_pos[2]), &offset);
+ if ( error )
{
printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
return -EINVAL;
}

- while ( (offset =
- get_next_ucode_from_buffer_amd(&mc, buf, size, offset)) > 0 )
- {
- error = get_matching_microcode(mc, cpu);
- if ( error < 0 )
+ mc = xmalloc_bytes(UCODE_MAX_SIZE);
+ if ( mc == NULL )
+ {
+ printk(KERN_ERR "microcode: error! "
+ "Can not allocate memory for microcode patch\n");
+ error = -ENOMEM;
+ goto out;
+ }
+
+ /* implicitely validates uci->mc.mc_valid */
+ uci->mc.mc_amd = mc;
+
+ /*
+ * It's possible the data file has multiple matching ucode,
+ * lets keep searching till the latest version
+ */
+ while ( (ret = get_next_ucode_from_buffer_amd(mc, buf, size, &offset)) == 0)
+ {
+ error = microcode_fits(mc, cpu);
+ if (error != 0)
+ continue;
+
+ error = apply_microcode(uci, cpu);
+ if (error == 0)
break;
- /*
- * It's possible the data file has multiple matching ucode,
- * lets keep searching till the latest version
- */
- if ( error == 1 )
- error = apply_microcode(cpu);
+ }
+
+ /* On success keep the microcode patch for
+ * re-apply on resume.
+ */
+ if (error) {
xfree(mc);
- }
- if ( offset > 0 )
- {
- xfree(mc);
- xfree(equiv_cpu_table);
- equiv_cpu_table = NULL;
- }
- if ( offset < 0 )
- error = offset;
+ mc = NULL;
+ }
+ uci->mc.mc_amd = mc;
+
+out:
+ xfree(equiv_cpu_table);
+ equiv_cpu_table = NULL;

return error;
}

static struct microcode_ops microcode_amd_ops = {
- .get_matching_microcode = get_matching_microcode,
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
diff -r 0ab2b283e892 -r fb0dc8143932 xen/arch/x86/microcode_intel.c
--- a/xen/arch/x86/microcode_intel.c Tue Jan 20 13:22:28 2009 +0000
+++ b/xen/arch/x86/microcode_intel.c Tue Jan 20 13:27:08 2009 +0000
@@ -244,12 +244,11 @@ static int get_matching_microcode(void *
return 1;
}

-static int apply_microcode(int cpu)
+static int apply_microcode(struct ucode_cpu_info *uci, int cpu)
{
unsigned long flags;
unsigned int val[2];
int cpu_num = raw_smp_processor_id();
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;

/* We should bind the task to the CPU */
BUG_ON(cpu_num != cpu);
@@ -318,7 +317,8 @@ static long get_next_ucode_from_buffer(v
return offset + total_size;
}

-static int cpu_request_microcode(int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(struct ucode_cpu_info *uci,
+ int cpu, const void *buf, size_t size)
{
long offset = 0;
int error = 0;
@@ -341,7 +341,7 @@ static int cpu_request_microcode(int cpu
*/
if ( error == 1 )
{
- apply_microcode(cpu);
+ apply_microcode(uci, cpu);
error = 0;
}
xfree(mc);
@@ -355,7 +355,6 @@ static int cpu_request_microcode(int cpu
}

static struct microcode_ops microcode_intel_ops = {
- .get_matching_microcode = get_matching_microcode,
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
diff -r 0ab2b283e892 -r fb0dc8143932 xen/include/asm-x86/microcode.h
--- a/xen/include/asm-x86/microcode.h Tue Jan 20 13:22:28 2009 +0000
+++ b/xen/include/asm-x86/microcode.h Tue Jan 20 13:27:08 2009 +0000
@@ -2,12 +2,13 @@
#define ASM_X86__MICROCODE_H

struct cpu_signature;
+struct ucode_cpu_info;

struct microcode_ops {
- int (*get_matching_microcode)(void *mc, int cpu);
- int (*cpu_request_microcode)(int cpu, const void *buf, size_t size);
- int (*collect_cpu_info)(int cpu_num, struct cpu_signature *csig);
- int (*apply_microcode)(int cpu);
+ int (*cpu_request_microcode)(struct ucode_cpu_info *uci,
+ int cpu, const void *buf, size_t size);
+ int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
+ int (*apply_microcode)(struct ucode_cpu_info *uci, int cpu);
};

struct microcode_header_intel {
@@ -43,28 +44,29 @@ struct extended_sigtable {
};

struct equiv_cpu_entry {
- unsigned int installed_cpu;
- unsigned int fixed_errata_mask;
- unsigned int fixed_errata_compare;
- unsigned int equiv_cpu;
-};
+ uint32_t installed_cpu;
+ uint32_t fixed_errata_mask;
+ uint32_t fixed_errata_compare;
+ uint16_t equiv_cpu;
+ uint16_t reserved;
+} __attribute__((packed));

struct microcode_header_amd {
- unsigned int data_code;
- unsigned int patch_id;
- unsigned char mc_patch_data_id[2];
- unsigned char mc_patch_data_len;
- unsigned char init_flag;
- unsigned int mc_patch_data_checksum;
- unsigned int nb_dev_id;
- unsigned int sb_dev_id;
- unsigned char processor_rev_id[2];
- unsigned char nb_rev_id;
- unsigned char sb_rev_id;
- unsigned char bios_api_rev;
- unsigned char reserved1[3];
- unsigned int match_reg[8];
-};
+ uint32_t data_code;
+ uint32_t patch_id;
+ uint8_t mc_patch_data_id[2];
+ uint8_t mc_patch_data_len;
+ uint8_t init_flag;
+ uint32_t mc_patch_data_checksum;
+ uint32_t nb_dev_id;
+ uint32_t sb_dev_id;
+ uint16_t processor_rev_id;
+ uint8_t nb_rev_id;
+ uint8_t sb_rev_id;
+ uint8_t bios_api_rev;
+ uint8_t reserved1[3];
+ uint32_t match_reg[8];
+} __attribute__((packed));

struct microcode_amd {
struct microcode_header_amd hdr;
@@ -79,11 +81,10 @@ struct cpu_signature {

struct ucode_cpu_info {
struct cpu_signature cpu_sig;
- int valid;
union {
struct microcode_intel *mc_intel;
struct microcode_amd *mc_amd;
- void *valid_mc;
+ void *mc_valid;
} mc;
};


_______________________________________________
Xen-changelog mailing list
Xen-changelog@lists.xensource.com
http://lists.xensource.com/xen-changelog