Skip to contentSkip to conversation

microcode fix

PATCH In review Christoph Egger 3 participants 8 messages Dec 12, 2011 – Dec 14, 2011

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 microcode fix

Remove hardcoded maximum size a microcode patch can have. This is dynamic now.

The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated.

Signed-off-by Christoph Egger <[email protected]>

P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/microcode_amd.c +16 -16
diff -r 848049b14ec7 xen/arch/x86/microcode_amd.c
@@ -27,18 +27,10 @@
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
-#define pr_debug(x...) ((void)0)
-
 #define UCODE_MAGIC                0x00414d44
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-#define UCODE_MAX_SIZE          (2048)
-#define DEFAULT_UCODE_DATASIZE  (896)
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_amd))
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
-#define DWSIZE                  (sizeof(uint32_t))
-
 /* serialize access to the physical write */
 static DEFINE_SPINLOCK(microcode_update_lock);
 

@@ -99,7 +91,7 @@ static int microcode_fits(void *mc, int 
     }
 
     if ( mc_header->patch_id <= uci->cpu_sig.rev )
-        return -EINVAL;
+        return 1;
 
     printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
            "update with version 0x%x (current=0x%x)\n",

@@ -147,8 +139,8 @@ static int apply_microcode(int cpu)
     return 0;
 }
 
-static int get_next_ucode_from_buffer_amd(void *mc, const void *buf,
-                                         size_t size, unsigned long *offset)
+static int get_next_ucode_from_buffer_amd(void *mc, size_t *mc_size,
+    const void *buf, size_t size, unsigned long *offset)
 {
     size_t total_size;
     const uint8_t *bufp = buf;

@@ -178,7 +170,12 @@ static int get_next_ucode_from_buffer_am
         return -EINVAL;
     }
 
-    memset(mc, 0, UCODE_MAX_SIZE);
+    if (*mc_size < total_size) {
+        xfree(mc);
+        mc = xmalloc_bytes(total_size);
+        *mc_size = total_size;
+    }
+    memset(mc, 0, *mc_size);
     memcpy(mc, (const void *)(&bufp[off + 8]), total_size);
 
     *offset = off + total_size + 8;

