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

[PATCH V2] 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 are-
 - Define HDR_SIZE's explicitly for code clarity.
 - Define AMD_UCODE_APPLY_SUCCESS so it's clear that we return this
   value if patch application succeeded regardless if there is only one
   container file or multiple ones concatenated.

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.

 - Adding Jan, Boris reviewed-by tags; Dropping Suravee's since logic changed..

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/microcode_amd.c |  141 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e83f4b6..9e5fa9a 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -29,6 +29,17 @@
 
 #define pr_debug(x...) ((void)0)
 
+#define CONT_HDR_SIZE           12
+#define SECTION_HDR_SIZE        8
+#define AMD_UCODE_APPLY_SUCCESS 0
+/*
+ * Currently, there are two AMD container files on
+ * https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode
+ * So, this is max number of container binaries that can be appended
+ * together and packaged along with initrd.
+ */
+#define AMD_MAX_CONT_APPEND     2
+
 struct __packed equiv_cpu_entry {
     uint32_t installed_cpu;
     uint32_t fixed_errata_mask;
@@ -124,30 +135,43 @@ static bool_t verify_patch_size(uint32_t patch_size)
     return (patch_size <= max_size);
 }
 
-static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
+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)
 {
-    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 found = 0;
 
-    /* We should bind the task to the CPU */
-    BUG_ON(cpu != raw_smp_processor_id());
-
-    current_cpu_id = cpuid_eax(0x00000001);
+    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;
+            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
+            found = 1;
             break;
         }
     }
 
-    if ( !equiv_cpu_id )
+    return found;
+}
+
+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;
+
+    /* We should bind the task to the CPU */
+    BUG_ON(cpu != raw_smp_processor_id());
+
+    current_cpu_id = cpuid_eax(0x00000001);
+
+     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 )
@@ -211,7 +235,7 @@ static int apply_microcode(int cpu)
 
     uci->cpu_sig.rev = rev;
 
-    return 0;
+    return AMD_UCODE_APPLY_SUCCESS;
 }
 
 static int get_ucode_from_buffer_amd(
@@ -236,7 +260,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 +292,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 +304,13 @@ 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 void *data,
+    size_t *curr_offset)
 {
+    uint32_t *buf = (uint32_t *) (data + *curr_offset);
     const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
 
-    /* No more data */
-    if ( mpbuf->len + 12 >= *offset )
-        return -EINVAL;
+    *curr_offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
 
     if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
     {
@@ -303,7 +334,30 @@ 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;
+    uint32_t *header;
+
+    while ( size_left )
+    {
+        header = (uint32_t *) (data + *offset);
+        if ( header[0] == UCODE_MAGIC &&
+             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
+            break;
+
+        size = header[1] + SECTION_HDR_SIZE;
+        *offset += size;
+
+        if ( size_left >= size )
+            size_left -= size;
+    }
+
+    if ( !size_left )
+        return -EINVAL;
 
     return 0;
 }
@@ -311,14 +365,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 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;
+    unsigned int containers_processed = 0;
 
     /* 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 +393,33 @@ 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 ( containers_processed <= AMD_MAX_CONT_APPEND )
+    {
+        containers_processed++;
+        error = install_equiv_cpu_table(mc_amd, buf, &offset);
+
+        if ( !error )
+        {
+             if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
+                                    &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);
+            goto out;
+        }
+    }
+
     if ( error )
     {
         xfree(mc_amd);
@@ -379,7 +464,15 @@ 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 )
+        /*
+         * If there happens to be multiple container files and if patch
+         * update succeeded on an earlier container itself, a stale error
+         * val is returned from get_ucode_from_buffer_amd. Since we already
+         * succeeded in patch application, return as success itself
+         */
+        if ( (containers_processed < AMD_MAX_CONT_APPEND) && error )
+            error = AMD_UCODE_APPLY_SUCCESS;
+        else if ( save_error )
             error = save_error;
     }
 
-- 
1.7.9.5

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

[Jan Beulich]

>>> On 23.06.14 at 22:25, <aravind.gopalakrishnan@amd.com> wrote:
>  - Adding Jan, Boris reviewed-by tags; Dropping Suravee's since logic 
> changed..

This is an absolute no-go: Neither did I (nor - afaict - Boris) offer
these, nor would a heavily changed patch warrant retaining any
previous offerings. Both of us did review the previous iteration,
but whether your changes actually allow for these tags can't be
known until both of us had a chance to look at them.

> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -29,6 +29,17 @@
>  
>  #define pr_debug(x...) ((void)0)
>  
> +#define CONT_HDR_SIZE           12
> +#define SECTION_HDR_SIZE        8
> +#define AMD_UCODE_APPLY_SUCCESS 0
> +/*
> + * Currently, there are two AMD container files on
> + * 
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode
> + * So, this is max number of container binaries that can be appended
> + * together and packaged along with initrd.
> + */
> +#define AMD_MAX_CONT_APPEND     2

So as soon as a 3rd one appears the code will need to be changed
again? Pretty bad approach I would say.

> @@ -124,30 +135,43 @@ static bool_t verify_patch_size(uint32_t patch_size)
>      return (patch_size <= max_size);
>  }
>  
> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +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)
>  {
> -    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 found = 0;

This is your return value - why is it "unsigned int" when the function
returns bool_t? But the variable is pointless anyway, since ...

>  
> -    /* We should bind the task to the CPU */
> -    BUG_ON(cpu != raw_smp_processor_id());
> -
> -    current_cpu_id = cpuid_eax(0x00000001);
> +    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;
> +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
> +            found = 1;
>              break;
>          }
>      }
>  
> -    if ( !equiv_cpu_id )
> +    return found;
> +}

