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

[PATCH] 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

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,
+    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 )
+    {
+        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 )
-- 
1.7.9.5

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

[Jan Beulich]

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

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

> +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];

Same comments as a above apply here.

> +        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?

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

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

Jan

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

[Aravind Gopalakrishnan]

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.

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

[Jan Beulich]

>>> On 20.06.14 at 18:06, <aravind.gopalakrishnan@amd.com> wrote:
> On 6/20/2014 3:11 AM, Jan Beulich wrote:
>>>>> On 19.06.14 at 17:14, <aravind.gopalakrishnan@amd.com> wrote:
>>> @@ -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.

I think I follow what you're saying, in which case this clearly shouldn't
be a separate patch. But behavior for single-container files still should
remain unaltered, so some change other than folding the if and else
branches is going to be needed afaict.

Jan

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

[Aravind Gopalakrishnan]

On 6/23/2014 1:33 AM, Jan Beulich wrote:
>>>> On 20.06.14 at 18:06, <aravind.gopalakrishnan@amd.com> wrote:
>> On 6/20/2014 3:11 AM, Jan Beulich wrote:
>>>>>> On 19.06.14 at 17:14, <aravind.gopalakrishnan@amd.com> wrote:
>>>> @@ -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.
> I think I follow what you're saying, in which case this clearly shouldn't
> be a separate patch. But behavior for single-container files still should
> remain unaltered, so some change other than folding the if and else
> branches is going to be needed afaict.
>
>

I have addressed your concern in a V2.

Thanks,
-Aravind.

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

[Boris Ostrovsky]

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 )

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

[Aravind Gopalakrishnan]

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

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

[Boris Ostrovsky]

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

So if you have two merged containers, the first one being 1K and the 
second one empty, then while processing the second container you will 
instead of returning -EINVAL continue because 'if (0+12 >= 1K) ' will 
evaluate to false. And it seems to me that you then may well  go beyond 
the end of the buffer.


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

Isn't it the other way around? We won't error out, we will continue. 
Which is wrong.

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

BTW, it may be worth documenting container format in English. As it 
stands now, one has to look at the code to figure out the layout.

-boris

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

[Aravind Gopalakrishnan]

On 6/20/2014 9:33 PM, Boris Ostrovsky wrote:
>
>>
>>
>>> 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
>
> So if you have two merged containers, the first one being 1K and the 
> second one empty, then while processing the second container you will 
> instead of returning -EINVAL continue because 'if (0+12 >= 1K) ' will 
> evaluate to false. 

Hmm. Yes, You are right. This check here seems meaningless..

> And it seems to me that you then may well  go beyond the end of the 
> buffer.
>
>

I don't think so as we will hit if ( mpbuf->len == 0 ) and return -EINVAL;

>>
>> 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.
>
> Isn't it the other way around? We won't error out, we will continue. 
> Which is wrong.
>

No. We should hit-
if ( mpbuf->len == 0 )
     {
         printk(KERN_ERR "microcode: Wrong microcode equivalent cpu 
table length\n");
         return -EINVAL;
     }

And with the while loop as it is now will not look for a possible 2nd 
container.

Also, as said below, in case we hit if ( mpbuf->type != 
UCODE_EQUIV_CPU_TABLE_TYPE ) or if ( !mc_amd->equiv_cpu_table )
then we will not enter the loop.

I have fixed this now.
Will send it in a V2.

>>
>> 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..
>>
>
> BTW, it may be worth documenting container format in English. As it 
> stands now, one has to look at the code to figure out the layout.
>

Yeah. I shall work on a draft version and send it as RFC.

Thanks,
-Aravind.