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

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

On 7/1/2014 3:23 AM, Jan Beulich wrote:
>>>> On 30.06.14 at 18:52, <aravind.gopalakrishnan@amd.com> wrote:
>> @@ -272,14 +293,12 @@ 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)
>>   {
>> -    const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>> +    const struct mpbhdr *mpbuf = (const struct mpbhdr *)(data + *offset + 4);
> There's still a pointless cast here.

Fixed.

>> @@ -303,7 +322,35 @@ 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 */
>> +    return 0;
>> +}
>> +
>> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
>> +{
>> +    size_t size;
>> +    const uint32_t *header;
>> +
>> +    while ( size_left )
>> +    {
>> +        if ( size_left < SECTION_HDR_SIZE )
>> +            return -EINVAL;
>> +
>> +        header = (const uint32_t *) (data + *offset);
> And you again introduce another one here.

Fixed.

>> +    }
>> +
>> +    if ( !size_left )
>> +        return -EINVAL;
> And btw, this check is kind of redundant with the size_left <
> SECTION_HDR_SIZE one inside the loop: If you make the loop
> header "for ( ; ; )", you won't need this extra if().

Fixed.

>> @@ -379,12 +464,48 @@ 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 )
>> +        /*
>> +         * 1. Given a situation where multiple containers exist and correct
>> +         *    patch lives on 1st container
>> +         * 2. We match equivalent ids using find_equiv_cpu_id() from the
>> +         *    earlier while() (On this case, matches on first container
>> +         *    file and we break)
>> +         * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>> +         *                                  buf, bufsize,&offset)) == 0 )
>> +         * 4. Find correct patch using microcode_fits() and apply the patch
>> +         *    (Assume: apply_microcode() is successful)
>> +         * 5. The while() loop from (3) continues to parse the binary as
>> +         *    there is a subsequent container file, but...
>> +         * 6. returns -EINVAL as the condition
>> +         *    if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to false.
>> +         * 7. A correct patch can only be on one container and not on any
>> +         *    subsequent ones. (Refer docs for more info) Therefore, we
>> +         *    don't have to parse a subsequent container. So, we can abort
>> +         *    the process here and discard this error value as we succeeded
>> +         *    already in patch update.
>> +         * 8. Since we need to return 0 for success, force error = 0.
>> +         */
> Much better. One thing I'd like to avoid though is special casing the 1st
> container in the description. Just refer to "some container other than
> the last one" instead.

Okay.

>> +        if ( offset < bufsize )
>> +            error = 0;
>> +        else if ( save_error )
>>               error = save_error;
> And with the comment having become precise, it is now more obvious
> what's odd here: Flushing the error to zero should imo be done
> conditionally upon the next thing following being a UCODE_MAGIC
> rather than the much more generic "offset < bufsize".

Hmm. Yep.
I have been experimenting with this-
if ( offset + SECTION_HDR_SIZE <= bufsize &&
                    *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) {
             printk("DEBUG: hit another container. breaking..\n");
             break;
         }

within the

while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,&offset)) == 0 )

loop and it seems to work fine. i.e, We can actually eliminate the need 
to workaround the
corner cases as we pre-check (if that's the right word) for the 
occurrence of a subsequent container
before the  if ( mpbuf->type != UCODE_UCODE_TYPE ) check fails and 
returns -EINVAL.
This ensures that 'error' variable retains a success value (0)

I have tested this bit with both the edge cases and they work fine.
As a consequence, I'll re-word the comments.

Thanks,
-Aravind.