From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH] x86, amd_ucode: Support multiple container files appended together Date: Fri, 20 Jun 2014 12:18:32 -0500 Message-ID: <53A46CE8.9010106@amd.com> References: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> <53A459F2.1080706@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A459F2.1080706@oracle.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: Boris Ostrovsky Cc: keir@xen.org, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 6/20/2014 10:57 AM, Boris Ostrovsky wrote: > 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. > Yeah. No point. I'll fix it. Thanks for the pointer :) >> + 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. Not really. It is basically checking if we have data at all to parse (like the comment says:/* No more data */) > If we have multiple containers merged I think tot_size is the size of > the merged file, not of an individual container. That's right. > And this makes the first check in install_equiv_cpu_table() not > especially meaningful. > Theoretically- If it so happens that we don't have any patch files and just the container header, then mpbuf->len would be zero and if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size ) will fail in this case- (which gives some meaning to the check being there) 1. If containers are concatenated, but there is no matching patch on the first one 2. So, we fast-fwd to next container file but this one has mpbuf->len=0 Which leads me to another scenario though- 1. If we do have multiple containers and if right patch is not on the first one, but on some subsequent container 2. If the first container has mpbuf->len=0 Then we'd just error out right there. Hmm.. I'll have to re-work the logic then. Come to think, if the first container is corrupted for some reason and returns error (or there's an -ENOMEM), then we are going to return pre-maturely Instead, we should probably (try to) forward to next container and look for patches there as well. Shall fix this logic in a v2.. Thanks, -Aravind