@@ -231,11 +228,12 @@ static int install_equiv_cpu_table(const
 static int cpu_request_microcode(int cpu, const void *buf, size_t size)
 {
     const uint32_t *buf_pos;
-    unsigned long offset = 0;
+    size_t offset = 0;
     int error = 0;
     int ret;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     void *mc;
+    size_t mc_size;
 
     /* We should bind the task to the CPU */
     BUG_ON(cpu != raw_smp_processor_id());

@@ -245,7 +243,7 @@ static int cpu_request_microcode(int cpu
     if ( buf_pos[0] != UCODE_MAGIC )
     {
         printk(KERN_ERR "microcode: error! Wrong "
-               "microcode patch file magic\n");
+            "microcode patch file magic\n");
         return -EINVAL;
     }
 

@@ -256,7 +254,8 @@ static int cpu_request_microcode(int cpu
         return -EINVAL;
     }
 
-    mc = xmalloc_bytes(UCODE_MAX_SIZE);
+    mc_size = buf_pos[offset + 1]; /* Size of 1st microcode patch in bytes */
+    mc = xmalloc_bytes(size);
     if ( mc == NULL )
     {
         printk(KERN_ERR "microcode: error! "

@@ -272,7 +271,8 @@ static int cpu_request_microcode(int cpu
      * 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)
+    while ( (ret = get_next_ucode_from_buffer_amd(mc, &mc_size,
+        buf, size, &offset)) == 0)
     {
         error = microcode_fits(mc, cpu);
         if (error <= 0)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 3 lines · Christoph Egger

Remove hardcoded maximum size a microcode patch can have. This is dynamic now.

The patch doesn't apply to xen-unstable tip.

-- Keir

Quoted 6 lines

The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated.

Signed-off-by Christoph Egger <[email protected]>

P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.

author
#2 lore raw
Quoted 7 lines · Keir Fraser
Quoted 3 lines · Christoph Egger

Remove hardcoded maximum size a microcode patch can have. This is dynamic now.

The patch doesn't apply to xen-unstable tip.

Ported to xen-unstable.

Signed-off-by Christoph Egger <[email protected]>
Quoted 12 lines

-- Keir

Quoted 6 lines

The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated.

Signed-off-by Christoph Egger<[email protected]>

P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/microcode_amd.c +74 -38
diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
@@ -26,8 +26,6 @@
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
-#define pr_debug(x...) ((void)0)
-
 struct equiv_cpu_entry {
     uint32_t installed_cpu;
     uint32_t fixed_errata_mask;

@@ -57,14 +55,17 @@ struct microcode_header_amd {
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-#define UCODE_MAX_SIZE          (2048)
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_amd))
+struct microcode_amd {
+    void *mpb;
+    size_t mpb_size;
+    struct equiv_cpu_entry *equiv_cpu_table;
+    size_t equiv_cpu_table_size;
+};
 
-struct microcode_amd {
-    struct microcode_header_amd hdr;
-    unsigned int mpb[(UCODE_MAX_SIZE - MC_HEADER_SIZE) / 4];
-    unsigned int equiv_cpu_table_size;
-    struct equiv_cpu_entry equiv_cpu_table[];
+struct mpbhdr {
+    uint32_t type;
+    uint32_t len;
+    const uint8_t data;
 };
 
 /* serialize access to the physical write */

@@ -94,7 +95,7 @@ static int collect_cpu_info(int cpu, str
 static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    const struct microcode_header_amd *mc_header = &mc_amd->hdr;
+    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 = 0x0;

@@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     uint32_t rev;
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    struct microcode_header_amd *hdr = mc_amd->mpb;
 
     /* We should bind the task to the CPU */
     BUG_ON(raw_smp_processor_id() != cpu);
 
     if ( mc_amd == NULL )
         return -EINVAL;
+    if ( mc_amd->mpb == NULL )
+        return -EINVAL;
 
     spin_lock_irqsave(&microcode_update_lock, flags);
 
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);

@@ -158,57 +162,60 @@ static int apply_microcode(int cpu)
     spin_unlock_irqrestore(&microcode_update_lock, flags);
 
     /* check current patch id and patch's id for match */
-    if ( rev != mc_amd->hdr.patch_id )
+    if ( rev != hdr->patch_id )
     {
         printk(KERN_ERR "microcode: CPU%d update from revision "
                "0x%x to 0x%x failed\n", cpu,
-               mc_amd->hdr.patch_id, rev);
+               hdr->patch_id, rev);
         return -EIO;
     }
 
     printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, uci->cpu_sig.rev, mc_amd->hdr.patch_id);
+           cpu, uci->cpu_sig.rev, hdr->patch_id);
 
     uci->cpu_sig.rev = rev;
 
     return 0;
 }
 
-static int get_next_ucode_from_buffer_amd(void *mc, const void *buf,
-                                         size_t size, unsigned long *offset)
+static int get_next_ucode_from_buffer_amd(struct microcode_amd *mc_amd,
+    const void *buf, size_t bufsize, unsigned long *offset)
 {
-    size_t total_size;
     const uint8_t *bufp = buf;
     unsigned long off;
+    const struct mpbhdr *mpbuf;
 
     off = *offset;
 
     /* No more data */
-    if ( off >= size )
+    if ( off >= bufsize )
         return 1;
 
-    if ( bufp[off] != UCODE_UCODE_TYPE )
+    mpbuf = (const struct mpbhdr *)&bufp[off];
+    if ( mpbuf->type != UCODE_UCODE_TYPE )
     {
         printk(KERN_ERR "microcode: error! "
                "Wrong microcode payload type field\n");
         return -EINVAL;
     }
 
-    total_size = (unsigned long) (bufp[off+4] + (bufp[off+5] << 8));
+    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
+           (unsigned long)bufsize, mpbuf->len, off);
 
-    printk(KERN_DEBUG "microcode: size %lu, total_size %lu, offset %ld\n",
-           (unsigned long)size, total_size, off);
-
-    if ( (off + total_size) > size )
+    if ( (off + mpbuf->len) > bufsize )
     {
         printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
         return -EINVAL;
     }
 
-    memset(mc, 0, UCODE_MAX_SIZE);
-    memcpy(mc, (const void *)(&bufp[off + 8]), total_size);
+    if (mc_amd->mpb_size < mpbuf->len) {
+        xfree(mc_amd->mpb);
+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+        mc_amd->mpb_size = mpbuf->len;
+    }
+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);
 
-    *offset = off + total_size + 8;
+    *offset = off + mpbuf->len + 8;
 
     return 0;
 }

@@ -234,10 +241,18 @@ static int install_equiv_cpu_table(
     if ( size == 0 )
     {
         printk(KERN_ERR "microcode: error! "
-               "Wrong microcode equivalnet cpu table length\n");
+               "Wrong microcode equivalent cpu table length\n");
         return -EINVAL;
     }
 
+    mc_amd->equiv_cpu_table = xmalloc_bytes(size);
+    if ( !mc_amd->equiv_cpu_table )
+    {
+        printk(KERN_ERR "microcode: error! "
+               "Can not allocate memory for equivalent cpu table\n");
+        return -ENOMEM;
+    }
+
     memcpy(mc_amd->equiv_cpu_table, &buf_pos[3], size);
     mc_amd->equiv_cpu_table_size = size;
 

@@ -246,11 +261,11 @@ static int install_equiv_cpu_table(
     return 0;
 }
 
