From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH V3] x86, amd_ucode: Support multiple container files appended together Date: Fri, 27 Jun 2014 12:07:35 -0500 Message-ID: <53ADA4D7.3090801@amd.com> References: <1403724849-14360-1-git-send-email-aravind.gopalakrishnan@amd.com> <53AD689D020000780001DE4E@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: <53AD689D020000780001DE4E@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 , xen-devel@lists.xen.org, boris.ostrovsky@oracle.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 6/27/2014 5:50 AM, Jan Beulich wrote: >>>> On 25.06.14 at 21:34, 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 >> >> Other changes introduced: >> - Define HDR_SIZE's explicitly for code clarity. >> >> Changes in V3 >> - Change approach that used AMD_MAX_CONT_APPEND to handle edge case >> - No need of a 'found' variable >> - return 0 for success and not AMD_UCODE_APPLY_SUCCESS as someone >> could just as easily break things by redefining it as bool >> - No need of 'header' in container_fast_forward >> - Handle more corner cases in container_fast_forward >> - Fix code style issues > Can you please get used to put this revision log after the first --- > marker? It doesn't belong in ti actual commit message. Okay. >> @@ -272,14 +293,13 @@ static int get_ucode_from_buffer_amd( >> >> static int install_equiv_cpu_table( >> struct microcode_amd *mc_amd, >> - const uint32_t *buf, >> + const void *data, >> size_t *offset) >> { >> + uint32_t *buf = (uint32_t *) (data + *offset); > I know I complained about this or a similar pointless cast for the > previous version. Yes, I thought I'll handle it as a separate cleanup patch as it was not directly related to the subject of the patch. But it's a small change, so I'll do this and include it in the commit message as well.. >> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset) >> +{ >> + size_t size; >> + >> + while ( size_left ) >> + { >> + if ( size_left < SECTION_HDR_SIZE ) >> + return -EINVAL; >> + >> + if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC && >> + *(const uint32_t *)(data + *offset + 4) == >> + UCODE_EQUIV_CPU_TABLE_TYPE ) >> + break; >> + >> + size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE; > Now these three casts+derefs surely warrant a "const uint32_t *" > variable, making the resulting code quite a bit easier to read. Okay. Will just use the bits in V2 with a const qualifier >> + if ( size_left < size ) >> + return -EINVAL; >> + >> + size_left -= size; >> + *offset += size; > Endless loop if size ends up being zero? (If you do such verifications > at all, you should perhaps be as strict as possible, Sorry about that (Again). Will fix. > i.e. if size is > required to be a multiple of 4 [which I think it is], you should also > check that.) I don't think that's a rule per-se. #define F16H_MPB_MAX_SIZE 3458 Which is indivisible by 4. >> @@ -334,7 +384,40 @@ 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 ( offset < bufsize ) >> + { >> + error = install_equiv_cpu_table(mc_amd, buf, &offset); >> + >> + if ( !error && >> + find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id, >> + &equiv_cpu_id) ) >> + break; >> + >> + /* >> + * Could happen as we advance 'offset' early >> + * in install_equiv_cpu_table >> + */ >> + if ( offset > bufsize ) >> + { >> + printk(KERN_ERR "microcode: Microcode buffer overrun\n"); >> + return -EINVAL; >> + } >> + >> + 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); >> + goto out; >> + } > So the immediately preceding error path used just "return" - why > are they different (and I'm not asking about the printk())? Ok, Shall use one or the other.. (Probably the goto as I see that's the favored one in this function) >> @@ -379,12 +462,35 @@ 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 ( save_error ) >> + /* >> + * If there happens to be multiple container files and if patch >> + * update succeeded on an earlier container itself, a stale error > Do you perhaps mean bogus instead of stale? Yes. Wrong word. Will fix. >> + * val is returned from get_ucode_from_buffer_amd. Since we already >> + * succeeded in patch application, force error = 0 >> + */ >> + if ( offset < bufsize ) >> + error = 0; >> + else if ( save_error ) >> error = save_error; > And still my question stands: Since you don't look at further containers > if you found a match and successfully applied the updated, why is this > change needed (or is the comment saying the wrong thing)? Maybe the comment needs to be more verbose(?) Yes, we found a match and yes, we applied the patch successfully. But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,&offset)) == 0 ) is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) and return -EINVAL which is assigned to the variable 'error' (Assuming ofc that there is a second container there which we don't need to parse because we have already succeeded in patch application) This is what I wanted to convey from "astale bogus error val is returned from get_ucode_from_buffer_amd." But, we need to return 0 on success; which is why this change is needed here.. > >> } >> >> if ( save_error ) >> { >> + /* >> + * By the time 'microcode_init' runs, we have already updated the >> + * patch level on all (currently) running cpus. >> + * But, get_ucode_from_buffer_amd will return -EINVAL as >> + * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case: >> + * Multiple containers are present && update succeeded with the >> + * first container file itself. >> + * >> + * Only this time, there is no 'applied_offset' as well. >> + * So, 'save_error' = 1. But error = -EINVAL. >> + * Hence, this check is necessary to return 0 for success. >> + */ >> + if ( (error != save_error) && (offset < bufsize) ) >> + error = 0; > Same for this change/comment. > Hmm.. I'm having trouble trying to re-word this comment then.. Given the situation where - we have already applied the patch update after 'microcode_presmp_init' and 'microcode_resume_cpu'; | v Now 'microcode_init' runs and calls into 'cpu_request_microcode'; | v We use 1st while loop to find_equiv_cpu_id() and match it with the container | v For this particular case, we assume it's a match on the 1st container; so break | v Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,&offset)) == 0 ) | v At some point, it will find the correct patch; but this time there is no need to update | v The behavior is now similar to what I have described above. i.e while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,&offset)) == 0 ) is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) and return -EINVAL which is assigned to the variable 'error' | v But, now (as stated in the comment..) * Only this time, there is no 'applied_offset' as well. + * So, 'save_error' = 1. But error = -EINVAL. | v And since we need to return 0 for success, this change is needed here. Thanks, -Aravind