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 11:06:49 -0500 Message-ID: <53A45C19.2000900@amd.com> References: <1403190841-22892-1-git-send-email-aravind.gopalakrishnan@amd.com> <53A408B8020000780001BE1A@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53A408B8020000780001BE1A@mail.emea.novell.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: Jan Beulich Cc: keir@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 6/20/2014 3:11 AM, Jan Beulich wrote: >>>> On 19.06.14 at 17:14, wrote: >> @@ -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; >> + } >> + } >> +} > Please have this function return a found/not-found indication > rather than having the callers rely on *equiv_cpu_id transitioning > from zero to no-zero. That way you can also drop the respective > initializers in the callers. Ok, Will modify this. >> @@ -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, >> + size_t *curr_offset) >> { >> + size_t off = *curr_offset; >> + uint32_t *buf = (uint32_t *) &data[off]; > I realize there are may casts there in this file. But please don't add > more of them without a strict need (replacing existing ones would > also be much appreciated). In the case here, if "data" was "const > void *" you'd get away with just "data + off". Furthermore there's > no reason for having the one-time use variable "off" here - just > use *offset directly. Ok, I'll change this per your suggestion. About replacing the existing casts, I'll generate a separate patch for it.. >> + if ( header[0] == UCODE_MAGIC && >> + header[1] == UCODE_EQUIV_CPU_TABLE_TYPE ) >> + break; >> + >> + size = header[1] + SECTION_HDR_SIZE; >> + *offset += size; >> + size_left -= size; > Don't you think you should first check for size_left >= size here? Yep. Thanks for pointing this out. I'll add it. >> @@ -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 ) >> + { >> + 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; > Either you use the error value returned from container_fast_forward(), > or you make that function simply return a bool_t. Hmm, Yep. I'll fix this. >> @@ -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; > First of all I don't think this change is logically a part of $subject (as > it also alters behavior for single container files). With that the > question then is why the change is really correct. And if it was really > correct, you could just drop the if() instead. > > The change is needed (as stated in the comments) because we end up returning err val (EINVAL) like so- DEBUG: cpu_request_microcode : no save_err;error=-22 even though update succeeded- (XEN) microcode: CPU0 found a matching microcode update with version 0x600063d (current=0x6000626) (XEN) microcode: CPU0 updated from revision 0x6000626 to 0x600063d That said, I also understand your point that we can just drop the if(). So, I'll change this as per your suggestion. The fact that we have to make this change as a _consequence_ of adding multiple container file support, might explain why it was done later right? I can make this a separate patch as well. Let me know what you think. Thanks, -Aravind.