-static int cpu_request_microcode(int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
 {
     const uint32_t *buf_pos;
     struct microcode_amd *mc_amd, *mc_old;
-    unsigned long offset = size;
+    size_t offset = bufsize;
     int error = 0;
     int ret;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

@@ -263,11 +278,11 @@ static int cpu_request_microcode(int cpu
     if ( buf_pos[0] != UCODE_MAGIC )
     {
         printk(KERN_ERR "microcode: error! Wrong "
-               "microcode patch file magic\n");
+            "microcode patch file magic\n");
         return -EINVAL;
     }
 
-    mc_amd = xmalloc_bytes(sizeof(*mc_amd) + buf_pos[2]);
+    mc_amd = xmalloc_bytes(sizeof(*mc_amd));
     if ( !mc_amd )
     {
         printk(KERN_ERR "microcode: error! "

@@ -291,8 +306,10 @@ static int cpu_request_microcode(int cpu
      * 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_amd->hdr, buf, size,
-                                                  &offset)) == 0 )
+    mc_amd->mpb = NULL;
+    mc_amd->mpb_size = 0;
+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
+        buf, bufsize, &offset)) == 0 )
     {
         error = microcode_fits(mc_amd, cpu);
         if (error <= 0)

@@ -335,17 +352,36 @@ static int microcode_resume_match(int cp
 
     if ( src != mc_amd )
     {
+        xfree(mc_amd->equiv_cpu_table);
+        xfree(mc_amd->mpb);
         xfree(mc_amd);
-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
+
+        mc_amd = xmalloc_bytes(sizeof(*src));
+        if ( !mc_amd )
+            goto err0;
+        mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
+        if ( !mc_amd->equiv_cpu_table )
+            goto err1;
+        mc_amd->mpb = xmalloc_bytes(src->mpb_size);
+        if ( !mc_amd->mpb )
+            goto err2;
+
+        mc_amd->equiv_cpu_table_size = src->equiv_cpu_table_size;
+        mc_amd->mpb_size = src->mpb_size;
         uci->mc.mc_amd = mc_amd;
-        if ( !mc_amd )
-            return -ENOMEM;
-        memcpy(mc_amd, src, UCODE_MAX_SIZE);
+        memcpy(mc_amd->mpb, src->mpb, src->mpb_size);
         memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
                src->equiv_cpu_table_size);
     }
 
     return 1;
+
+err2:
+    xfree(mc_amd->equiv_cpu_table);
+err1:
+    xfree(mc_amd);
+err0:
+    return -ENOMEM;
 }
 
 static const struct microcode_ops microcode_amd_ops = {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 356

Quoted 7 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/microcode_amd.c +2 -0
@@ -335,17 +352,36 @@ static int microcode_resume_match(int cp
 
    if ( src != mc_amd )
    {
+        xfree(mc_amd->equiv_cpu_table);
+        xfree(mc_amd->mpb);

mc_amd can be NULL here.

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 359

Quoted 4 lines
xen/arch/x86/microcode_amd.c +2 -1
        xfree(mc_amd);
-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
+
+        mc_amd = xmalloc_bytes(sizeof(*src));

xmalloc(struct microcode_amd)

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 361

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+        if ( !mc_amd )
+            goto err0;

The error handling is broken here. You need to clear out uci->mc.mc_amd (i.e. move up the respective assignment).

Jan

author
#4 lore raw
Quoted 15 lines · Jan Beulich

Review static int microcode_resume_match(int cp · line 356

Quoted 7 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
(quoted diff) +2 -0
@@ -335,17 +352,36 @@ static int microcode_resume_match(int cp
 
     if ( src != mc_amd )
     {
+        xfree(mc_amd->equiv_cpu_table);
+        xfree(mc_amd->mpb);

mc_amd can be NULL here.

Review static int microcode_resume_match(int cp · line 359

Quoted 4 lines
(quoted diff) +2 -1
         xfree(mc_amd);
-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
+
+        mc_amd = xmalloc_bytes(sizeof(*src));
Quoted 7 lines

xmalloc(struct microcode_amd)

Review static int microcode_resume_match(int cp · line 361

Quoted 2 lines
(quoted diff) +2 -0
+        if ( !mc_amd )
+            goto err0;

The error handling is broken here. You need to clear out uci->mc.mc_amd (i.e. move up the respective assignment).

Thanks for review.
New version attached.

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/microcode_amd.c +81 -39
diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
@@ -26,8 +26,6 @@
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
-#define pr_debug(x...) ((void)0)
-
 struct equiv_cpu_entry {
     uint32_t installed_cpu;
     uint32_t fixed_errata_mask;

@@ -57,14 +55,17 @@ struct microcode_header_amd {
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-#define UCODE_MAX_SIZE          (2048)
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_amd))
+struct microcode_amd {
+    void *mpb;
+    size_t mpb_size;
+    struct equiv_cpu_entry *equiv_cpu_table;
+    size_t equiv_cpu_table_size;
+};
 
-struct microcode_amd {
-    struct microcode_header_amd hdr;
-    unsigned int mpb[(UCODE_MAX_SIZE - MC_HEADER_SIZE) / 4];
-    unsigned int equiv_cpu_table_size;
-    struct equiv_cpu_entry equiv_cpu_table[];
+struct mpbhdr {
+    uint32_t type;
+    uint32_t len;
+    const uint8_t data;
 };
 
 /* serialize access to the physical write */

@@ -94,7 +95,7 @@ static int collect_cpu_info(int cpu, str
 static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    const struct microcode_header_amd *mc_header = &mc_amd->hdr;
+    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 = 0x0;

@@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     uint32_t rev;
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    struct microcode_header_amd *hdr = mc_amd->mpb;
 
     /* We should bind the task to the CPU */
     BUG_ON(raw_smp_processor_id() != cpu);
 
     if ( mc_amd == NULL )
         return -EINVAL;
+    if ( mc_amd->mpb == NULL )
+        return -EINVAL;
 
     spin_lock_irqsave(&microcode_update_lock, flags);
 
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);

@@ -158,57 +162,60 @@ static int apply_microcode(int cpu)
     spin_unlock_irqrestore(&microcode_update_lock, flags);
 
     /* check current patch id and patch's id for match */
-    if ( rev != mc_amd->hdr.patch_id )
+    if ( rev != hdr->patch_id )
     {
         printk(KERN_ERR "microcode: CPU%d update from revision "
                "0x%x to 0x%x failed\n", cpu,
-               mc_amd->hdr.patch_id, rev);
+               hdr->patch_id, rev);
         return -EIO;
     }
 
     printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, uci->cpu_sig.rev, mc_amd->hdr.patch_id);
+           cpu, uci->cpu_sig.rev, hdr->patch_id);
 
     uci->cpu_sig.rev = rev;
 
     return 0;
 }
 
-static int get_next_ucode_from_buffer_amd(void *mc, const void *buf,
-                                         size_t size, unsigned long *offset)
+static int get_next_ucode_from_buffer_amd(struct microcode_amd *mc_amd,
+    const void *buf, size_t bufsize, unsigned long *offset)
 {
-    size_t total_size;
     const uint8_t *bufp = buf;
     unsigned long off;
+    const struct mpbhdr *mpbuf;
 
     off = *offset;
 
     /* No more data */
-    if ( off >= size )
+    if ( off >= bufsize )
         return 1;
 
-    if ( bufp[off] != UCODE_UCODE_TYPE )
+    mpbuf = (const struct mpbhdr *)&bufp[off];
+    if ( mpbuf->type != UCODE_UCODE_TYPE )
     {
         printk(KERN_ERR "microcode: error! "
                "Wrong microcode payload type field\n");
         return -EINVAL;
     }
 
-    total_size = (unsigned long) (bufp[off+4] + (bufp[off+5] << 8));
+    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
+           (unsigned long)bufsize, mpbuf->len, off);
 
-    printk(KERN_DEBUG "microcode: size %lu, total_size %lu, offset %ld\n",
-           (unsigned long)size, total_size, off);
-
-    if ( (off + total_size) > size )
+    if ( (off + mpbuf->len) > bufsize )
     {
         printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
         return -EINVAL;
     }
 
-    memset(mc, 0, UCODE_MAX_SIZE);
-    memcpy(mc, (const void *)(&bufp[off + 8]), total_size);
+    if (mc_amd->mpb_size < mpbuf->len) {
+        xfree(mc_amd->mpb);
+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+        mc_amd->mpb_size = mpbuf->len;
+    }
+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);
 
-    *offset = off + total_size + 8;
+    *offset = off + mpbuf->len + 8;
 
     return 0;
 }

@@ -234,10 +241,18 @@ static int install_equiv_cpu_table(
     if ( size == 0 )
     {
         printk(KERN_ERR "microcode: error! "
-               "Wrong microcode equivalnet cpu table length\n");
+               "Wrong microcode equivalent cpu table length\n");
         return -EINVAL;
     }
 
+    mc_amd->equiv_cpu_table = xmalloc_bytes(size);
+    if ( !mc_amd->equiv_cpu_table )
+    {
+        printk(KERN_ERR "microcode: error! "
+               "Can not allocate memory for equivalent cpu table\n");
+        return -ENOMEM;
+    }
+
     memcpy(mc_amd->equiv_cpu_table, &buf_pos[3], size);
     mc_amd->equiv_cpu_table_size = size;
 

@@ -246,11 +261,11 @@ static int install_equiv_cpu_table(
     return 0;
 }
 
-static int cpu_request_microcode(int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
 {
     const uint32_t *buf_pos;
     struct microcode_amd *mc_amd, *mc_old;
-    unsigned long offset = size;
+    size_t offset = bufsize;
     int error = 0;
     int ret;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

@@ -263,11 +278,11 @@ static int cpu_request_microcode(int cpu
     if ( buf_pos[0] != UCODE_MAGIC )
     {
         printk(KERN_ERR "microcode: error! Wrong "
-               "microcode patch file magic\n");
+            "microcode patch file magic\n");
         return -EINVAL;
     }
 
-    mc_amd = xmalloc_bytes(sizeof(*mc_amd) + buf_pos[2]);
+    mc_amd = xmalloc_bytes(sizeof(*mc_amd));
     if ( !mc_amd )
     {
         printk(KERN_ERR "microcode: error! "

@@ -291,8 +306,10 @@ static int cpu_request_microcode(int cpu
      * 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_amd->hdr, buf, size,
-                                                  &offset)) == 0 )
+    mc_amd->mpb = NULL;
+    mc_amd->mpb_size = 0;
+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
+        buf, bufsize, &offset)) == 0 )
     {
         error = microcode_fits(mc_amd, cpu);
         if (error <= 0)

@@ -333,19 +350,44 @@ static int microcode_resume_match(int cp
     if ( res <= 0 )
         return res;
 
+    ASSERT(src != NULL);
     if ( src != mc_amd )
     {
-        xfree(mc_amd);
-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
-        uci->mc.mc_amd = mc_amd;
+        if (mc_amd != NULL) {
+            if (mc_amd->equiv_cpu_table)
+                xfree(mc_amd->equiv_cpu_table);
+            if (mc_amd->mpb)
+                xfree(mc_amd->mpb);
+            xfree(mc_amd);
+        }
+
+        mc_amd = xmalloc(struct microcode_amd);
         if ( !mc_amd )
-            return -ENOMEM;
-        memcpy(mc_amd, src, UCODE_MAX_SIZE);
+            goto err0;
+        mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
+        if ( !mc_amd->equiv_cpu_table )
+            goto err1;
+        mc_amd->mpb = xmalloc_bytes(src->mpb_size);
+        if ( !mc_amd->mpb )
+            goto err2;
+
+        mc_amd->equiv_cpu_table_size = src->equiv_cpu_table_size;
+        mc_amd->mpb_size = src->mpb_size;
+        memcpy(mc_amd->mpb, src->mpb, src->mpb_size);
         memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
                src->equiv_cpu_table_size);
     }
 
+    uci->mc.mc_amd = mc_amd;
     return 1;
+
+err2:
+    xfree(mc_amd->equiv_cpu_table);
+err1:
+    xfree(mc_amd);
+err0:
+    uci->mc.mc_amd = mc_amd = NULL;
+    return -ENOMEM;
 }
 
 static const struct microcode_ops microcode_amd_ops = {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#5 lore raw

xen/arch/x86/microcode_amd.c struct microcode_header_amd { · line 68

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/microcode_amd.c +4 -0
+struct mpbhdr {
+    uint32_t type;
+    uint32_t len;
+    const uint8_t data;

What's this? If anything, this should be data[] (and no need for const).

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 145

Quoted 5 lines
xen/arch/x86/microcode_amd.c +1 -0
@@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
    uint32_t rev;
    struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    struct microcode_header_amd *hdr = mc_amd->mpb;

Using mc_amd here, but ...

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 151

Quoted 5 lines
xen/arch/x86/microcode_amd.c +0 -0
    /* We should bind the task to the CPU */
    BUG_ON(raw_smp_processor_id() != cpu);
 
    if ( mc_amd == NULL )
        return -EINVAL;

... the NULL check is only here.

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 153

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+    if ( mc_amd->mpb == NULL )
+        return -EINVAL;

And quite obviously you should preferably use the local variable here...

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 157

Quoted 2 lines
xen/arch/x86/microcode_amd.c +1 -1
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);

... and here.

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 203

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
+           (unsigned long)bufsize, mpbuf->len, off);

The cast is pointless; to avoid it, use %zu as format string or declare the parameter as 'unsigned long'.

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 214

Quoted 4 lines
xen/arch/x86/microcode_amd.c +4 -0
+    if (mc_amd->mpb_size < mpbuf->len) {
+        xfree(mc_amd->mpb);
+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+        mc_amd->mpb_size = mpbuf->len;

NULL check missing here (and in such event the clearing of ->mpb_size).

xen/arch/x86/microcode_amd.c static int apply_microcode(int cpu) · line 216

Quoted 1 line
xen/arch/x86/microcode_amd.c +1 -0
+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);

Unnecessary cast.

xen/arch/x86/microcode_amd.c static int cpu_request_microcode(int cpu · line 281

Quoted 3 lines
xen/arch/x86/microcode_amd.c +1 -1
        printk(KERN_ERR "microcode: error! Wrong "
-               "microcode patch file magic\n");
+            "microcode patch file magic\n");

Why are you still mis-adjusting indentation here?

Quoted 1 line

+ mc_amd = xmalloc_bytes(sizeof(*mc_amd));

As said during the first review round - this ought to be

    mc_amd = xmalloc(struct microcode_amd);

xen/arch/x86/microcode_amd.c static int cpu_request_microcode(int cpu · line 312

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
+        buf, bufsize, &offset)) == 0 )

Bad indentation.

Quoted 1 line

+ ASSERT(src != NULL);

Pointless.

Quoted 1 line

+ if (mc_amd != NULL) {

While this NULL check is needed, ...

Quoted 1 line

+ if (mc_amd->equiv_cpu_table)

... this and ...

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 359

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+                xfree(mc_amd->equiv_cpu_table);
+            if (mc_amd->mpb)

... this are pointless.

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 364

Quoted 5 lines
xen/arch/x86/microcode_amd.c +5 -0
+                xfree(mc_amd->mpb);
+            xfree(mc_amd);
+        }
+
+        mc_amd = xmalloc(struct microcode_amd);

    uci->mc.mc_amd = mc_amd;
Quoted 1 line
        if ( !mc_amd )

(as was the case in the original code). No need to do this once in the success path and once in the error one.

xen/arch/x86/microcode_amd.c static int microcode_resume_match(int cp · line 390

Quoted 2 lines
xen/arch/x86/microcode_amd.c +2 -0
+    uci->mc.mc_amd = mc_amd = NULL;
+    return -ENOMEM;

Even if it was necessary to keep it this way, initializing a local variable immediately before returning is bogus (and calling for a compiler warning and hence a build failure due to -Werror sooner or later).

Jan

author
#6 lore raw
Quoted 7 lines · Jan Beulich

Review

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
(quoted diff) +4 -0
+struct mpbhdr {
+    uint32_t type;
+    uint32_t len;
+    const uint8_t data;

What's this?

This little structure eliminates the uses buf_pos[x] throughout the code which is much easier to read and understand.

Quoted 1 line

If anything, this should be data[] (and no need for const).

Then it is data[]

Quoted 19 lines

Review static int apply_microcode(int cpu) · line 145

Quoted 5 lines
(quoted diff) +1 -0
@@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
     struct ucode_cpu_info *uci =&per_cpu(ucode_cpu_info, cpu);
     uint32_t rev;
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    struct microcode_header_amd *hdr = mc_amd->mpb;

Using mc_amd here, but ...

Review static int apply_microcode(int cpu) · line 150

Quoted 5 lines
(quoted diff) +0 -0
     /* We should bind the task to the CPU */
     BUG_ON(raw_smp_processor_id() != cpu);
 
     if ( mc_amd == NULL )
         return -EINVAL;

... the NULL check is only here.

Review static int apply_microcode(int cpu) · line 152

Quoted 2 lines
(quoted diff) +2 -0
+    if ( mc_amd->mpb == NULL )
+        return -EINVAL;
Quoted 7 lines

And quite obviously you should preferably use the local variable here...

Review static int apply_microcode(int cpu) · line 153

Quoted 2 lines
(quoted diff) +1 -1
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);

... and here.

done (assuming you mean 'hdr').

Quoted 5 lines

Review static int apply_microcode(int cpu) · line 155

Quoted 2 lines
(quoted diff) +2 -0
+    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
+           (unsigned long)bufsize, mpbuf->len, off);

The cast is pointless; to avoid it, use %zu as format string or declare the parameter as 'unsigned long'.

done.

Quoted 7 lines

Review static int apply_microcode(int cpu) · line 159

Quoted 4 lines
(quoted diff) +4 -0
+    if (mc_amd->mpb_size<  mpbuf->len) {
+        xfree(mc_amd->mpb);
+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+        mc_amd->mpb_size = mpbuf->len;

NULL check missing here (and in such event the clearing of ->mpb_size).

done.

Quoted 3 lines

Review static int apply_microcode(int cpu) · line 160

Quoted 1 line
(quoted diff) +1 -0
+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);

Unnecessary cast.

Right.

Quoted 5 lines

Review static int apply_microcode(int cpu)

Quoted 3 lines
(quoted diff) +1 -1
         printk(KERN_ERR "microcode: error! Wrong "
-               "microcode patch file magic\n");
+            "microcode patch file magic\n");

