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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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