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

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

Changes in V3
 - 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.

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
 xen/arch/x86/microcode_amd.c |  150 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 128 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e83f4b6..8a7fa4a 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,13 @@ 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)
 {
+    uint32_t *buf = (uint32_t *) (data + *offset);
     const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
 
-    /* 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 +323,33 @@ 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;
+
+    while ( size_left )
+    {
+        if ( size_left < SECTION_HDR_SIZE )
+            return -EINVAL;
+
+        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
+             *(const uint32_t *)(data + *offset + 4) ==
+                                 UCODE_EQUIV_CPU_TABLE_TYPE )
+            break;
+
+        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;
+        if ( size_left < size )
+            return -EINVAL;
+
+        size_left -= size;
+        *offset += size;
+    }
+
+    if ( !size_left )
+        return -EINVAL;
 
     return 0;
 }
@@ -311,14 +357,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 +384,40 @@ 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");
+            return -EINVAL;
+        }
+
+        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 +462,35 @@ 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, force error = 0
+         */
+        if ( offset < bufsize )
+            error = 0;
+        else if ( save_error )
             error = save_error;
     }
 
     if ( save_error )
     {
+        /*
+         * By the time 'microcode_init' runs, we have already updated the
+         * patch level on all (currently) running cpus.
+         * But, get_ucode_from_buffer_amd will return -EINVAL as
+         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
+         * Multiple containers are present && update succeeded with the
+         * first container file itself.
+         *
+         * Only this time, there is no 'applied_offset' as well.
+         * So, 'save_error' = 1. But error = -EINVAL.
+         * Hence, this check is necessary to return 0 for success.
+         */
+        if ( (error != save_error) && (offset < bufsize) )
+            error = 0;
+
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }
-- 
1.7.9.5

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

[Boris Ostrovsky]

On 06/25/2014 03:34 PM, 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
>
> Other changes introduced:
>   - Define HDR_SIZE's explicitly for code clarity.
>
> Changes in V3
>   - 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.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
>   xen/arch/x86/microcode_amd.c |  150 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 128 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index e83f4b6..8a7fa4a 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,13 @@ 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)
>   {
> +    uint32_t *buf = (uint32_t *) (data + *offset);
>       const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
>   
> -    /* 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 +323,33 @@ 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;
> +
> +    while ( size_left )
> +    {
> +        if ( size_left < SECTION_HDR_SIZE )
> +            return -EINVAL;
> +
> +        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
> +             *(const uint32_t *)(data + *offset + 4) ==
> +                                 UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;
> +        if ( size_left < size )
> +            return -EINVAL;
> +
> +        size_left -= size;
> +        *offset += size;
> +    }
> +
> +    if ( !size_left )
> +        return -EINVAL;
>   
>       return 0;
>   }
> @@ -311,14 +357,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 +384,40 @@ 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");
> +            return -EINVAL;
> +        }
> +
> +        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;
> +        }
> +    }
> +

I just realized that I don't understand something: if you have two 
merged container files, both having an entry for current processor in 
the equivalence table but only the second file having the actual patch 
(or at least the patch with higher version), will we get to load that patch?

It seems to me that in this case we will break from the loop above 
pointing to microcode data in the first container. We then enter 
get_ucode_from_buffer_amd() loop and when we reach the second container 
that routine will return -EINVAL when it sees that type is not 
UCODE_UCODE_TYPE. It won't print the warning if sees UCODE_MAGIC but 
still gives back an error.

In other words, do we need to somehow fast-forward over the second 
header in this loop as well?

-boris

>       if ( error )
>       {
>           xfree(mc_amd);
> @@ -379,12 +462,35 @@ 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, force error = 0
> +         */
> +        if ( offset < bufsize )
> +            error = 0;
> +        else if ( save_error )
>               error = save_error;
>       }
>   
>       if ( save_error )
>       {
> +        /*
> +         * By the time 'microcode_init' runs, we have already updated the
> +         * patch level on all (currently) running cpus.
> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
> +         * Multiple containers are present && update succeeded with the
> +         * first container file itself.
> +         *
> +         * Only this time, there is no 'applied_offset' as well.
> +         * So, 'save_error' = 1. But error = -EINVAL.
> +         * Hence, this check is necessary to return 0 for success.
> +         */
> +        if ( (error != save_error) && (offset < bufsize) )
> +            error = 0;
> +
>           xfree(mc_amd);
>           uci->mc.mc_amd = mc_old;
>       }

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