Why are you still mis-adjusting indentation here?

oh...

Quoted 5 lines
Quoted 1 line

+ mc_amd = xmalloc_bytes(sizeof(*mc_amd));

As said during the first review round - this ought to be

     mc_amd = xmalloc(struct microcode_amd);

sorry, didn't realize that it was not only relevant to the shown line of code.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
+        buf, bufsize,&offset)) == 0 )

Bad indentation.

Fixed.

Quoted 3 lines
Quoted 1 line

+ ASSERT(src != NULL);

Pointless.

Maybe. Maybe not. Anyway, I removed it.

Quoted 26 lines
Quoted 1 line

+ if (mc_amd != NULL) {

While this NULL check is needed, ...

Quoted 1 line

+ if (mc_amd->equiv_cpu_table)

... this and ...

Review

Quoted 2 lines
(quoted diff) +2 -0
+                xfree(mc_amd->equiv_cpu_table);
+            if (mc_amd->mpb)

... this are pointless.

Review

Quoted 5 lines
(quoted diff) +5 -0
+                xfree(mc_amd->mpb);
+            xfree(mc_amd);
+        }
+
+        mc_amd = xmalloc(struct microcode_amd);

     uci->mc.mc_amd = mc_amd;
Quoted 1 line
         if ( !mc_amd )

