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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 6/20/2014 10:57 AM, Boris Ostrovsky wrote:
> 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.
>

Yeah. No point. I'll fix it.
Thanks for the pointer :)

>> +    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. 
Not really.
It is basically checking if we have data at all to parse (like the 
comment says:/* No more data */)

> If we have multiple containers merged I think tot_size is the size of 
> the merged file, not of an individual container. 

That's right.

> And this makes the first check in install_equiv_cpu_table() not 
> especially meaningful.
>

Theoretically-
If it so happens that we don't have any patch files and just the 
container header,
then mpbuf->len would be zero and
  if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size )
will fail in this case- (which gives some meaning to the check being there)
1. If containers are concatenated, but there is no matching patch on the 
first one
2. So, we fast-fwd to next container file but this one has mpbuf->len=0

Which leads me to another scenario though-
1. If we do have multiple containers and if right patch is not on the 
first one, but on some subsequent container
2. If the first container has mpbuf->len=0
Then we'd just error out right there. Hmm.. I'll have to re-work the 
logic then.

Come to think, if the first container is corrupted for some reason and 
returns error (or there's an -ENOMEM), then we are going to return 
pre-maturely
Instead, we should probably (try to) forward to next container and look 
for patches there as well.

Shall fix this logic in a v2..

Thanks,
-Aravind