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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.14 at 22:25, <aravind.gopalakrishnan@xxxxxxx> wrote:
>  - Adding Jan, Boris reviewed-by tags; Dropping Suravee's since logic 
> changed..

This is an absolute no-go: Neither did I (nor - afaict - Boris) offer
these, nor would a heavily changed patch warrant retaining any
previous offerings. Both of us did review the previous iteration,
but whether your changes actually allow for these tags can't be
known until both of us had a chance to look at them.

> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -29,6 +29,17 @@
>  
>  #define pr_debug(x...) ((void)0)
>  
> +#define CONT_HDR_SIZE           12
> +#define SECTION_HDR_SIZE        8
> +#define AMD_UCODE_APPLY_SUCCESS 0
> +/*
> + * Currently, there are two AMD container files on
> + * 
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode
> + * So, this is max number of container binaries that can be appended
> + * together and packaged along with initrd.
> + */
> +#define AMD_MAX_CONT_APPEND     2

So as soon as a 3rd one appears the code will need to be changed
again? Pretty bad approach I would say.

> @@ -124,30 +135,43 @@ static bool_t verify_patch_size(uint32_t patch_size)
>      return (patch_size <= max_size);
>  }
>  
> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry 
> *equiv_cpu_table,
> +                              unsigned int current_cpu_id,
> +                              unsigned int *equiv_cpu_id)
>  {
> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> -    const struct microcode_header_amd *mc_header = mc_amd->mpb;
> -    const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
> -    unsigned int current_cpu_id;
> -    unsigned int equiv_cpu_id = 0x0;
>      unsigned int i;
> +    unsigned int found = 0;

This is your return value - why is it "unsigned int" when the function
returns bool_t? But the variable is pointless anyway, since ...

>  
> -    /* We should bind the task to the CPU */
> -    BUG_ON(cpu != raw_smp_processor_id());
> -
> -    current_cpu_id = cpuid_eax(0x00000001);
> +    if ( !equiv_cpu_table )
> +        return 0;
>  
>      for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ )
>      {
>          if ( current_cpu_id == equiv_cpu_table[i].installed_cpu )
>          {
> -            equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> +            found = 1;
>              break;
>          }
>      }
>  
> -    if ( !equiv_cpu_id )
> +    return found;
> +}

... rather than break-ing from the loop you could just "return 1"
there.

> @@ -211,7 +235,7 @@ static int apply_microcode(int cpu)
>  
>      uci->cpu_sig.rev = rev;
>  
> -    return 0;
> +    return AMD_UCODE_APPLY_SUCCESS;

Definitely not: This function _has to_ return zero (due to its use in
the initializer of microcode_amd_ops), i.e. someone altering the
definition of AMD_UCODE_APPLY_SUCCESS (e.g. making it a
boolean) would break things.

> @@ -236,7 +260,15 @@ static int get_ucode_from_buffer_amd(
>      mpbuf = (const struct mpbhdr *)&bufp[off];
>      if ( mpbuf->type != UCODE_UCODE_TYPE )
>      {
> -        printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
> +        /*
> +         * We will hit this condition only if an update has succeeded
> +         * but there is still another container file being parsed.
> +         * In that case, there is no need of this ERR message to be
> +         * printed.
> +         */
> +        if ( mpbuf->type != UCODE_MAGIC )
> +            printk(KERN_ERR "microcode: Wrong microcode payload type 
> field\n");

The more often I look at it, the less convinced am I that this is okay:
UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
that the field matches up with the next block signature. Either don't
abuse the field, or make clear via comment why it is done this way.

> @@ -272,14 +304,13 @@ static int get_ucode_from_buffer_amd(
>  
>  static int install_equiv_cpu_table(
>      struct microcode_amd *mc_amd,
> -    const uint32_t *buf,
> -    size_t *offset)
> +    const void *data,
> +    size_t *curr_offset)

Is there any strong reason to rename "offset" to "curr_offset", ...

>  {
> +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
>      const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>  
> -    /* No more data */
> -    if ( mpbuf->len + 12 >= *offset )
> -        return -EINVAL;

Iirc you and Boris agreed that this check is pointless _here_. But I
doubt it can be removed without replacement elsewhere.

> +    *curr_offset += mpbuf->len + CONT_HDR_SIZE;      /* add header length */
>  
>      if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>      {
> @@ -303,7 +334,30 @@ 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 */

... and to move this assignment up?

> +    return 0;
> +}
> +
> +static int container_fast_forward(const void *data, size_t size_left, size_t 
> *offset)
> +{
> +    size_t size;
> +    uint32_t *header;
> +
> +    while ( size_left )
> +    {
> +        header = (uint32_t *) (data + *offset);

Pointless (and wrong, as it discards the const qualifier) cast.

> +        if ( header[0] == UCODE_MAGIC &&
> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )

Are these two valid if size_left < SECTION_HDR_SIZE?

> +            break;
> +
> +        size = header[1] + SECTION_HDR_SIZE;
> +        *offset += size;
> +
> +        if ( size_left >= size )
> +            size_left -= size;
> +    }

And if size_left < size we're continuing the loop (perhaps indefinitely)?

> @@ -334,7 +393,33 @@ 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 ( containers_processed <= AMD_MAX_CONT_APPEND )

Leaving aside the point made above regarding this limit, you set the
limit to 2 but then allow 3 iterations?

> +    {
> +        containers_processed++;
> +        error = install_equiv_cpu_table(mc_amd, buf, &offset);
> +
> +        if ( !error )
> +        {
> +             if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> +                                    &equiv_cpu_id) )
> +                break;

I think I said this on earlier occasions already: Such if-s should be
folded into one.

> +        }
> +
> +        error = container_fast_forward(buf, (bufsize - offset), &offset);

Pointless parentheses around the second argument expression.

> @@ -379,7 +464,15 @@ 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
> +         * val is returned from get_ucode_from_buffer_amd. Since we already
> +         * succeeded in patch application, return as success itself
> +         */
> +        if ( (containers_processed < AMD_MAX_CONT_APPEND) && error )
> +            error = AMD_UCODE_APPLY_SUCCESS;
> +        else if ( save_error )
>              error = save_error;

This still seems wrong to me: For one, you don't ever try to apply
a patch from another container if one succeeded (since the looking
for containers sits in a separate loop prior to the one looking into
the container, and there's no path back to the top of the first loop).
Further I again don't follow the conditions you use:
containers_processed >= AMD_MAX_CONT_APPEND should have
got dealt with (if at all possible) much earlier (after the first loop),
and the "&& error" looks superfluous as long as
AMD_UCODE_APPLY_SUCCESS == 0. Which finally gets us back to
the same point made above - this function again has to return zero
on success, no matter what AMD_UCODE_APPLY_SUCCESS is
defined to.

Jan

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