[Aravind Gopalakrishnan]

On 6/26/2014 8:14 AM, Boris Ostrovsky wrote:
>
>> +    /*
>> +     * 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");
>> +            return -EINVAL;
>> +        }
>> +
>> +        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;
>> +        }
>> +    }
>> +
>
> I just realized that I don't understand something: if you have two 
> merged container files, both having an entry for current processor in 
> the equivalence table but only the second file having the actual patch 
> (or at least the patch with higher version), will we get to load that 
> patch?
>

We will not come across such a situation:
One container has patch files for families 10h,11h,12h,14h and the other 
has patches for 15h.
So there will be an equiv_cpu_id match in (at least and) only *one* of 
the containers.
(If there ever are patches for 16h, then I suspect it will have a 
separate container of it's own. So there will not be any clashes with 
older containers)

Besides, the patches themselves are not incremental, i.e;
If there is a newer patch, then it handles all errata/bugs that were 
handled by the previous patch and then some.
Which means, you will not have a container that has two patches for the 
same processor.
So, *strictly speaking*, even the loop to get_ucode_from_buffer_amd() 
till the end of buffer is not necessary once we
have already applied a patch.

But, to change this behavior now will need more logic rework..
Then again, if it works fine right now, would we really want to change 
this behavior?

Thanks,
-Aravind.

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

[Jan Beulich]

>>> On 25.06.14 at 21:34, <aravind.gopalakrishnan@amd.com> 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 
> 
> Other changes introduced:
>  - Define HDR_SIZE's explicitly for code clarity.
> 
> Changes in V3
>  - 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

Can you please get used to put this revision log after the first ---
marker? It doesn't belong in ti actual commit message.

> @@ -272,14 +293,13 @@ 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)
>  {
> +    uint32_t *buf = (uint32_t *) (data + *offset);

I know I complained about this or a similar pointless cast for the
previous version.

> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
> +{
> +    size_t size;
> +
> +    while ( size_left )
> +    {
> +        if ( size_left < SECTION_HDR_SIZE )
> +            return -EINVAL;
> +
> +        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
> +             *(const uint32_t *)(data + *offset + 4) ==
> +                                 UCODE_EQUIV_CPU_TABLE_TYPE )
> +            break;
> +
> +        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;

Now these three casts+derefs surely warrant a "const uint32_t *"
variable, making the resulting code quite a bit easier to read.

> +        if ( size_left < size )
> +            return -EINVAL;
> +
> +        size_left -= size;
> +        *offset += size;

Endless loop if size ends up being zero? (If you do such verifications
at all, you should perhaps be as strict as possible, i.e. if size is
required to be a multiple of 4 [which I think it is], you should also
check that.)

> @@ -334,7 +384,40 @@ 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");
> +            return -EINVAL;
> +        }
> +
> +        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;
> +        }

So the immediately preceding error path used just "return" - why
are they different (and I'm not asking about the printk())?

> @@ -379,12 +462,35 @@ 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

Do you perhaps mean bogus instead of stale?

> +         * val is returned from get_ucode_from_buffer_amd. Since we already
> +         * succeeded in patch application, force error = 0
> +         */
> +        if ( offset < bufsize )
> +            error = 0;
> +        else if ( save_error )
>              error = save_error;

And still my question stands: Since you don't look at further containers
if you found a match and successfully applied the updated, why is this
change needed (or is the comment saying the wrong thing)?