... rather than break-ing from the loop you could just "return 1"
there.

> @@ -211,7 +235,7 @@ static int apply_microcode(int cpu)
>  
>      uci->cpu_sig.rev = rev;
>  
> -    return 0;
> +    return AMD_UCODE_APPLY_SUCCESS;

Definitely not: This function _has to_ return zero (due to its use in
the initializer of microcode_amd_ops), i.e. someone altering the
definition of AMD_UCODE_APPLY_SUCCESS (e.g. making it a
boolean) would break things.

> @@ -236,7 +260,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");

The more often I look at it, the less convinced am I that this is okay:
UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
that the field matches up with the next block signature. Either don't
abuse the field, or make clear via comment why it is done this way.

> @@ -272,14 +304,13 @@ 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 void *data,
> +    size_t *curr_offset)

Is there any strong reason to rename "offset" to "curr_offset", ...

>  {
> +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
>      const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>  
> -    /* No more data */
> -    if ( mpbuf->len + 12 >= *offset )
> -        return -EINVAL;

Iirc you and Boris agreed that this check is pointless _here_. But I
doubt it can be removed without replacement elsewhere.

> +    *curr_offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
>  
>      if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>      {
> @@ -303,7 +334,30 @@ 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 */

... and to move this assignment up?

> +    return 0;
> +}
> +
> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
> +{
> +    size_t size;
> +    uint32_t *header;
> +
> +    while ( size_left )
> +    {
> +        header = (uint32_t *) (data + *offset);

Pointless (and wrong, as it discards the const qualifier) cast.

> +        if ( header[0] == UCODE_MAGIC &&
> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )

Are these two valid if size_left < SECTION_HDR_SIZE?

> +            break;
> +
> +        size = header[1] + SECTION_HDR_SIZE;
> +        *offset += size;
> +
> +        if ( size_left >= size )
> +            size_left -= size;
> +    }

And if size_left < size we're continuing the loop (perhaps indefinitely)?

> @@ -334,7 +393,33 @@ 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 ( containers_processed <= AMD_MAX_CONT_APPEND )

Leaving aside the point made above regarding this limit, you set the
limit to 2 but then allow 3 iterations?

> +    {
> +        containers_processed++;
> +        error = install_equiv_cpu_table(mc_amd, buf, &offset);
> +
> +        if ( !error )
> +        {
> +             if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id,
> +                                    &equiv_cpu_id) )
> +                break;

I think I said this on earlier occasions already: Such if-s should be
folded into one.

> +        }
> +
> +        error = container_fast_forward(buf, (bufsize - offset), &offset);

Pointless parentheses around the second argument expression.

> @@ -379,7 +464,15 @@ 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 )
> +        /*
> +         * If there happens to be multiple container files and if patch
> +         * update succeeded on an earlier container itself, a stale error
> +         * val is returned from get_ucode_from_buffer_amd. Since we already
> +         * succeeded in patch application, return as success itself
> +         */
> +        if ( (containers_processed < AMD_MAX_CONT_APPEND) && error )
> +            error = AMD_UCODE_APPLY_SUCCESS;
> +        else if ( save_error )
>              error = save_error;

