[PATCH] microcode fix

[PATCH] microcode fix

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]


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 <Christoph.Egger@amd.com>

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

-- 
---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

[-- Attachment #2: xen_microcode_fam15.diff --]
[-- Type: text/plain, Size: 3418 bytes --]

diff -r 848049b14ec7 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c	Mon Nov 14 20:15:35 2011 +0000
+++ b/xen/arch/x86/microcode_amd.c	Mon Dec 12 14:36:07 2011 +0100
@@ -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)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] microcode fix

[Keir Fraser]

On 12/12/2011 14:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

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

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

 -- Keir

> The microcode patch for family15h can be larger than 2048 bytes
> and gets silently truncated.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.

Re: [PATCH] microcode fix

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On 12/12/11 15:33, Keir Fraser wrote:
> On 12/12/2011 14:02, "Christoph Egger"<Christoph.Egger@amd.com>  wrote:
>
>>
>> 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 <Christoph.Egger@amd.com>

>
>   -- Keir
>
>> The microcode patch for family15h can be larger than 2048 bytes
>> and gets silently truncated.
>>
>> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com>
>>
>> P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.
>
>
>


-- 
---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

[-- Attachment #2: xen_microcode_fam15.diff --]
[-- Type: text/plain, Size: 8158 bytes --]

diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c	Mon Dec 12 10:47:26 2011 +0100
+++ b/xen/arch/x86/microcode_amd.c	Tue Dec 13 15:16:02 2011 +0100
@@ -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 = {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] microcode fix

[Jan Beulich]

>>> On 13.12.11 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote:
>@@ -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.

>         xfree(mc_amd);
>-        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
>+
>+        mc_amd = xmalloc_bytes(sizeof(*src));

xmalloc(struct microcode_amd)

>+        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

Re: [PATCH] microcode fix

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On 12/13/11 16:03, Jan Beulich wrote:
>>>> On 13.12.11 at 15:22, Christoph Egger<Christoph.Egger@amd.com>  wrote:
>> @@ -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.
>
>>          xfree(mc_amd);
>> -        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
>> +
>> +        mc_amd = xmalloc_bytes(sizeof(*src));
>
> xmalloc(struct microcode_amd)
>
>> +        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 <Christoph.Egger@amd.com>


-- 
---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

[-- Attachment #2: xen_microcode_fam15.diff --]
[-- Type: text/plain, Size: 8429 bytes --]

diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c	Mon Dec 12 10:47:26 2011 +0100
+++ b/xen/arch/x86/microcode_amd.c	Wed Dec 14 11:16:04 2011 +0100
@@ -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 = {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] microcode fix

[Jan Beulich]

>>> On 14.12.11 at 11:17, Christoph Egger <Christoph.Egger@amd.com> wrote:
>+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).

>@@ -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 ...
 
>     /* 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.

>+    if ( mc_amd->mpb == NULL )
>+        return -EINVAL;

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

>-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
>+    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);

... and here.

>+    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'.

>+    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).

>+    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);

Unnecessary cast.

>         printk(KERN_ERR "microcode: error! Wrong "
>-               "microcode patch file magic\n");
>+            "microcode patch file magic\n");

Why are you still mis-adjusting indentation here?

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

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

    mc_amd = xmalloc(struct microcode_amd);

>+    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
>+        buf, bufsize, &offset)) == 0 )

Bad indentation.

>+    ASSERT(src != NULL);

Pointless.

>+        if (mc_amd != NULL) {

While this NULL check is needed, ...

>+            if (mc_amd->equiv_cpu_table)

... this and ...

>+                xfree(mc_amd->equiv_cpu_table);
>+            if (mc_amd->mpb)

... this are pointless.

>+                xfree(mc_amd->mpb);
>+            xfree(mc_amd);
>+        }
>+
>+        mc_amd = xmalloc(struct microcode_amd);

    uci->mc.mc_amd = mc_amd;

>         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.

>+    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

Re: [PATCH] microcode fix

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

On 12/14/11 13:42, Jan Beulich wrote:
>>>> On 14.12.11 at 11:17, Christoph Egger<Christoph.Egger@amd.com>  wrote:
>> +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.

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

Then it is data[]

>> @@ -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 ...
>
>>      /* 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.
>
>> +    if ( mc_amd->mpb == NULL )
>> +        return -EINVAL;
>
> And quite obviously you should preferably use the local variable
> here...
>
>> -    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').

>> +    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.

>> +    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.

>> +    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);
>
> Unnecessary cast.

Right.

>>          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...

>> +    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.

>> +    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
>> +        buf, bufsize,&offset)) == 0 )
>
> Bad indentation.

Fixed.

>> +    ASSERT(src != NULL);
>
> Pointless.

Maybe. Maybe not. Anyway, I removed it.

>> +        if (mc_amd != NULL) {
>
> While this NULL check is needed, ...
>
>> +            if (mc_amd->equiv_cpu_table)
>
> ... this and ...
>
>> +                xfree(mc_amd->equiv_cpu_table);
>> +            if (mc_amd->mpb)
>
> ... this are pointless.
>
>> +                xfree(mc_amd->mpb);
>> +            xfree(mc_amd);
>> +        }
>> +
>> +        mc_amd = xmalloc(struct microcode_amd);
>
>      uci->mc.mc_amd = mc_amd;
>
>>          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.
>
>> +    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 <Christoph.Egger@amd.com>

> Jan
>
>


-- 
---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

[-- Attachment #2: xen_microcode_fam15.diff --]
[-- Type: text/plain, Size: 9316 bytes --]

diff -r f1ab2c2138d8 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c	Mon Dec 12 10:47:26 2011 +0100
+++ b/xen/arch/x86/microcode_amd.c	Wed Dec 14 14:52:04 2011 +0100
@@ -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 = {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] microcode fix

[Jan Beulich]

>>> On 12.12.11 at 15:02, Christoph Egger <Christoph.Egger@amd.com> wrote:
> 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 <Christoph.Egger@amd.com>
> 
> 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)

>@@ -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