[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4] x86, amd_ucode: Support multiple container files appended together
>>> On 30.06.14 at 18:52, <aravind.gopalakrishnan@xxxxxxx> 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. > @@ -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. > + > + if ( header[0] == UCODE_MAGIC && > + header[1] == UCODE_EQUIV_CPU_TABLE_TYPE ) > + break; > + > + size = header[1] + SECTION_HDR_SIZE; > + if ( size == 0 || size_left < size ) > + return -EINVAL; > + > + size_left -= size; > + *offset += size; I know I pointed at only the size == 0 case leading to an infinite loop, but I assumed you wouldn't limit the adjustment to just that extreme case. Is e.g. size == 1 valid here? Likely not, i.e. you will rather want to use a "size < some_minimum_value" check above. > + } > + > + 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(). > @@ -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. > + 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". Furthermore I wonder whether you wouldn't rather want to clear save_error in that case (simplifying - if not eliminating the need for - the change below). Jan > } > > if ( save_error ) > { > + /* > + * 1. Given a situation where multiple containers exist and correct > + * patch lives on 1st container (AND) we have already applied the > + * patch update after microcode_presmp_init(), > microcode_resume_cpu() > + * 2. microcode_init() runs and calls into cpu_request_microcode() > + * 3. Read points 2 to 7 from previous comment. > + * 4. Except that, this time there is no 'applied_offset' > + * => 'save_error' = 1. But error = -EINVAL > + * 5. Since we need to return 0 for success, force it here. > + */ > + if ( (error != save_error) && (offset < bufsize) ) > + error = 0; > + > xfree(mc_amd); > uci->mc.mc_amd = mc_old; > } _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |