xen/ucode: Improve commentary for parsing AMD containers

Commit Message

Andrew Cooper Sept. 13, 2024, 12:39 p.m. UTC

Despite writing this code, it's not the easiest logic to follow.

Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of
what's going on.

No functional change.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
---
 xen/arch/x86/cpu/microcode/amd.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Comments

Jan Beulich Sept. 13, 2024, 5:56 p.m. UTC | #1

On 13.09.2024 14:39, Andrew Cooper wrote:
> Despite writing this code, it's not the easiest logic to follow.
> 
> Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of
> what's going on.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <[e-mail]>

I'm okay with this as is, so
Acked-by: Jan Beulich <[e-mail]>
yet ...

> @@ -335,10 +335,10 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>          buf  += 4;
>          size -= 4;
>  
> -        if ( size < sizeof(*et) ||
> -             (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
> -             size - sizeof(*et) < et->len ||
> -             et->len % sizeof(et->eq[0]) )
> +        if ( size < sizeof(*et) ||                   /* Insufficient space for header? */
> +             (et = buf)->type != UCODE_EQUIV_TYPE || /* Not an Equivalence Table? */
> +             size - sizeof(*et) < et->len ||         /* Insufficient space for table? */
> +             et->len % sizeof(et->eq[0]) )           /* Not a multiple of equiv_cpu_entry? */

... this of course goes quite a bit beyond 80 cols (yet worse for the
other block further down). How about


        if ( /* Insufficient space for header? */
             size < sizeof(*et) ||
             /* Not an Equivalence Table? */
             (et = buf)->type != UCODE_EQUIV_TYPE ||
             /* Insufficient space for table? */
             size - sizeof(*et) < et->len ||
             /* Not a multiple of equiv_cpu_entry? */
             et->len % sizeof(et->eq[0]) )

?

Jan

Patch

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 32490c8b7d2a..35bcec643c04 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -52,11 +52,11 @@  struct microcode_patch {
 };
 
 #define UCODE_MAGIC                0x00414d44
-#define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
+#define UCODE_EQUIV_TYPE           0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
 struct container_equiv_table {
-    uint32_t type; /* UCODE_EQUIV_CPU_TABLE_TYPE */
+    uint32_t type; /* UCODE_EQUIV_TYPE */
     uint32_t len;
     struct equiv_cpu_entry eq[];
 };
@@ -335,10 +335,10 @@  static struct microcode_patch *cf_check cpu_request_microcode(
         buf  += 4;
         size -= 4;
 
-        if ( size < sizeof(*et) ||
-             (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
-             size - sizeof(*et) < et->len ||
-             et->len % sizeof(et->eq[0]) )
+        if ( size < sizeof(*et) ||                   /* Insufficient space for header? */
+             (et = buf)->type != UCODE_EQUIV_TYPE || /* Not an Equivalence Table? */
+             size - sizeof(*et) < et->len ||         /* Insufficient space for table? */
+             et->len % sizeof(et->eq[0]) )           /* Not a multiple of equiv_cpu_entry? */
         {
             printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n");
             error = -EINVAL;
@@ -351,7 +351,12 @@  static struct microcode_patch *cf_check cpu_request_microcode(
 
         error = scan_equiv_cpu_table(et);
 
-        /* -ESRCH means no applicable microcode in this container. */
+        /*
+         * -ESRCH means no applicable microcode in this container.  But, there
+         * might be subsequent containers in the blob.  Skipping to the end of
+         * this container still requires us to follow the UCODE_UCODE_TYPE/len
+         * metadata because there's no overall container length given.
+         */
         if ( error && error != -ESRCH )
             break;
         skip_ucode = error;
@@ -361,10 +366,10 @@  static struct microcode_patch *cf_check cpu_request_microcode(
         {
             const struct container_microcode *mc;
 
-            if ( size < sizeof(*mc) ||
-                 (mc = buf)->type != UCODE_UCODE_TYPE ||
-                 size - sizeof(*mc) < mc->len ||
-                 mc->len < sizeof(struct microcode_patch) )
+            if ( size < sizeof(*mc) ||                      /* Insufficient space for container header? */
+                 (mc = buf)->type != UCODE_UCODE_TYPE ||    /* Not an ucode blob? */
+                 size - sizeof(*mc) < mc->len ||            /* Insufficient space for blob? */
+                 mc->len < sizeof(struct microcode_patch) ) /* Insufficient space for patch header? */
             {
                 printk(XENLOG_ERR "microcode: Bad microcode data\n");
                 error = -EINVAL;