From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86, amd_ucode: Support multiple container files appended together Date: Fri, 20 Jun 2014 11:57:38 -0400 Message-ID: <53A459F2.1080706@oracle.com> References: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Aravind Gopalakrishnan Cc: keir@xen.org, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 06/19/2014 11:14 AM, Aravind Gopalakrishnan wrote: > This patch adds support for parsing through multiple AMD container > binaries concatenated together. It is a feature already present in Linux. > Link to linux patch: > http://lkml.kernel.org/r/1370463236-2115-3-git-send-email-jacob.shin@amd.com > > While at it, define HDR_SIZES and use these for clarity. > > Signed-off-by: Aravind Gopalakrishnan > Reviewed-by: Suravee Suthikulpanit > --- > xen/arch/x86/microcode_amd.c | 120 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 103 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c > index e83f4b6..6478dda 100644 > --- a/xen/arch/x86/microcode_amd.c > +++ b/xen/arch/x86/microcode_amd.c > @@ -29,6 +29,9 @@ > > #define pr_debug(x...) ((void)0) > > +#define CONT_HDR_SIZE 12 > +#define SECTION_HDR_SIZE 8 > + > struct __packed equiv_cpu_entry { > uint32_t installed_cpu; > uint32_t fixed_errata_mask; > @@ -124,6 +127,25 @@ static bool_t verify_patch_size(uint32_t patch_size) > return (patch_size <= max_size); > } > > +static void find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table, > + unsigned int current_cpu_id, > + unsigned int *equiv_cpu_id) > +{ > + unsigned int i; > + > + if ( !equiv_cpu_table ) > + return; > + > + for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ ) > + { > + if ( current_cpu_id == equiv_cpu_table[i].installed_cpu ) > + { > + *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff; > + break; > + } > + } > +} > + > static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) > { > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > @@ -131,21 +153,13 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) > const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table; > unsigned int current_cpu_id; > unsigned int equiv_cpu_id = 0x0; > - unsigned int i; > > /* We should bind the task to the CPU */ > BUG_ON(cpu != raw_smp_processor_id()); > > current_cpu_id = cpuid_eax(0x00000001); > > - for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ ) > - { > - if ( current_cpu_id == equiv_cpu_table[i].installed_cpu ) > - { > - equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff; > - break; > - } > - } > + find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id); > > if ( !equiv_cpu_id ) > return 0; > @@ -236,7 +250,15 @@ static int get_ucode_from_buffer_amd( > mpbuf = (const struct mpbhdr *)&bufp[off]; > if ( mpbuf->type != UCODE_UCODE_TYPE ) > { > - printk(KERN_ERR "microcode: Wrong microcode payload type field\n"); > + /* > + * We will hit this condition only if an update has succeeded > + * but there is still another container file being parsed. > + * In that case, there is no need of this ERR message to be > + * printed. > + */ > + if ( mpbuf->type != UCODE_MAGIC ) > + printk(KERN_ERR "microcode: Wrong microcode payload type field\n"); > + > return -EINVAL; > } > > @@ -260,7 +282,7 @@ static int get_ucode_from_buffer_amd( > } > memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len); > > - *offset = off + mpbuf->len + 8; > + *offset = off + mpbuf->len + SECTION_HDR_SIZE; > > pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", > raw_smp_processor_id(), bufsize, mpbuf->len, off, > @@ -272,13 +294,16 @@ static int get_ucode_from_buffer_amd( > > static int install_equiv_cpu_table( > struct microcode_amd *mc_amd, > - const uint32_t *buf, > - size_t *offset) > + const uint8_t *data, > + size_t *tot_size, Why is this a pointer? This routine does not modify the value. > + size_t *curr_offset) > { > + size_t off = *curr_offset; > + uint32_t *buf = (uint32_t *) &data[off]; > const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1]; > > /* No more data */ > - if ( mpbuf->len + 12 >= *offset ) > + if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size ) > return -EINVAL; > > if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) > @@ -303,7 +328,32 @@ static int install_equiv_cpu_table( > memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len); > mc_amd->equiv_cpu_table_size = mpbuf->len; > > - *offset = mpbuf->len + 12; /* add header length */ > + *curr_offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ > + > + return 0; > +} > + > +static int container_fast_forward(const uint8_t *data, size_t size_left, size_t *offset) > +{ > + size_t size, off; > + uint32_t *header; > + > + while ( size_left ) > + { > + off = *offset; > + header = (uint32_t *) &data[off]; > + if ( header[0] == UCODE_MAGIC && > + header[1] == UCODE_EQUIV_CPU_TABLE_TYPE ) > + break; > + > + size = header[1] + SECTION_HDR_SIZE; > + *offset += size; > + size_left -= size; > + > + } > + > + if ( !size_left ) > + return -EINVAL; > > return 0; > } > @@ -311,14 +361,19 @@ static int install_equiv_cpu_table( > static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > { > struct microcode_amd *mc_amd, *mc_old; > - size_t offset = bufsize; > + size_t offset = 0; > + size_t tot_size = bufsize; > size_t last_offset, applied_offset = 0; > int error = 0, save_error = 1; > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > + unsigned int current_cpu_id; > + unsigned int equiv_cpu_id = 0x0; > > /* We should bind the task to the CPU */ > BUG_ON(cpu != raw_smp_processor_id()); > > + current_cpu_id = cpuid_eax(0x00000001); > + > if ( *(const uint32_t *)buf != UCODE_MAGIC ) > { > printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); > @@ -334,7 +389,30 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > goto out; > } > > - error = install_equiv_cpu_table(mc_amd, buf, &offset); > + /* > + * Multiple container file support: > + * 1. check if this container file has equiv_cpu_id match > + * 2. If not, fast-fwd to next container file > + */ > + while ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size, > + &offset)) == 0 ) install_equiv_cpu_table() checks whether container size is larger than the buffer size. If we have multiple containers merged I think tot_size is the size of the merged file, not of an individual container. And this makes the first check in install_equiv_cpu_table() not especially meaningful. -boris > + { > + find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id, > + &equiv_cpu_id); > + if ( equiv_cpu_id ) > + break; > + > + error = container_fast_forward(buf, (bufsize - offset), &offset); > + if ( error ) > + { > + printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n" > + "microcode: Failed to update patch level. " > + "Current lvl:%#x\n", cpu, uci->cpu_sig.rev); > + error = -EINVAL; > + goto out; > + } > + } > + > if ( error ) > { > xfree(mc_amd); > @@ -379,8 +457,16 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > save_error = get_ucode_from_buffer_amd( > mc_amd, buf, bufsize, &applied_offset); > > + /* > + * If there happens to be multiple container files and if patch > + * update succeeded on first container itself, a stale error val > + * is returned from get_ucode_from_buffer_amd. So, force > + * error = 0 here as we have already succeeded in the update. > + */ > if ( save_error ) > error = save_error; > + else > + error = 0; > } > > if ( save_error )