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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.14 at 21:34, <aravind.gopalakrishnan@xxxxxxx> wrote:
> This patch adds support for parsing through multiple AMD container
> binaries concatenated together. It is a feature already present in Linux.
> Link to linux patch:
> http://lkml.kernel.org/r/1370463236-2115-3-git-send-email-jacob.shin@xxxxxxx 
> 
> Other changes introduced:
>  - Define HDR_SIZE's explicitly for code clarity.
> 
> Changes in V3
>  - Change approach that used AMD_MAX_CONT_APPEND to handle edge case
>  - No need of a 'found' variable
>  - return 0 for success and not AMD_UCODE_APPLY_SUCCESS as someone
>    could just as easily break things by redefining it as bool
>  - No need of 'header' in container_fast_forward
>  - Handle more corner cases in container_fast_forward
>  - Fix code style issues

Can you please get used to put this revision log after the first ---
marker? It doesn't belong in ti actual commit message.

> @@ -272,14 +293,13 @@ 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)
>  {
> +    uint32_t *buf = (uint32_t *) (data + *offset);

I know I complained about this or a similar pointless cast for the
previous version.

> +static int container_fast_forward(const void *data, size_t size_left, size_t 
> *offset)
> +{
> +    size_t size;
> +
> +    while ( size_left )
> +    {
> +        if ( size_left < SECTION_HDR_SIZE )
> +            return -EINVAL;
> +
> +        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
> +             *(const uint32_t *)(data + *offset + 4) ==
> +                                 UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;

Now these three casts+derefs surely warrant a "const uint32_t *"
variable, making the resulting code quite a bit easier to read.

> +        if ( size_left < size )
> +            return -EINVAL;
> +
> +        size_left -= size;
> +        *offset += size;

Endless loop if size ends up being zero? (If you do such verifications
at all, you should perhaps be as strict as possible, i.e. if size is
required to be a multiple of 4 [which I think it is], you should also
check that.)

> @@ -334,7 +384,40 @@ 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 ( offset < bufsize )
> +    {
> +        error = install_equiv_cpu_table(mc_amd, buf, &offset);
> +
> +        if ( !error &&
> +             find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> +                               &equiv_cpu_id) )
> +                break;
> +
> +        /*
> +         * Could happen as we advance 'offset' early
> +         * in install_equiv_cpu_table
> +         */
> +        if ( offset > bufsize )
> +        {
> +            printk(KERN_ERR "microcode: Microcode buffer overrun\n");
> +            return -EINVAL;
> +        }
> +
> +        error = container_fast_forward(buf, bufsize - offset, &offset);
> +        if ( error )
> +        {
> +            printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container 
> file\n"
> +                   "microcode: Failed to update patch level. "
> +                   "Current lvl:%#x\n", cpu, uci->cpu_sig.rev);
> +            goto out;
> +        }

So the immediately preceding error path used just "return" - why
are they different (and I'm not asking about the printk())?

> @@ -379,12 +462,35 @@ 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

Do you perhaps mean bogus instead of stale?

> +         * val is returned from get_ucode_from_buffer_amd. Since we already
> +         * succeeded in patch application, force error = 0
> +         */
> +        if ( offset < bufsize )
> +            error = 0;
> +        else if ( save_error )
>              error = save_error;

And still my question stands: Since you don't look at further containers
if you found a match and successfully applied the updated, why is this
change needed (or is the comment saying the wrong thing)?

>      }
>  
>      if ( save_error )
>      {
> +        /*
> +         * By the time 'microcode_init' runs, we have already updated the
> +         * patch level on all (currently) running cpus.
> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
> +         * Multiple containers are present && update succeeded with the
> +         * first container file itself.
> +         *
> +         * Only this time, there is no 'applied_offset' as well.
> +         * So, 'save_error' = 1. But error = -EINVAL.
> +         * Hence, this check is necessary to return 0 for success.
> +         */
> +        if ( (error != save_error) && (offset < bufsize) )
> +            error = 0;

Same for this change/comment.

Jan

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