>      }
>  
>      if ( save_error )
>      {
> +        /*
> +         * By the time 'microcode_init' runs, we have already updated the
> +         * patch level on all (currently) running cpus.
> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
> +         * Multiple containers are present && update succeeded with the
> +         * first container file itself.
> +         *
> +         * Only this time, there is no 'applied_offset' as well.
> +         * So, 'save_error' = 1. But error = -EINVAL.
> +         * Hence, this check is necessary to return 0 for success.
> +         */
> +        if ( (error != save_error) && (offset < bufsize) )
> +            error = 0;

Same for this change/comment.

Jan

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

[Aravind Gopalakrishnan]

On 6/27/2014 5:50 AM, Jan Beulich wrote:
>>>> On 25.06.14 at 21:34, <aravind.gopalakrishnan@amd.com> 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
>>
>> Other changes introduced:
>>   - Define HDR_SIZE's explicitly for code clarity.
>>
>> Changes in V3
>>   - 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
> Can you please get used to put this revision log after the first ---
> marker? It doesn't belong in ti actual commit message.

Okay.

>> @@ -272,14 +293,13 @@ 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)
>>   {
>> +    uint32_t *buf = (uint32_t *) (data + *offset);
> I know I complained about this or a similar pointless cast for the
> previous version.

Yes, I thought I'll handle it as a separate cleanup patch as it was not 
directly
related to the subject of the patch. But it's a small change, so I'll do 
this and include
it in the commit message as well..

>> +static int container_fast_forward(const void *data, size_t size_left, size_t *offset)
>> +{
>> +    size_t size;
>> +
>> +    while ( size_left )
>> +    {
>> +        if ( size_left < SECTION_HDR_SIZE )
>> +            return -EINVAL;
>> +
>> +        if ( *(const uint32_t *)(data + *offset) == UCODE_MAGIC &&
>> +             *(const uint32_t *)(data + *offset + 4) ==
>> +                                 UCODE_EQUIV_CPU_TABLE_TYPE )
>> +            break;
>> +
>> +        size = *(uint32_t *)(data + *offset + 4) + SECTION_HDR_SIZE;
> Now these three casts+derefs surely warrant a "const uint32_t *"
> variable, making the resulting code quite a bit easier to read.

Okay. Will just use the bits in V2 with a const qualifier

>> +        if ( size_left < size )
>> +            return -EINVAL;
>> +
>> +        size_left -= size;
>> +        *offset += size;
> Endless loop if size ends up being zero? (If you do such verifications
> at all, you should perhaps be as strict as possible,

Sorry about that (Again). Will fix.


> i.e. if size is
> required to be a multiple of 4 [which I think it is], you should also
> check that.)

I don't think that's a rule per-se.
#define F16H_MPB_MAX_SIZE 3458
Which is indivisible by 4.

>> @@ -334,7 +384,40 @@ 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");
>> +            return -EINVAL;
>> +        }
>> +
>> +        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;
>> +        }
> So the immediately preceding error path used just "return" - why
> are they different (and I'm not asking about the printk())?

Ok, Shall use one or the other.. (Probably the goto as I see that's the 
favored one in this function)


>> @@ -379,12 +462,35 @@ 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
> Do you perhaps mean bogus instead of stale?

Yes. Wrong word. Will fix.

>> +         * val is returned from get_ucode_from_buffer_amd. Since we already
>> +         * succeeded in patch application, force error = 0
>> +         */
>> +        if ( offset < bufsize )
>> +            error = 0;
>> +        else if ( save_error )
>>               error = save_error;
> And still my question stands: Since you don't look at further containers
> if you found a match and successfully applied the updated, why is this
> change needed (or is the comment saying the wrong thing)?

Maybe the comment needs to be more verbose(?)

Yes, we found a match and yes, we applied the patch successfully.
But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
and return -EINVAL
which is assigned to the variable 'error'
(Assuming ofc that there is a second container there which we don't need 
to parse because
we have already succeeded in patch application)

This is what I wanted to convey from

"astale  bogus error val is returned from get_ucode_from_buffer_amd."

But, we need to return 0 on success; which is why this change is needed 
here..

>
>>       }
>>   
>>       if ( save_error )
>>       {
>> +        /*
>> +         * By the time 'microcode_init' runs, we have already updated the
>> +         * patch level on all (currently) running cpus.
>> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
>> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
>> +         * Multiple containers are present && update succeeded with the
>> +         * first container file itself.
>> +         *
>> +         * Only this time, there is no 'applied_offset' as well.
>> +         * So, 'save_error' = 1. But error = -EINVAL.
>> +         * Hence, this check is necessary to return 0 for success.
>> +         */
>> +        if ( (error != save_error) && (offset < bufsize) )
>> +            error = 0;
> Same for this change/comment.
>

Hmm.. I'm having trouble trying to re-word this comment then..

Given the situation where  - we have already applied the patch update after
'microcode_presmp_init' and 'microcode_resume_cpu';
    |
    v
Now 'microcode_init' runs and calls into 'cpu_request_microcode';
    |
    v
We use 1st while loop to find_equiv_cpu_id() and match it with the container
    |
    v
For this particular case, we assume it's a match on the 1st container; 
so break
    |
    v
Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
    |
    v
At some point, it will find the correct patch; but this time there is no 
need to update
    |
    v
The behavior is now similar to what I have described above. i.e
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
bufsize,&offset)) == 0 )
is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
and return -EINVAL
which is assigned to the variable 'error'
    |
    v
