Re: [Xen-devel] [PATCH V4] x86, amd_ucode: Support multiple container files appended together

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 7/1/2014 3:23 AM, Jan Beulich wrote:
> > > > 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.
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel