From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH] microcode fix Date: Wed, 14 Dec 2011 14:53:03 +0100 Message-ID: <4EE8AA3F.9040704@amd.com> References: <4EE75FB4.30609@amd.com> <4EE7776E0200007800067623@nat28.tlf.novell.com> <4EE877A8.7080706@amd.com> <4EE8A7DE0200007800067B17@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------080706070403020908090207" Return-path: In-Reply-To: <4EE8A7DE0200007800067B17@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "xen-devel@lists.xensource.com" , Keir Fraser List-Id: xen-devel@lists.xenproject.org --------------080706070403020908090207 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 12/14/11 13:42, Jan Beulich wrote: >>>> On 14.12.11 at 11:17, Christoph Egger 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 > 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 --------------080706070403020908090207 Content-Type: text/plain; name="xen_microcode_fam15.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xen_microcode_fam15.diff" Content-Description: xen_microcode_fam15.diff 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 #include -#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(µcode_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(µcode_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 = { --------------080706070403020908090207 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------080706070403020908090207--