But, now (as stated in the comment..)

* Only this time, there is no 'applied_offset' as well.
+         * So, 'save_error' = 1. But error = -EINVAL.

    |
    v
And since we need to return 0 for success, this change is needed here.

Thanks,
-Aravind

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

[Jan Beulich]

>>> On 27.06.14 at 19:07, <aravind.gopalakrishnan@amd.com> wrote:
> On 6/27/2014 5:50 AM, Jan Beulich wrote:
>>>>> On 25.06.14 at 21:34, <aravind.gopalakrishnan@amd.com> wrote:
>>> @@ -379,12 +462,35 @@ 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
>> Do you perhaps mean bogus instead of stale?
> 
> Yes. Wrong word. Will fix.
> 
>>> +         * val is returned from get_ucode_from_buffer_amd. Since we already
>>> +         * succeeded in patch application, force error = 0
>>> +         */
>>> +        if ( offset < bufsize )
>>> +            error = 0;
>>> +        else if ( save_error )
>>>               error = save_error;
>> And still my question stands: Since you don't look at further containers
>> if you found a match and successfully applied the updated, why is this
>> change needed (or is the comment saying the wrong thing)?
> 
> Maybe the comment needs to be more verbose(?)

It's not about the amount of comment, but about it needing to be
precise.

> Yes, we found a match and yes, we applied the patch successfully.
> But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
> and return -EINVAL
> which is assigned to the variable 'error'
> (Assuming ofc that there is a second container there which we don't need 
> to parse because
> we have already succeeded in patch application)
> 
> This is what I wanted to convey from
> 
> "astale  bogus error val is returned from get_ucode_from_buffer_amd."
> 
> But, we need to return 0 on success; which is why this change is needed 
> here..

I think I understand now: This talks about the case of a _subsequent_
container (never really looked at) following, causing the unwanted
-EINVAL. Whereas your comment said "earlier container", implying (to
me) that it talks about one that earlier code did look at.

>>>       }
>>>   
>>>       if ( save_error )
>>>       {
>>> +        /*
>>> +         * By the time 'microcode_init' runs, we have already updated the
>>> +         * patch level on all (currently) running cpus.
>>> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
>>> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
>>> +         * Multiple containers are present && update succeeded with the
>>> +         * first container file itself.
>>> +         *
>>> +         * Only this time, there is no 'applied_offset' as well.
>>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>> +         * Hence, this check is necessary to return 0 for success.
>>> +         */
>>> +        if ( (error != save_error) && (offset < bufsize) )
>>> +            error = 0;
>> Same for this change/comment.
>>
> 
> Hmm.. I'm having trouble trying to re-word this comment then..
> 
> Given the situation where  - we have already applied the patch update after
> 'microcode_presmp_init' and 'microcode_resume_cpu';
>     |
>     v
> Now 'microcode_init' runs and calls into 'cpu_request_microcode';
>     |
>     v
> We use 1st while loop to find_equiv_cpu_id() and match it with the container
>     |
>     v
> For this particular case, we assume it's a match on the 1st container; 
> so break
>     |
>     v
> Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
>     |
>     v
> At some point, it will find the correct patch; but this time there is no 
> need to update
>     |
>     v
> The behavior is now similar to what I have described above. i.e
> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, 
> bufsize,&offset)) == 0 )
> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE ) 
> and return -EINVAL
> which is assigned to the variable 'error'
>     |
>     v
> But, now (as stated in the comment..)
> 
> * Only this time, there is no 'applied_offset' as well.
> +         * So, 'save_error' = 1. But error = -EINVAL.
> 
>     |
>     v
> And since we need to return 0 for success, this change is needed here.

So since this is similar to the previous comment, rather than
duplicating information, perhaps just refer to the earlier one, adding
_only_ the information of the different aspect(s) here. And use the
right words: To me at least the "Only this time" implies something
different than what I think you mean - "Except that this time ..."
would be the words I'd use (but a native English speaker may need
to be consulted in case you view this differently).

Jan

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

[Aravind Gopalakrishnan]

