Re: [PATCH] x86, amd_ucode: Support multiple container files appended together

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/19/2014 11:14 AM, Aravind Gopalakrishnan 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@amd.com
>
> While at it, define HDR_SIZES and use these for clarity.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>   xen/arch/x86/microcode_amd.c |  120 ++++++++++++++++++++++++++++++++++++------
>   1 file changed, 103 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index e83f4b6..6478dda 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -29,6 +29,9 @@
>   
>   #define pr_debug(x...) ((void)0)
>   
> +#define CONT_HDR_SIZE       12
> +#define SECTION_HDR_SIZE    8
> +
>   struct __packed equiv_cpu_entry {
>       uint32_t installed_cpu;
>       uint32_t fixed_errata_mask;
> @@ -124,6 +127,25 @@ static bool_t verify_patch_size(uint32_t patch_size)
>       return (patch_size <= max_size);
>   }
>   
> +static void find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
> +                              unsigned int current_cpu_id,
> +                              unsigned int *equiv_cpu_id)
> +{
> +    unsigned int i;
> +
> +    if ( !equiv_cpu_table )
> +        return;
> +
> +    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;
> +            break;
> +        }
> +    }
> +}
> +
>   static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>   {
>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> @@ -131,21 +153,13 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>       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;
>   
>       /* We should bind the task to the CPU */
>       BUG_ON(cpu != raw_smp_processor_id());
>   
>       current_cpu_id = cpuid_eax(0x00000001);
>   
> -    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;
> -            break;
> -        }
> -    }
> +    find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id);
>   
>       if ( !equiv_cpu_id )
>           return 0;
> @@ -236,7 +250,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");
> +
>           return -EINVAL;
>       }
>   
> @@ -260,7 +282,7 @@ static int get_ucode_from_buffer_amd(
>       }
>       memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
>   
> -    *offset = off + mpbuf->len + 8;
> +    *offset = off + mpbuf->len + SECTION_HDR_SIZE;
>   
>       pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
>                raw_smp_processor_id(), bufsize, mpbuf->len, off,
> @@ -272,13 +294,16 @@ 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 uint8_t *data,
> +    size_t *tot_size,

Why is this a pointer? This routine does not modify the value.

> +    size_t *curr_offset)
>   {
> +    size_t off = *curr_offset;
> +    uint32_t *buf = (uint32_t *) &data[off];
>       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>   
>       /* No more data */
> -    if ( mpbuf->len + 12 >= *offset )
> +    if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size )
>           return -EINVAL;
>   
>       if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
> @@ -303,7 +328,32 @@ 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 */
> +    *curr_offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
> +
> +    return 0;
> +}
> +
> +static int container_fast_forward(const uint8_t *data, size_t size_left, size_t *offset)
> +{
> +    size_t size, off;
> +    uint32_t *header;
> +
> +    while ( size_left )
> +    {
> +        off = *offset;
> +        header = (uint32_t *) &data[off];
> +        if ( header[0] == UCODE_MAGIC &&
> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = header[1] + SECTION_HDR_SIZE;
> +        *offset += size;
> +        size_left -= size;
> +
> +    }
> +
> +    if ( !size_left )
> +        return -EINVAL;
>   
>       return 0;
>   }
> @@ -311,14 +361,19 @@ static int install_equiv_cpu_table(
>   static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>   {
>       struct microcode_amd *mc_amd, *mc_old;
> -    size_t offset = bufsize;
> +    size_t offset = 0;
> +    size_t tot_size = bufsize;
>       size_t last_offset, applied_offset = 0;
>       int error = 0, save_error = 1;
>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> +    unsigned int current_cpu_id;
> +    unsigned int equiv_cpu_id = 0x0;
>   
>       /* We should bind the task to the CPU */
>       BUG_ON(cpu != raw_smp_processor_id());
>   
> +    current_cpu_id = cpuid_eax(0x00000001);
> +
>       if ( *(const uint32_t *)buf != UCODE_MAGIC )
>       {
>           printk(KERN_ERR "microcode: Wrong microcode patch file magic\n");
> @@ -334,7 +389,30 @@ 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 ( (error = install_equiv_cpu_table(mc_amd, buf, &tot_size,
> +                                             &offset)) == 0 )

install_equiv_cpu_table() checks whether container size is larger than 
the buffer size. If we have multiple containers merged I think tot_size 
is the size of the merged file, not of an individual container. And this 
makes the first check in install_equiv_cpu_table() not especially 
meaningful.

-boris

> +    {
> +        find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> +                          &equiv_cpu_id);
> +        if ( equiv_cpu_id )
> +            break;
> +
> +        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);
> +            error = -EINVAL;
> +            goto out;
> +        }
> +    }
> +
>       if ( error )
>       {
>           xfree(mc_amd);
> @@ -379,8 +457,16 @@ 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 there happens to be multiple container files and if patch
> +         * update succeeded on first container itself, a stale error val
> +         * is returned from get_ucode_from_buffer_amd. So, force
> +         * error = 0 here as we have already succeeded in the update.
> +         */
>           if ( save_error )
>               error = save_error;
> +        else
> +            error = 0;
>       }
>   
>       if ( save_error )