This still seems wrong to me: For one, you don't ever try to apply
a patch from another container if one succeeded (since the looking
for containers sits in a separate loop prior to the one looking into
the container, and there's no path back to the top of the first loop).
Further I again don't follow the conditions you use:
containers_processed >= AMD_MAX_CONT_APPEND should have
got dealt with (if at all possible) much earlier (after the first loop),
and the "&& error" looks superfluous as long as
AMD_UCODE_APPLY_SUCCESS == 0. Which finally gets us back to
the same point made above - this function again has to return zero
on success, no matter what AMD_UCODE_APPLY_SUCCESS is
defined to.

Jan

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

[Aravind Gopalakrishnan]

On 6/24/2014 2:23 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -29,6 +29,17 @@
>>   
>>   #define pr_debug(x...) ((void)0)
>>   
>> +#define CONT_HDR_SIZE           12
>> +#define SECTION_HDR_SIZE        8
>> +#define AMD_UCODE_APPLY_SUCCESS 0
>> +/*
>> + * Currently, there are two AMD container files on
>> + *
>> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode
>> + * So, this is max number of container binaries that can be appended
>> + * together and packaged along with initrd.
>> + */
>> +#define AMD_MAX_CONT_APPEND     2
> So as soon as a 3rd one appears the code will need to be changed
> again? Pretty bad approach I would say.

Yeah. I thought about it too. But if a new container comes up, then it 
will still take
very little effort to change this macro here.

The idea was that I could use this for catching the corner case.

Anyway, I have got a different way to handle that. (so this macro def 
will go as a result)
(explanation below...)

>> @@ -124,30 +135,43 @@ static bool_t verify_patch_size(uint32_t patch_size)
>>       return (patch_size <= max_size);
>>   }
>>   
>> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>> +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)
>>   {
>> -    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 found = 0;
> This is your return value - why is it "unsigned int" when the function
> returns bool_t? But the variable is pointless anyway, since ...
>
>>   
>> -    /* We should bind the task to the CPU */
>> -    BUG_ON(cpu != raw_smp_processor_id());
>> -
>> -    current_cpu_id = cpuid_eax(0x00000001);
>> +    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;
>> +            *equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0xffff;
>> +            found = 1;
>>               break;
>>           }
>>       }
>>   
>> -    if ( !equiv_cpu_id )
>> +    return found;
>> +}
> ... rather than break-ing from the loop you could just "return 1"
> there.

Fixed this.

>> @@ -211,7 +235,7 @@ static int apply_microcode(int cpu)
>>   
>>       uci->cpu_sig.rev = rev;
>>   
>> -    return 0;
>> +    return AMD_UCODE_APPLY_SUCCESS;
> Definitely not: This function _has to_ return zero (due to its use in
> the initializer of microcode_amd_ops), i.e. someone altering the
> definition of AMD_UCODE_APPLY_SUCCESS (e.g. making it a
> boolean) would break things.

So, the main reason behind introducing this macro was that-
For single containers, in cases of success, the last value assigned for 
'error'
is from 'apply_microcode' (above). And unless there is a 'save_error', 
this is the value
returned.
I wanted to make it clear that we are returning this value for multiple 
containers too (on success)

I understand your concern, So I have changed it to return 0.

>> @@ -236,7 +260,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");
> The more often I look at it, the less convinced am I that this is okay:
> UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
> that the field matches up with the next block signature. Either don't
> abuse the field, or make clear via comment why it is done this way.

Would this be better? -
if ( *(const uint32_t *)buf != UCODE_MAGIC )

I can still let the above comment be there (or modify it to make it more 
clear?)


>> @@ -272,14 +304,13 @@ 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 void *data,
>> +    size_t *curr_offset)
> Is there any strong reason to rename "offset" to "curr_offset", ...

No. Have fixed this.

>>   {
>> +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
>>       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>>   
>> -    /* No more data */
>> -    if ( mpbuf->len + 12 >= *offset )
>> -        return -EINVAL;
> Iirc you and Boris agreed that this check is pointless _here_. But I
> doubt it can be removed without replacement elsewhere.

For single containers, this check made some sense earlier as we verify 
to see there is *some*
data beyond the equivalent_table structure.
Say, mpbuf->len=0 and we return error val; Due to the fact that we have 
already advanced *offset,
cases when we reach EOF or *offset goes over bufsize is handled in 
container_fast_forward
function.

For multiple containers, we will always have at least two such container 
headers and hence,
mpbuf->len + 12 is always less than total_size

If first container for some reason is corrupted and exposes 
mpbuf->len=0,  we return EINVAL
and forward to next container.
(This is infact one reason to  advance *offset earlier. See below)

Now, if the last container were to have mpbuf->len=0,
As Boris mentioned on earlier thread, we will
continue because 'if (0+12 >= tot_size) ' is false.
Here too, we will return EINVAL.

Again, advancing *offset early allows to workaround these issues.
And this check can be removed as a result.

>> +    *curr_offset += mpbuf->len + CONT_HDR_SIZE;	/* add header length */
>>   
>>       if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE )
>>       {
>> @@ -303,7 +334,30 @@ 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 */
> ... and to move this assignment up?

So, If for some reason this function returns error val for the _first_ 
container;
Then *offset would still be 0.
And 'container_fast_forward' will not advance it as both 'MAGIC' and 
'UCODE_EQUIV_CPU_TABLE_TYPE' checks match.
We will be stuck in this loop.

Hence, I moved the assignment up.
But, this is also introducing some issues that I failed to handle here.
Have fixed it now. (See below)

>> +    return 0;
>> +}
>> +
>> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
>> +{
>> +    size_t size;
>> +    uint32_t *header;
>> +
>> +    while ( size_left )
>> +    {
>> +        header = (uint32_t *) (data + *offset);
> Pointless (and wrong, as it discards the const qualifier) cast.

Ok. I have removed this cast (and as a consequence *header) entirely.
Also need -
if ( size_left < 0 ) check. Reason-
With offset value being advanced aggressively in 'install_equiv_cpu_table',
It could be that we go over the bufsize. This situation will be caught here.


>> +        if ( header[0] == UCODE_MAGIC &&
>> +             header[1] == UCODE_EQUIV_CPU_TABLE_TYPE )
> Are these two valid if size_left < SECTION_HDR_SIZE?

Fixed this.

>> +            break;
>> +
>> +        size = header[1] + SECTION_HDR_SIZE;
>> +        *offset += size;
>> +
>> +        if ( size_left >= size )
>> +            size_left -= size;
>> +    }
> And if size_left < size we're continuing the loop (perhaps indefinitely)?

Fixed this.
Instead of above check, I inverted the relation and saved an indentation 
level.


>> @@ -334,7 +393,33 @@ 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 ( containers_processed <= AMD_MAX_CONT_APPEND )
> Leaving aside the point made above regarding this limit, you set the
> limit to 2 but then allow 3 iterations?

Have modified this to while ( offset < bufsize )

>> @@ -379,7 +464,15 @@ 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 )
>> +        /*
>> +         * If there happens to be multiple container files and if patch
>> +         * update succeeded on an earlier container itself, a stale error
>> +         * val is returned from get_ucode_from_buffer_amd. Since we already
>> +         * succeeded in patch application, return as success itself
>> +         */
>> +        if ( (containers_processed < AMD_MAX_CONT_APPEND) && error )
>> +            error = AMD_UCODE_APPLY_SUCCESS;
>> +        else if ( save_error )
>>               error = save_error;
> This still seems wrong to me: For one, you don't ever try to apply
> a patch from another container if one succeeded (since the looking
> for containers sits in a separate loop prior to the one looking into
> the container, and there's no path back to the top of the first loop).

This is because there can be only one container that has the valid patch.

> Further I again don't follow the conditions you use:
> containers_processed >= AMD_MAX_CONT_APPEND should have
> got dealt with (if at all possible) much earlier (after the first loop),
> and the "&& error" looks superfluous as long as
> AMD_UCODE_APPLY_SUCCESS == 0. Which finally gets us back to
> the same point made above - this function again has to return zero
> on success, no matter what AMD_UCODE_APPLY_SUCCESS is
> defined to.
>
>

Ok, I have removed AMD_MAX_CONT_APPEND altogether now, and changed it to-
if ( offset < bufsize )
   error = 0;

 From the loop: while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
we will always have (offset == bufsize) if the patch happens to be on 
the last container file (even as apply_microcode succeeds)

So, the modified check should work fine. While at the same time does not 
change behavior of single container files.

Btw, we also need a similar check here:
   if ( save_error )
     {
         if ( (error != save_error) && (offset < bufsize) ) {
              error = 0;
         }
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }

Reason:
By the time 'microcode_init' runs, we have already updated the patch 
level on all (currently) running cpus.
So, get_ucode_from_buffer_amd will return -EINVAL due to -
if ( mpbuf->type != UCODE_UCODE_TYPE )

Only this time, there is no 'applied_offset' as well.
So, 'save_error' = 1. But error == -EINVAL.
This situation can only occur in the case of multiple container files 
and only when update succeeded with
the first container file itself. Hence, this check is also necessary to 
return 0 for success.

I'll await your comments before sending out a V3.

Thanks,
-Aravind.

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

[Boris Ostrovsky]

On 06/24/2014 06:42 PM, Aravind Gopalakrishnan wrote:
> On 6/24/2014 2:23 AM, Jan Beulich wrote:
>
>>> @@ -272,14 +304,13 @@ 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 void *data,
>>> +    size_t *curr_offset)
>> Is there any strong reason to rename "offset" to "curr_offset", ...
>
> No. Have fixed this.
>
>>>   {
>>> +    uint32_t *buf = (uint32_t *) (data + *curr_offset);
>>>       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>>>   -    /* No more data */
>>> -    if ( mpbuf->len + 12 >= *offset )
>>> -        return -EINVAL;
>> Iirc you and Boris agreed that this check is pointless _here_. But I
>> doubt it can be removed without replacement elsewhere.
>
> For single containers, this check made some sense earlier as we verify 
> to see there is *some*
> data beyond the equivalent_table structure.
> Say, mpbuf->len=0 and we return error val; Due to the fact that we 
> have already advanced *offset,
> cases when we reach EOF or *offset goes over bufsize is handled in 
> container_fast_forward
> function.
>
> For multiple containers, we will always have at least two such 
> container headers and hence,
> mpbuf->len + 12 is always less than total_size
>
> If first container for some reason is corrupted and exposes 
> mpbuf->len=0,  we return EINVAL
> and forward to next container.
> (This is infact one reason to  advance *offset earlier. See below)
>
> Now, if the last container were to have mpbuf->len=0,
> As Boris mentioned on earlier thread, we will
> continue because 'if (0+12 >= tot_size) ' is false.
> Here too, we will return EINVAL.
>
> Again, advancing *offset early allows to workaround these issues.
> And this check can be removed as a result.

Let's say we have a single container and the file got truncated (i.e. 
bufsize in cpu_request_microcode() is smaller than it should be). Aren't 
we now risking doing a memcpy out of too short a buffer?