On 6/30/2014 4:32 AM, Jan Beulich wrote:
>> Yes, we found a match and yes, we applied the patch successfully.
>> But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE )
>> and return -EINVAL
>> which is assigned to the variable 'error'
>> (Assuming ofc that there is a second container there which we don't need
>> to parse because
>> we have already succeeded in patch application)
>>
>> This is what I wanted to convey from
>>
>> "astale  bogus error val is returned from get_ucode_from_buffer_amd."
>>
>> But, we need to return 0 on success; which is why this change is needed
>> here..
> I think I understand now: This talks about the case of a _subsequent_
> container (never really looked at) following, causing the unwanted
> -EINVAL. Whereas your comment said "earlier container", implying (to
> me) that it talks about one that earlier code did look at.
>
>>>>        }
>>>>    
>>>>        if ( save_error )
>>>>        {
>>>> +        /*
>>>> +         * By the time 'microcode_init' runs, we have already updated the
>>>> +         * patch level on all (currently) running cpus.
>>>> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
>>>> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
>>>> +         * Multiple containers are present && update succeeded with the
>>>> +         * first container file itself.
>>>> +         *
>>>> +         * Only this time, there is no 'applied_offset' as well.
>>>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>>> +         * Hence, this check is necessary to return 0 for success.
>>>> +         */
>>>> +        if ( (error != save_error) && (offset < bufsize) )
>>>> +            error = 0;
>>> Same for this change/comment.
>>>
>> Hmm.. I'm having trouble trying to re-word this comment then..
>>
>> Given the situation where  - we have already applied the patch update after
>> 'microcode_presmp_init' and 'microcode_resume_cpu';
>>      |
>>      v
>> Now 'microcode_init' runs and calls into 'cpu_request_microcode';
>>      |
>>      v
>> We use 1st while loop to find_equiv_cpu_id() and match it with the container
>>      |
>>      v
>> For this particular case, we assume it's a match on the 1st container;
>> so break
>>      |
>>      v
>> Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>>      |
>>      v
>> At some point, it will find the correct patch; but this time there is no
>> need to update
>>      |
>>      v
>> The behavior is now similar to what I have described above. i.e
>> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE )
>> and return -EINVAL
>> which is assigned to the variable 'error'
>>      |
>>      v
>> But, now (as stated in the comment..)
>>
>> * Only this time, there is no 'applied_offset' as well.
>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>
>>      |
>>      v
>> And since we need to return 0 for success, this change is needed here.
> So since this is similar to the previous comment, rather than
> duplicating information, perhaps just refer to the earlier one, adding
> _only_ the information of the different aspect(s) here. And use the
> right words: To me at least the "Only this time" implies something
> different than what I think you mean - "Except that this time ..."
> would be the words I'd use (but a native English speaker may need
> to be consulted in case you view this differently).
>
>

Ok. I have followed a similar step-wise approach  (think this is simpler 
to follow) in the comments
for V4. Hopefully it's more precisely worded..

I am also working to document container file format, where I will also 
talk about why a correct patch can
be on only one or the other container, but not both.

Thanks,
-Aravind.

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

[Boris Ostrovsky]

On 06/27/2014 04:15 AM, Jan Beulich wrote:
>>>> On 26.06.14 at 22:00, <boris.ostrovsky@oracle.com> wrote:
>> The trouble is that if I have two files in my hands, both called
>> microcode_amd_fam15h.bin, I don't know which one is the more up-to-date
>> (would be nice to have a tool to print file info).
>>
>> Of course, one can argue that in that case I shouldn't be using neither
>> but HW makes its own verification of patches so in principle trying both
>> should be safe (provided that SW tries not to load older revision).
> I think we should focus on getting things to work with realistic
> blobs, i.e. such that distros and their tools would generate. If you
> update a respective package, it'll be clear which blob is the latest,
> and there's generally not going to be two blobs for the same family
> put on a single system. People obtaining ucode blobs manually from
> elsewhere should know what they're doing and not try to paste
> together two of them targeting the same family.

Ok.

I only mention this scenario because I myself needed to deal with this 
(on more than one occasion, in fact) when I had two containers and 
didn't know which one is which.

-boris

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

[Jan Beulich]

>>> On 26.06.14 at 22:00, <boris.ostrovsky@oracle.com> wrote:
> The trouble is that if I have two files in my hands, both called 
> microcode_amd_fam15h.bin, I don't know which one is the more up-to-date 
> (would be nice to have a tool to print file info).
> 
> Of course, one can argue that in that case I shouldn't be using neither 
> but HW makes its own verification of patches so in principle trying both 
> should be safe (provided that SW tries not to load older revision).

I think we should focus on getting things to work with realistic
blobs, i.e. such that distros and their tools would generate. If you
update a respective package, it'll be clear which blob is the latest,
and there's generally not going to be two blobs for the same family
put on a single system. People obtaining ucode blobs manually from
elsewhere should know what they're doing and not try to paste
together two of them targeting the same family.