(as was the case in the original code). No need to do this once in the success path and once in the error one.

Quoted 6 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+    uci->mc.mc_amd = mc_amd = NULL;
+    return -ENOMEM;

Even if it was necessary to keep it this way, initializing a local variable immediately before returning is bogus (and calling for a compiler warning and hence a build failure due to -Werror sooner or later).

Fixed.

New version attached.

Signed-off-by Christoph Egger <[email protected]>
Quoted 3 lines

Jan

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/microcode_amd.c +93 -48
diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
@@ -26,8 +26,6 @@
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
-#define pr_debug(x...) ((void)0)
-
 struct equiv_cpu_entry {
     uint32_t installed_cpu;
     uint32_t fixed_errata_mask;

@@ -57,14 +55,17 @@ struct microcode_header_amd {
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-#define UCODE_MAX_SIZE          (2048)
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_amd))
+struct microcode_amd {
+    void *mpb;
+    size_t mpb_size;
+    struct equiv_cpu_entry *equiv_cpu_table;
+    size_t equiv_cpu_table_size;
+};
 
-struct microcode_amd {
-    struct microcode_header_amd hdr;
-    unsigned int mpb[(UCODE_MAX_SIZE - MC_HEADER_SIZE) / 4];
-    unsigned int equiv_cpu_table_size;
-    struct equiv_cpu_entry equiv_cpu_table[];
+struct mpbhdr {
+    uint32_t type;
+    uint32_t len;
+    uint8_t data[];
 };
 
 /* serialize access to the physical write */

@@ -94,7 +95,7 @@ static int collect_cpu_info(int cpu, str
 static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    const struct microcode_header_amd *mc_header = &mc_amd->hdr;
+    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 = 0x0;

@@ -141,6 +142,7 @@ static int apply_microcode(int cpu)
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     uint32_t rev;
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    struct microcode_header_amd *hdr;
 
     /* We should bind the task to the CPU */
     BUG_ON(raw_smp_processor_id() != cpu);

@@ -148,9 +150,13 @@ static int apply_microcode(int cpu)
     if ( mc_amd == NULL )
         return -EINVAL;
 
+    hdr = mc_amd->mpb;
+    if ( hdr == NULL )
+        return -EINVAL;
+
     spin_lock_irqsave(&microcode_update_lock, flags);
 
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);

@@ -158,99 +164,115 @@ static int apply_microcode(int cpu)
     spin_unlock_irqrestore(&microcode_update_lock, flags);
 
     /* check current patch id and patch's id for match */
-    if ( rev != mc_amd->hdr.patch_id )
+    if ( rev != hdr->patch_id )
     {
         printk(KERN_ERR "microcode: CPU%d update from revision "
                "0x%x to 0x%x failed\n", cpu,
-               mc_amd->hdr.patch_id, rev);
+               hdr->patch_id, rev);
         return -EIO;
     }
 
     printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, uci->cpu_sig.rev, mc_amd->hdr.patch_id);
+           cpu, uci->cpu_sig.rev, hdr->patch_id);
 
     uci->cpu_sig.rev = rev;
 
     return 0;
 }
 
-static int get_next_ucode_from_buffer_amd(void *mc, const void *buf,
-                                         size_t size, unsigned long *offset)
+static int get_next_ucode_from_buffer_amd(struct microcode_amd *mc_amd,
+    const void *buf, size_t bufsize, unsigned long *offset)
 {
-    size_t total_size;
     const uint8_t *bufp = buf;
     unsigned long off;
+    const struct mpbhdr *mpbuf;
 
     off = *offset;
 
     /* No more data */
-    if ( off >= size )
+    if ( off >= bufsize )
         return 1;
 
-    if ( bufp[off] != UCODE_UCODE_TYPE )
+    mpbuf = (const struct mpbhdr *)&bufp[off];
+    if ( mpbuf->type != UCODE_UCODE_TYPE )
     {
         printk(KERN_ERR "microcode: error! "
                "Wrong microcode payload type field\n");
         return -EINVAL;
     }
 
-    total_size = (unsigned long) (bufp[off+4] + (bufp[off+5] << 8));
+    printk(KERN_DEBUG "microcode: size %zu, block size %u, offset %ld\n",
+           bufsize, mpbuf->len, off);
 
-    printk(KERN_DEBUG "microcode: size %lu, total_size %lu, offset %ld\n",
-           (unsigned long)size, total_size, off);
-
-    if ( (off + total_size) > size )
+    if ( (off + mpbuf->len) > bufsize )
     {
         printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
         return -EINVAL;
     }
 
-    memset(mc, 0, UCODE_MAX_SIZE);
-    memcpy(mc, (const void *)(&bufp[off + 8]), total_size);
+    if (mc_amd->mpb_size < mpbuf->len) {
+        if (mc_amd->mpb) {
+            xfree(mc_amd->mpb);
+            mc_amd->mpb_size = 0;
+        }
+        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+        if (mc_amd->mpb == NULL)
+            return -ENOMEM;
+        mc_amd->mpb_size = mpbuf->len;
+    }
+    memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
 
-    *offset = off + total_size + 8;
+    *offset = off + mpbuf->len + 8;
 
     return 0;
 }
 
 static int install_equiv_cpu_table(
     struct microcode_amd *mc_amd,
-    const uint32_t *buf_pos,
+    const uint32_t *buf,
     unsigned long *offset)
 {
-    uint32_t size = buf_pos[2];
+    const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
 
     /* No more data */
-    if ( size + 12 >= *offset )
+    if ( mpbuf->len + 12 >= *offset )
         return -EINVAL;
 
-    if ( buf_pos[1] != UCODE_EQUIV_CPU_TABLE_TYPE )
+    if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
     {
         printk(KERN_ERR "microcode: error! "
                "Wrong microcode equivalent cpu table type field\n");
         return -EINVAL;
     }
 
-    if ( size == 0 )
+    if ( mpbuf->len == 0 )
     {
         printk(KERN_ERR "microcode: error! "
-               "Wrong microcode equivalnet cpu table length\n");
+               "Wrong microcode equivalent cpu table length\n");
         return -EINVAL;
     }
 