-boris

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

[Jan Beulich]

>>> On 25.06.14 at 00:42, <aravind.gopalakrishnan@amd.com> wrote:
> On 6/24/2014 2:23 AM, Jan Beulich wrote:
>>> @@ -236,7 +260,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");
>> The more often I look at it, the less convinced am I that this is okay:
>> UCODE_MAGIC isn't a valid value for mpbuf->type, it just so happens
>> that the field matches up with the next block signature. Either don't
>> abuse the field, or make clear via comment why it is done this way.
> 
> Would this be better? -
> if ( *(const uint32_t *)buf != UCODE_MAGIC )
> 
> I can still let the above comment be there (or modify it to make it more 
> clear?)

As I said in the previous reply (see above), I'm not really decided
which of the two options is the better one. Perhaps the one using
a cast on buf as you just now suggested is slightly preferable, as
it matches the other similar check(s).

>>> +    return 0;
>>> +}
>>> +
>>> +static int container_fast_forward(const void *data, size_t size_left, 
> size_t *offset)
>>> +{
>>> +    size_t size;
>>> +    uint32_t *header;
>>> +
>>> +    while ( size_left )
>>> +    {
>>> +        header = (uint32_t *) (data + *offset);
>> Pointless (and wrong, as it discards the const qualifier) cast.
> 
> Ok. I have removed this cast (and as a consequence *header) entirely.
> Also need -
> if ( size_left < 0 ) check. Reason-
> With offset value being advanced aggressively in 'install_equiv_cpu_table',
> It could be that we go over the bufsize. This situation will be caught here.

Except that size_left is of unsigned type, i.e. will never be < 0.

Jan

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

[Aravind Gopalakrishnan]

On 6/24/2014 11:04 PM, Boris Ostrovsky wrote:
>
>>>>   -    /* No more data */
>>>> -    if ( mpbuf->len + 12 >= *offset )
>>>> -        return -EINVAL;
>>> Iirc you and Boris agreed that this check is pointless _here_. But I
>>> doubt it can be removed without replacement elsewhere.
>>
>> For single containers, this check made some sense earlier as we 
>> verify to see there is *some*
>> data beyond the equivalent_table structure.
>> Say, mpbuf->len=0 and we return error val; Due to the fact that we 
>> have already advanced *offset,
>> cases when we reach EOF or *offset goes over bufsize is handled in 
>> container_fast_forward
>> function.
>>
>> For multiple containers, we will always have at least two such 
>> container headers and hence,
>> mpbuf->len + 12 is always less than total_size
>>
>> If first container for some reason is corrupted and exposes 
>> mpbuf->len=0,  we return EINVAL
>> and forward to next container.
>> (This is infact one reason to  advance *offset earlier. See below)
>>
>> Now, if the last container were to have mpbuf->len=0,
>> As Boris mentioned on earlier thread, we will
>> continue because 'if (0+12 >= tot_size) ' is false.
>> Here too, we will return EINVAL.
>>
>> Again, advancing *offset early allows to workaround these issues.
>> And this check can be removed as a result.
>
> Let's say we have a single container and the file got truncated (i.e. 
> bufsize in cpu_request_microcode() is smaller than it should be). 
> Aren't we now risking doing a memcpy out of too short a buffer?
>
>

No, because in 'install_equiv_cpu_table', we only alloc memory and 
memcpy data for the equiv_cpu_table.
Alloc-ing memory (and memcpy) for the patch is handled by 
get_ucode_from_buffer_amd;
and corrupted files (like the ones you say) should be handled by this-
  if ( (off + mpbuf->len) > bufsize )
{
    printk(KERN_ERR "microcode: Bad data in microcode data file\n");
    return -EINVAL;
}

-Aravind.

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

[Aravind Gopalakrishnan]

On 6/25/2014 5:47 AM, Jan Beulich wrote:
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int container_fast_forward(const void *data, size_t size_left,
>> size_t *offset)
>>>> +{
>>>> +    size_t size;
>>>> +    uint32_t *header;
>>>> +
>>>> +    while ( size_left )
>>>> +    {
>>>> +        header = (uint32_t *) (data + *offset);
>>> Pointless (and wrong, as it discards the const qualifier) cast.
>> Ok. I have removed this cast (and as a consequence *header) entirely.
>> Also need -
>> if ( size_left < 0 ) check. Reason-
>> With offset value being advanced aggressively in 'install_equiv_cpu_table',
>> It could be that we go over the bufsize. This situation will be caught here.
> Except that size_left is of unsigned type, i.e. will never be < 0.
>
>
Oh. Missed that. Shall fix this.

Thanks,
-Aravind.