Jan

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

[Boris Ostrovsky]

On 06/26/2014 05:55 PM, Aravind Gopalakrishnan wrote:
> On 6/26/2014 3:00 PM, Boris Ostrovsky wrote:
>>
>>>
>>> The process(,scripts) to create containers ensure that we only have 
>>> patches for f10h-f14h on 'microcode_amd.bin'
>>> and patches for f15h models (could be different model/stepping 
>>> values) on microcode_amd_fam15h.bin
>>>
>>> So it is safe to assume that we we only have a matching equiv_id in 
>>> only one of the containers.
>>
>> The trouble is that if I have two files in my hands, both called 
>> microcode_amd_fam15h.bin, I don't know which one is the more 
>> up-to-date (would be nice to have a tool to print file info).
>>
>> Of course, one can argue that in that case I shouldn't be using 
>> neither but HW makes its own verification of patches so in principle 
>> trying both should be safe (provided that SW tries not to load older 
>> revision).
>>
> Hmm, yeah... But this is a pretty contrived case and I'm still not 
> completely convinced this is something we should handle..
> Here are my thoughts:
> - If such a case arises, one can just pull the latest containers which 
> are published
>   - here: 
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode 
>
>   - and here: http://www.amd64.org/microcode.html
>   - distros also keep themselves updated by pulling the binaries from 
> above links
> - We can add documentation notes stating to the effect that 
> users(sysadmins) are better off not concatenating two containers of 
> same processor family.
>   - It won't break anything, but you just might end up on a 
> not-so-latest microcode patch level


AFAIK the code always picks the latest version of the patch.


>   - And also point to above links to resolve such deadlock situations
> - If you would still prefer that we handle this case, then we probably 
> should not do it as you described.
>   - The proper way would be to go back to the start of the 1st while 
> loop.
>     - Because we can go back to find_equiv_cpu_id() and take things 
> from there again..
>     - So, more logic rework..
>  (I'm ok with re-working the code, but do let me know if this a case 
> we should care for..)

I think it's worth doing.

-boris

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

[Aravind Gopalakrishnan]

On 6/26/2014 3:00 PM, Boris Ostrovsky wrote:
>
>>
>> The process(,scripts) to create containers ensure that we only have 
>> patches for f10h-f14h on 'microcode_amd.bin'
>> and patches for f15h models (could be different model/stepping 
>> values) on microcode_amd_fam15h.bin
>>
>> So it is safe to assume that we we only have a matching equiv_id in 
>> only one of the containers.
>
> The trouble is that if I have two files in my hands, both called 
> microcode_amd_fam15h.bin, I don't know which one is the more 
> up-to-date (would be nice to have a tool to print file info).
>
> Of course, one can argue that in that case I shouldn't be using 
> neither but HW makes its own verification of patches so in principle 
> trying both should be safe (provided that SW tries not to load older 
> revision).
>
Hmm, yeah... But this is a pretty contrived case and I'm still not 
completely convinced this is something we should handle..
Here are my thoughts:
- If such a case arises, one can just pull the latest containers which 
are published
   - here: 
https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/amd-ucode 

   - and here: http://www.amd64.org/microcode.html
   - distros also keep themselves updated by pulling the binaries from 
above links
- We can add documentation notes stating to the effect that 
users(sysadmins) are better off not concatenating two containers of same 
processor family.
   - It won't break anything, but you just might end up on a 
not-so-latest microcode patch level
   - And also point to above links to resolve such deadlock situations
- If you would still prefer that we handle this case, then we probably 
should not do it as you described.
   - The proper way would be to go back to the start of the 1st while loop.
     - Because we can go back to find_equiv_cpu_id() and take things 
from there again..
     - So, more logic rework..
  (I'm ok with re-working the code, but do let me know if this a case we 
should care for..)

Thanks,
-Aravind.

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

[Boris Ostrovsky]

