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

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

[Aravind Gopalakrishnan]

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

Other changes introduced:
 - Define HDR_SIZE's explicitly for code clarity.
 - Minor cleanup: Remove extra casts in that are used in
   install_equiv_cpu_table()

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
Changes in V4: (per Jan suggestions)
 - Make comments that workaround the corner cases more precise.
 - Remove another pointless cast in file.
 - Go back to using 'header' variable in container_fast_forward
   with a const qualifier.
 - Catch another corner case (size == 0) in container_fast_forward

Changes in V3: (per Jan suggestions)
 - 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

Changes in V2
 - per Jan suggestion
   - return bool_t from find_equiv_cpu_id
    => drop the respective initializers of equiv_cpu_id in the callers
   - Reduce number of casts by using void * for passing raw binary whenever
     possible
   - Need size_left >= size check in container_fast_forward
   - Use error value returned from container_fast_forward in
     cpu_request_microcode
   - If changing code around 'error = save_error', make sure the behavior
     for single container files does not change

 - per Boris suggestion
   - No need use pointer type for tot_size
    => We can remove this from the caller too as it is unnecessary
   - if ( mpbuf->len + CONT_HDR_SIZE >= *tot_size ) check can be
     removed
 
 -  Fix logic
   - if the first container is corrupted for some reason and returns error
     then we return pre-maturely. Instead, now we at least (try to) forward
     to next container and look for patches there as well
   - Using AMD_MAX_CONT_APPEND as loop condition check simply because I
     wanted to avoid using a while (1) which would also work just as well
   - This check makes some logical sense too as there are only 2 available
     AMD containers out there now. So if we did not find correct patch by then,
     we might as well stop trying.

 xen/arch/x86/microcode_amd.c |  167 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 144 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e83f4b6..e605f22 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,30 +127,41 @@ static bool_t verify_patch_size(uint32_t patch_size)
     return (patch_size <= max_size);
 }
 
+static bool_t 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 0;
+
+    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;
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     const struct microcode_header_amd *mc_header = mc_amd->mpb;
     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;
+    unsigned int equiv_cpu_id;
 
     /* 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;
-        }
-    }
-
-    if ( !equiv_cpu_id )
+    if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) )
         return 0;
 
     if ( (mc_header->processor_rev_id) != equiv_cpu_id )
@@ -236,7 +250,14 @@ 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");
+        /*
+         * In a situation where ucode update has succeeded;
+         * but there is a subsequent container file being parsed,
+         * then, there is no need of this ERR message to be printed.
+         */
+        if ( *(const uint32_t *)buf != UCODE_MAGIC )
+            printk(KERN_ERR "microcode: Wrong microcode payload type field\n");
+
         return -EINVAL;
     }
 
@@ -260,7 +281,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,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);
 
-    /* No more data */
-    if ( mpbuf->len + 12 >= *offset )
-        return -EINVAL;
+    *offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
 
     if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
     {
@@ -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);
+
+        if ( header[0] == UCODE_MAGIC &&
+             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
+            break;
+
+        size = header[1] + SECTION_HDR_SIZE;
+        if ( size == 0 || size_left < size )
+            return -EINVAL;
+
+        size_left -= size;
+        *offset += size;
+    }
+
+    if ( !size_left )
+        return -EINVAL;
 
     return 0;
 }
@@ -311,14 +358,18 @@ 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 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;
 
     /* 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 +385,41 @@ 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");
+            error = -EINVAL;
+            goto out;
+        }
+
+        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;
+        }
+    }
+
     if ( error )
     {
         xfree(mc_amd);
@@ -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.
+         */
+        if ( offset < bufsize )
+            error = 0;
+        else if ( save_error )
             error = save_error;
     }
 
     if ( save_error )
     {
+        /*
+         * 1. Given a situation where multiple containers exist and correct
+         *    patch lives on 1st container (AND) we have already applied the
+         *    patch update after microcode_presmp_init(), microcode_resume_cpu()
+         * 2. microcode_init() runs and calls into cpu_request_microcode()
+         * 3. Read points 2 to 7 from previous comment.
+         * 4. Except that, this time there is no 'applied_offset'
+         *     => 'save_error' = 1. But error = -EINVAL
+         * 5. Since we need to return 0 for success, force it here.
+         */
+        if ( (error != save_error) && (offset < bufsize) )
+            error = 0;
+
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }
-- 
1.7.9.5

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

[Jan Beulich]

>>> 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.

> @@ -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.

> +
> +        if ( header[0] == UCODE_MAGIC &&
> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = header[1] + SECTION_HDR_SIZE;
> +        if ( size == 0 || size_left < size )
> +            return -EINVAL;
> +
> +        size_left -= size;
> +        *offset += size;

I know I pointed at only the size == 0 case leading to an infinite loop,
but I assumed you wouldn't limit the adjustment to just that extreme
case. Is e.g. size == 1 valid here? Likely not, i.e. you will rather want
to use a "size < some_minimum_value" check above.

> +    }
> +
> +    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().

> @@ -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.

> +        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". Furthermore
I wonder whether you wouldn't rather want to clear save_error in
that case (simplifying - if not eliminating the need for - the change
below).

Jan

>      }
>  
>      if ( save_error )
>      {
> +        /*
> +         * 1. Given a situation where multiple containers exist and correct
> +         *    patch lives on 1st container (AND) we have already applied the
> +         *    patch update after microcode_presmp_init(), microcode_resume_cpu()
> +         * 2. microcode_init() runs and calls into cpu_request_microcode()
> +         * 3. Read points 2 to 7 from previous comment.
> +         * 4. Except that, this time there is no 'applied_offset'
> +         *     => 'save_error' = 1. But error = -EINVAL
> +         * 5. Since we need to return 0 for success, force it here.
> +         */
> +        if ( (error != save_error) && (offset < bufsize) )
> +            error = 0;
> +
>          xfree(mc_amd);
>          uci->mc.mc_amd = mc_old;
>      }

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

[Aravind Gopalakrishnan]

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.