[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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@xxxxxxx>
---
  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;
      }


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.