On 06/26/2014 12:37 PM, Aravind Gopalakrishnan wrote:
> On 6/26/2014 11:01 AM, Boris Ostrovsky wrote:
>>
>>             +
>>             +        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;
>>             +        }
>>             +    }
>>             +
>>
>>
>>         I just realized that I don't understand something: if you have
>>         two merged container files, both having an entry for current
>>         processor in the equivalence table but only the second file
>>         having the actual patch (or at least the patch with higher
>>         version), will we get to load that patch?
>>
>>
>>     We will not come across such a situation:
>>     One container has patch files for families 10h,11h,12h,14h and the
>>     other has patches for 15h.
>>     So there will be an equiv_cpu_id match in (at least and) only
>>     *one* of the containers.
>>     (If there ever are patches for 16h, then I suspect it will have a
>>     separate container of it's own. So there will not be any clashes
>>     with older containers)
>>
>>     Besides, the patches themselves are not incremental, i.e;
>>     If there is a newer patch, then it handles all errata/bugs that
>>     were handled by the previous patch and then some.
>>     Which means, you will not have a container that has two patches
>>     for the same processor.
>>     So, *strictly speaking*, even the loop to
>>     get_ucode_from_buffer_amd() till the end of buffer is not
>>     necessary once we
>>     have already applied a patch.
>>
>>     But, to change this behavior now will need more logic rework..
>>
>>
>> I don't think there is a whole lot of work/testing (especially since 
>> I am not the one doing it ;-)) --- you just need to skip header and 
>> equivalence table of the next container.
>>
>> if ( mpbuf->type != UCODE_UCODE_TYPE )
>> {
>>      if ( *(const uint32_t *)buf == UCODE_MAGIC )
>>      {
>>          struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[2]; // or 
>> [1]?
> Neither will work as we will need to first advance buf 4 bytes.. (but 
> I get your point)
> Regardless, this is not necessary.
> Let me clarify (below)
>>          *offset += mpbuf->len + CONT_HDR_SIZE + 4;  // I think 4, to 
>> skip MAGIC
>>                                                      // and next word.
>>          // Now we've fast-forwarded past header and eq. table
>>          return 0; // or, goto beginning?
>>      }
>> }
>>
>> Will that work?
>
> But we don't actually need to parse the second container for reasons 
> given in the previous thread.
>
> Example scenarios-
> 1. Order of containers in initrd: f15h container, container for 
> remaining families.
>   - equiv_cpu_id match for first container (surely you are on a f15h 
> model)
>     - parse this container, find correct patch and try to apply
>     - No need to parse second container as it will not have patches 
> for f15h.
>   - no match for 1st container, but match on 2nd, then current 
> processor is in set [f10h,f14h]
>     - parse this container, find correct patch and try to apply
>   - no match on both.
>    - borked containers. So, abort.
>
> 2. Order of containers: container for families from f10h-f14h; 
> container for f15h
>   - equiv_cpu_id match for 1st container (surely you are on some 
> family in the set of [f10h, f14h])
>     - Parse container, find patch, try to apply
>     - No need to parse second container as it will not have patches 
> for f10h-f14h
>  - found match on 2nd, then current processor is a f15h model,
>     - parse container, find patch, try to apply
>  - no match
>     - borked container. So abort.
>
> The process(,scripts) to create containers ensure that we only have 
> patches for f10h-f14h on 'microcode_amd.bin'
> and patches for f15h models (could be different model/stepping values) 
> on microcode_amd_fam15h.bin
>
> So it is safe to assume that we we only have a matching equiv_id in 
> only one of the containers.

The trouble is that if I have two files in my hands, both called 
microcode_amd_fam15h.bin, I don't know which one is the more up-to-date 
(would be nice to have a tool to print file info).

Of course, one can argue that in that case I shouldn't be using neither 
but HW makes its own verification of patches so in principle trying both 
should be safe (provided that SW tries not to load older revision).

-boris

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

[Aravind Gopalakrishnan]

On 6/26/2014 11:01 AM, Boris Ostrovsky wrote:
>
>             +
>             +        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;
>             +        }
>             +    }
>             +
>
>
>         I just realized that I don't understand something: if you have
>         two merged container files, both having an entry for current
>         processor in the equivalence table but only the second file
>         having the actual patch (or at least the patch with higher
>         version), will we get to load that patch?
>
>
>     We will not come across such a situation:
>     One container has patch files for families 10h,11h,12h,14h and the
>     other has patches for 15h.
>     So there will be an equiv_cpu_id match in (at least and) only
>     *one* of the containers.
>     (If there ever are patches for 16h, then I suspect it will have a
>     separate container of it's own. So there will not be any clashes
>     with older containers)
>
>     Besides, the patches themselves are not incremental, i.e;
>     If there is a newer patch, then it handles all errata/bugs that
>     were handled by the previous patch and then some.
>     Which means, you will not have a container that has two patches
>     for the same processor.
>     So, *strictly speaking*, even the loop to
>     get_ucode_from_buffer_amd() till the end of buffer is not
>     necessary once we
>     have already applied a patch.
>
>     But, to change this behavior now will need more logic rework..
>
>
> I don't think there is a whole lot of work/testing (especially since I 
> am not the one doing it ;-)) --- you just need to skip header and 
> equivalence table of the next container.
>
> if ( mpbuf->type != UCODE_UCODE_TYPE )
> {
>      if ( *(const uint32_t *)buf == UCODE_MAGIC )
>      {
>          struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[2]; // or [1]?
Neither will work as we will need to first advance buf 4 bytes.. (but I 
get your point)
Regardless, this is not necessary.
Let me clarify (below)
>          *offset += mpbuf->len + CONT_HDR_SIZE + 4;  // I think 4, to skip MAGIC
>                                                      // and next word.
>          // Now we've fast-forwarded past header and eq. table
>          return 0; // or, goto beginning?
>      }
> }
>
> Will that work?

But we don't actually need to parse the second container for reasons 
given in the previous thread.

Example scenarios-
1. Order of containers in initrd: f15h container, container for 
remaining families.
   - equiv_cpu_id match for first container (surely you are on a f15h model)
     - parse this container, find correct patch and try to apply
     - No need to parse second container as it will not have patches for 
f15h.
   - no match for 1st container, but match on 2nd, then current 
processor is in set [f10h,f14h]
     - parse this container, find correct patch and try to apply
   - no match on both.
    - borked containers. So, abort.

2. Order of containers: container for families from f10h-f14h; container 
for f15h
   - equiv_cpu_id match for 1st container (surely you are on some family 
in the set of [f10h, f14h])
     - Parse container, find patch, try to apply
     - No need to parse second container as it will not have patches for 
f10h-f14h
  - found match on 2nd, then current processor is a f15h model,
     - parse container, find patch, try to apply
  - no match
     - borked container. So abort.

The process(,scripts) to create containers ensure that we only have 
patches for f10h-f14h on 'microcode_amd.bin'
and patches for f15h models (could be different model/stepping values) 
on microcode_amd_fam15h.bin

So it is safe to assume that we we only have a matching equiv_id in only 
one of the containers.

-Aravind.

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

[Boris Ostrovsky]

[-- Attachment #1.1: Type: text/plain, Size: 3653 bytes --]

On 06/26/2014 11:03 AM, Aravind Gopalakrishnan wrote:

    On 6/26/2014 8:14 AM, Boris Ostrovsky wrote:


            +    /*
            +     * 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");
            +            return -EINVAL;
            +        }
            +
            +        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;
            +        }
            +    }
            +


        I just realized that I don't understand something: if you have
        two merged container files, both having an entry for current
        processor in the equivalence table but only the second file
        having the actual patch (or at least the patch with higher
        version), will we get to load that patch?


    We will not come across such a situation:
    One container has patch files for families 10h,11h,12h,14h and the
    other has patches for 15h.
    So there will be an equiv_cpu_id match in (at least and) only *one*
    of the containers.
    (If there ever are patches for 16h, then I suspect it will have a
    separate container of it's own. So there will not be any clashes
    with older containers)

    Besides, the patches themselves are not incremental, i.e;
    If there is a newer patch, then it handles all errata/bugs that were
    handled by the previous patch and then some.
    Which means, you will not have a container that has two patches for
    the same processor.
    So, *strictly speaking*, even the loop to
    get_ucode_from_buffer_amd() till the end of buffer is not necessary
    once we
    have already applied a patch.

    But, to change this behavior now will need more logic rework..


I don't think there is a whole lot of work/testing (especially since I 
am not the one doing it ;-)) --- you just need to skip header and 
equivalence table of the next container.

if ( mpbuf->type != UCODE_UCODE_TYPE )
{
     if ( *(const uint32_t *)buf == UCODE_MAGIC )
     {
         struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[2]; // or [1]?
         *offset += mpbuf->len + CONT_HDR_SIZE + 4;  // I think 4, to skip MAGIC
                                                     // and next word.
         // Now we've fast-forwarded past header and eq. table
         return 0; // or, goto beginning?
     }
}


Will that work?

    Then again, if it works fine right now, would we really want to
    change this behavior?

    Thanks,
    -Aravind.



[-- Attachment #1.2: Type: text/html, Size: 5347 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel