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

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

On 6/20/2014 3:11 AM, Jan Beulich wrote:
>>>> On 19.06.14 at 17:14, <aravind.gopalakrishnan@amd.com> wrote:
>> @@ -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;
>> +        }
>> +    }
>> +}
> Please have this function return a found/not-found indication
> rather than having the callers rely on *equiv_cpu_id transitioning
> from zero to no-zero. That way you can also drop the respective
> initializers in the callers.

Ok, Will modify this.
>> @@ -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,
>> +    size_t *curr_offset)
>>   {
>> +    size_t off = *curr_offset;
>> +    uint32_t *buf = (uint32_t *) &data[off];
> I realize there are may casts there in this file. But please don't add
> more of them without a strict need (replacing existing ones would
> also be much appreciated). In the case here, if "data" was "const
> void *" you'd get away with just "data + off". Furthermore there's
> no reason for having the one-time use variable "off" here - just
> use *offset directly.
Ok, I'll change this per your suggestion.
About replacing the existing casts, I'll generate a separate patch for it..

>> +        if ( header[0] == UCODE_MAGIC &&
>> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
>> +            break;
>> +
>> +        size = header[1] + SECTION_HDR_SIZE;
>> +        *offset += size;
>> +        size_left -= size;
> Don't you think you should first check for size_left >= size here?

Yep. Thanks for pointing this out. I'll add it.

>> @@ -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 )
>> +    {
>> +        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;
> Either you use the error value returned from container_fast_forward(),
> or you make that function simply return a bool_t.
Hmm, Yep. I'll fix this.

>> @@ -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;
> First of all I don't think this change is logically a part of $subject (as
> it also alters behavior for single container files). With that the
> question then is why the change is really correct. And if it was really
> correct, you could just drop the if() instead.
>
>
The change is needed (as stated in the comments) because we end up 
returning err val (EINVAL) like so-
DEBUG: cpu_request_microcode :  no save_err;error=-22

even though update succeeded-
(XEN) microcode: CPU0 found a matching microcode update with version 
0x600063d (current=0x6000626)
(XEN) microcode: CPU0 updated from revision 0x6000626 to 0x600063d

That said, I also understand your point that we can just drop the if(). 
So, I'll change this as per your suggestion.

The fact that we have to make this change as a _consequence_ of adding 
multiple container file support,
might explain why it was done later right?
I can make this a separate patch as well. Let me know what you think.

Thanks,
-Aravind.