-    memcpy(mc_amd->equiv_cpu_table, &buf_pos[3], size);
-    mc_amd->equiv_cpu_table_size = size;
+    mc_amd->equiv_cpu_table = xmalloc_bytes(mpbuf->len);
+    if ( !mc_amd->equiv_cpu_table )
+    {
+        printk(KERN_ERR "microcode: error! "
+               "Can not allocate memory for equivalent cpu table\n");
+        return -ENOMEM;
+    }
 
-    *offset = size + 12;	/* add header length */
+    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 */
 
     return 0;
 }
 
-static int cpu_request_microcode(int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
 {
-    const uint32_t *buf_pos;
+    const uint32_t *magic;
     struct microcode_amd *mc_amd, *mc_old;
-    unsigned long offset = size;
+    size_t offset = bufsize;
     int error = 0;
     int ret;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

@@ -258,16 +280,15 @@ static int cpu_request_microcode(int cpu
     /* We should bind the task to the CPU */
     BUG_ON(cpu != raw_smp_processor_id());
 
-    buf_pos = (const uint32_t *)buf;
-
-    if ( buf_pos[0] != UCODE_MAGIC )
+    magic = (const uint32_t *)buf;
+    if ( *magic != UCODE_MAGIC )
     {
         printk(KERN_ERR "microcode: error! Wrong "
                "microcode patch file magic\n");
         return -EINVAL;
     }
 
-    mc_amd = xmalloc_bytes(sizeof(*mc_amd) + buf_pos[2]);
+    mc_amd = xmalloc(struct microcode_amd);
     if ( !mc_amd )
     {
         printk(KERN_ERR "microcode: error! "

@@ -291,7 +312,9 @@ static int cpu_request_microcode(int cpu
      * 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_amd->hdr, buf, size,
+    mc_amd->mpb = NULL;
+    mc_amd->mpb_size = 0;
+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize,
                                                   &offset)) == 0 )
     {
         error = microcode_fits(mc_amd, cpu);

@@ -335,17 +358,39 @@ static int microcode_resume_match(int cp
 
     if ( src != mc_amd )
     {
-        xfree(mc_amd);
-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
+        if (mc_amd != NULL) {
+            xfree(mc_amd->equiv_cpu_table);
+            xfree(mc_amd->mpb);
+            xfree(mc_amd);
+        }
+
+        mc_amd = xmalloc(struct microcode_amd);
         uci->mc.mc_amd = mc_amd;
         if ( !mc_amd )
-            return -ENOMEM;
-        memcpy(mc_amd, src, UCODE_MAX_SIZE);
+            goto err0;
+        mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
+        if ( !mc_amd->equiv_cpu_table )
+            goto err1;
+        mc_amd->mpb = xmalloc_bytes(src->mpb_size);
+        if ( !mc_amd->mpb )
+            goto err2;
+
+        mc_amd->equiv_cpu_table_size = src->equiv_cpu_table_size;
+        mc_amd->mpb_size = src->mpb_size;
+        memcpy(mc_amd->mpb, src->mpb, src->mpb_size);
         memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
                src->equiv_cpu_table_size);
     }
 
     return 1;
+
+err2:
+    xfree(mc_amd->equiv_cpu_table);
+err1:
+    xfree(mc_amd);
+err0:
+    uci->mc.mc_amd = NULL;
+    return -ENOMEM;
 }
 
 static const struct microcode_ops microcode_amd_ops = {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#7 lore raw
In reply to Christoph Egger
Quoted 10 lines
Quoted 1 line
Quoted 1 line · Christoph Egger

Remove hardcoded maximum size a microcode patch can have. This is dynamic now.

The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated.

Signed-off-by Christoph Egger <[email protected]>

P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.

Please sync up with latest -unstable. At least this hunk (which also is completely unrelated to anything mentioned in the description)

xen/arch/x86/microcode_amd.c static int microcode_fits(void *mc, int · line 97

Quoted 9 lines
xen/arch/x86/microcode_amd.c +1 -1
@@ -99,7 +91,7 @@ static int microcode_fits(void *mc, int 
    }
 
    if ( mc_header->patch_id <= uci->cpu_sig.rev )
-        return -EINVAL;
+        return 1;
 
    printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
           "update with version 0x%x (current=0x%x)\n",

would not apply anymore (and is wrong - zero is being and should be returned in this case).

There's also a stray hunk (mis?)adjusting only formatting, which should be removed.

Jan