From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH V4] x86, amd_ucode: Support multiple container files appended together Date: Wed, 2 Jul 2014 11:48:16 -0500 Message-ID: <53B437D0.9090609@amd.com> References: <1404147156-29868-1-git-send-email-aravind.gopalakrishnan@amd.com> <53B28C05020000780001EDD8@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: <53B28C05020000780001EDD8@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: boris.ostrovsky@oracle.com, keir@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 7/1/2014 3:23 AM, Jan Beulich wrote: >>>> On 30.06.14 at 18:52, wrote: >> @@ -272,14 +293,12 @@ 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) >> { >> - const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1]; >> + const struct mpbhdr *mpbuf = (const struct mpbhdr *)(data + *offset + 4); > There's still a pointless cast here. Fixed. >> @@ -303,7 +322,35 @@ 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 */ >> + return 0; >> +} >> + >> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset) >> +{ >> + size_t size; >> + const uint32_t *header; >> + >> + while ( size_left ) >> + { >> + if ( size_left < SECTION_HDR_SIZE ) >> + return -EINVAL; >> + >> + header = (const uint32_t *) (data + *offset); > And you again introduce another one here. Fixed. >> + } >> + >> + if ( !size_left ) >> + return -EINVAL; > And btw, this check is kind of redundant with the size_left < > SECTION_HDR_SIZE one inside the loop: If you make the loop > header "for ( ; ; )", you won't need this extra if(). Fixed. >> @@ -379,12 +464,48 @@ 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 ) >> + /* >> + * 1. Given a situation where multiple containers exist and correct >> + * patch lives on 1st container >> + * 2. We match equivalent ids using find_equiv_cpu_id() from the >> + * earlier while() (On this case, matches on first container >> + * file and we break) >> + * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >> + * buf, bufsize,&offset)) == 0 ) >> + * 4. Find correct patch using microcode_fits() and apply the patch >> + * (Assume: apply_microcode() is successful) >> + * 5. The while() loop from (3) continues to parse the binary as >> + * there is a subsequent container file, but... >> + * 6. returns -EINVAL as the condition >> + * if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to false. >> + * 7. A correct patch can only be on one container and not on any >> + * subsequent ones. (Refer docs for more info) Therefore, we >> + * don't have to parse a subsequent container. So, we can abort >> + * the process here and discard this error value as we succeeded >> + * already in patch update. >> + * 8. Since we need to return 0 for success, force error = 0. >> + */ > Much better. One thing I'd like to avoid though is special casing the 1st > container in the description. Just refer to "some container other than > the last one" instead. Okay. >> + if ( offset < bufsize ) >> + error = 0; >> + else if ( save_error ) >> error = save_error; > And with the comment having become precise, it is now more obvious > what's odd here: Flushing the error to zero should imo be done > conditionally upon the next thing following being a UCODE_MAGIC > rather than the much more generic "offset < bufsize". Hmm. Yep. I have been experimenting with this- if ( offset + SECTION_HDR_SIZE <= bufsize && *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) { printk("DEBUG: hit another container. breaking..\n"); break; } within the while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,&offset)) == 0 ) loop and it seems to work fine. i.e, We can actually eliminate the need to workaround the corner cases as we pre-check (if that's the right word) for the occurrence of a subsequent container before the if ( mpbuf->type != UCODE_UCODE_TYPE ) check fails and returns -EINVAL. This ensures that 'error' variable retains a success value (0) I have tested this bit with both the edge cases and they work fine. As a consequence, I'll re-word the comments. Thanks, -Aravind.