[PATCH 0/5] x86/ucode: Minor cleanup

[PATCH 0/5] x86/ucode: Minor cleanup

[Andrew Cooper]

Misc cleanup without functional change.  Mostly to improve clarity.

Andrew Cooper (5):
  x86/ucode: Rename hypercall-context functions
  x86/ucode: Drop the parse_blob() wrapper
  x86/ucode: Rename the cpu_request_microcode() hook to parse()
  x86/ucode: Rename the apply_microcode() hook to load()
  x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

 xen/arch/x86/cpu/microcode/amd.c     | 11 ++---
 xen/arch/x86/cpu/microcode/core.c    | 72 +++++++++++++++-------------
 xen/arch/x86/cpu/microcode/intel.c   | 12 ++---
 xen/arch/x86/cpu/microcode/private.h |  9 ++--
 xen/arch/x86/include/asm/microcode.h |  4 +-
 xen/arch/x86/platform_hypercall.c    |  6 +--
 6 files changed, 59 insertions(+), 55 deletions(-)


base-commit: a974725a87a1afc8056b41c56dfe7fe272a7169c
-- 
2.39.5

[PATCH 1/5] x86/ucode: Rename hypercall-context functions

[Andrew Cooper]

microcode_update{,_helper}() are overly generic names in a file that has
multiple update routines and helper functions contexts.

Rename microcode_update() to ucode_update_hcall() so it explicitly identifies
itself as hypercall context, and rename microcode_update_helper() to
ucode_update_hcall_cont() to make it clear it is in continuation context.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c    | 10 +++++-----
 xen/arch/x86/include/asm/microcode.h |  4 ++--
 xen/arch/x86/platform_hypercall.c    |  6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 1d58cb0f3bc1..21077b449c38 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -586,7 +586,7 @@ struct ucode_buf {
     char buffer[];
 };
 
-static long cf_check microcode_update_helper(void *data)
+static long cf_check ucode_update_hcall_cont(void *data)
 {
     int ret;
     struct ucode_buf *buffer = data;
@@ -722,8 +722,8 @@ static long cf_check microcode_update_helper(void *data)
     return ret;
 }
 
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
-                     unsigned long len, unsigned int flags)
+int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
+                       unsigned long len, unsigned int flags)
 {
     int ret;
     struct ucode_buf *buffer;
@@ -748,11 +748,11 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
     buffer->flags = flags;
 
     /*
-     * Always queue microcode_update_helper() on CPU0.  Most of the logic
+     * Always queue ucode_update_hcall_cont() on CPU0.  Most of the logic
      * won't care, but the update of the Raw CPU policy wants to (re)run on
      * the BSP.
      */
-    return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
+    return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
 }
 
 static int __init cf_check microcode_init(void)
diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h
index a278773f8b5d..dd20cdb9ebb6 100644
--- a/xen/arch/x86/include/asm/microcode.h
+++ b/xen/arch/x86/include/asm/microcode.h
@@ -22,8 +22,8 @@ struct cpu_signature {
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 
 void microcode_set_module(unsigned int idx);
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
-                     unsigned long len, unsigned int flags);
+int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
+                       unsigned long len, unsigned int flags);
 int microcode_update_one(void);
 
 struct boot_info;
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 67f851237def..90abd3197fc9 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -313,7 +313,7 @@ ret_t do_platform_op(
 
         guest_from_compat_handle(data, op->u.microcode.data);
 
-        ret = microcode_update(data, op->u.microcode.length, 0);
+        ret = ucode_update_hcall(data, op->u.microcode.length, 0);
         break;
     }
 
@@ -323,8 +323,8 @@ ret_t do_platform_op(
 
         guest_from_compat_handle(data, op->u.microcode2.data);
 
-        ret = microcode_update(data, op->u.microcode2.length,
-                               op->u.microcode2.flags);
+        ret = ucode_update_hcall(data, op->u.microcode2.length,
+                                 op->u.microcode2.flags);
         break;
     }
 
-- 
2.39.5

Re: [PATCH 1/5] x86/ucode: Rename hypercall-context functions

[Jason Andryuk]

On 2024-10-24 09:22, Andrew Cooper wrote:
> microcode_update{,_helper}() are overly generic names in a file that has
> multiple update routines and helper functions contexts.
> 
> Rename microcode_update() to ucode_update_hcall() so it explicitly identifies
> itself as hypercall context, and rename microcode_update_helper() to
> ucode_update_hcall_cont() to make it clear it is in continuation context.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

[PATCH 2/5] x86/ucode: Drop the parse_blob() wrapper

[Andrew Cooper]

This separates the collect_cpu_info() and cpu_request_microcode() calls for
later cleanup, and frees up the name to be reused a little differently.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/core.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 21077b449c38..cad38d859eee 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -237,19 +237,6 @@ static struct patch_with_flags nmi_patch =
     .patch  = ZERO_BLOCK_PTR,
 };
 
-/*
- * Return a patch that covers current CPU. If there are multiple patches,
- * return the one with the highest revision number. Return error If no
- * patch is found and an error occurs during the parsing process. Otherwise
- * return NULL.
- */
-static struct microcode_patch *parse_blob(const char *buf, size_t len)
-{
-    alternative_vcall(ucode_ops.collect_cpu_info);
-
-    return alternative_call(ucode_ops.cpu_request_microcode, buf, len, true);
-}
-
 static void microcode_free_patch(const struct microcode_patch *patch)
 {
     xfree((struct microcode_patch *)patch);
@@ -616,7 +603,10 @@ static long cf_check ucode_update_hcall_cont(void *data)
         goto put;
     }
 
-    patch_with_flags.patch = parse_blob(buffer->buffer, buffer->len);
+    alternative_vcall(ucode_ops.collect_cpu_info);
+    patch_with_flags.patch = alternative_call(ucode_ops.cpu_request_microcode,
+                                              (const void *)buffer->buffer,
+                                              buffer->len, true);
     patch_with_flags.flags = buffer->flags;
     xfree(buffer);
     if ( IS_ERR(patch_with_flags.patch) )
@@ -797,7 +787,8 @@ static int __init early_update_cache(const void *data, size_t len)
     if ( !data )
         return -ENOMEM;
 
-    patch = parse_blob(data, len);
+    alternative_vcall(ucode_ops.collect_cpu_info);
+    patch = alternative_call(ucode_ops.cpu_request_microcode, data, len, true);
     if ( IS_ERR(patch) )
     {
         printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
-- 
2.39.5

Re: [PATCH 2/5] x86/ucode: Drop the parse_blob() wrapper

[Jason Andryuk]

On 2024-10-24 09:22, Andrew Cooper wrote:
> This separates the collect_cpu_info() and cpu_request_microcode() calls for
> later cleanup, and frees up the name to be reused a little differently.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

[PATCH 3/5] x86/ucode: Rename the cpu_request_microcode() hook to parse()

[Andrew Cooper]

cpu_request_microcode() was never a good name, and the microcode suffix is
redundant.  Rename it to simply parse().

Introduce ucode_parse() and ucode_parse_dup() wrappers around the parse()
hook, also abstracting away the make_copy parameter and associated
const-correctness.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c     |  4 ++--
 xen/arch/x86/cpu/microcode/core.c    | 22 ++++++++++++++++------
 xen/arch/x86/cpu/microcode/intel.c   |  4 ++--
 xen/arch/x86/cpu/microcode/private.h |  2 +-
 4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 0fe869eff119..3f147c10ca67 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -311,7 +311,7 @@ static int scan_equiv_cpu_table(const struct container_equiv_table *et)
     return -ESRCH;
 }
 
-static struct microcode_patch *cf_check cpu_request_microcode(
+static struct microcode_patch *cf_check amd_ucode_parse(
     const void *buf, size_t size, bool make_copy)
 {
     const struct microcode_patch *saved = NULL;
@@ -444,7 +444,7 @@ static struct microcode_patch *cf_check cpu_request_microcode(
 }
 
 static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
-    .cpu_request_microcode            = cpu_request_microcode,
+    .parse                            = amd_ucode_parse,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
     .compare_patch                    = compare_patch,
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index cad38d859eee..29655a44ae62 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -217,6 +217,18 @@ static void __init microcode_grab_module(struct boot_info *bi)
 
 static struct microcode_ops __ro_after_init ucode_ops;
 
+/* Parse a ucode blob.  Returns a pointer to a heap-allocated copy, or PTR_ERR. */
+static struct microcode_patch *ucode_parse_dup(const char *buf, size_t len)
+{
+    return alternative_call(ucode_ops.parse, buf, len, true);
+}
+
+/* Parse a ucode blob.  Returns a pointer into @buf, or PTR_ERR. */
+static const struct microcode_patch *ucode_parse(const char *buf, size_t len)
+{
+    return alternative_call(ucode_ops.parse, buf, len, false);
+}
+
 static DEFINE_SPINLOCK(microcode_mutex);
 
 DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
@@ -604,9 +616,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
     }
 
     alternative_vcall(ucode_ops.collect_cpu_info);
-    patch_with_flags.patch = alternative_call(ucode_ops.cpu_request_microcode,
-                                              (const void *)buffer->buffer,
-                                              buffer->len, true);
+    patch_with_flags.patch = ucode_parse_dup(buffer->buffer, buffer->len);
     patch_with_flags.flags = buffer->flags;
     xfree(buffer);
     if ( IS_ERR(patch_with_flags.patch) )
@@ -788,7 +798,7 @@ static int __init early_update_cache(const void *data, size_t len)
         return -ENOMEM;
 
     alternative_vcall(ucode_ops.collect_cpu_info);
-    patch = alternative_call(ucode_ops.cpu_request_microcode, data, len, true);
+    patch = ucode_parse_dup(data, len);
     if ( IS_ERR(patch) )
     {
         printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
@@ -832,7 +842,7 @@ static int __init early_microcode_update_cpu(void)
 {
     const void *data = NULL;
     size_t len;
-    struct microcode_patch *patch;
+    const struct microcode_patch *patch;
 
     if ( ucode_blob.size )
     {
@@ -848,7 +858,7 @@ static int __init early_microcode_update_cpu(void)
     if ( !data )
         return -ENOMEM;
 
-    patch = ucode_ops.cpu_request_microcode(data, len, false);
+    patch = ucode_parse(data, len);
     if ( IS_ERR(patch) )
     {
         printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index bad51f64724a..3d3f7e57db80 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -334,7 +334,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch,
     return 0;
 }
 
-static struct microcode_patch *cf_check cpu_request_microcode(
+static struct microcode_patch *cf_check intel_ucode_parse(
     const void *buf, size_t size, bool make_copy)
 {
     int error = 0;
@@ -406,7 +406,7 @@ static bool __init can_load_microcode(void)
 }
 
 static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
-    .cpu_request_microcode            = cpu_request_microcode,
+    .parse                            = intel_ucode_parse,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
     .compare_patch                    = compare_patch,
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index c72f060ac394..e30acee1536b 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -37,7 +37,7 @@ struct microcode_ops {
      * If one is not found, (nothing matches the current CPU), return NULL.
      * Also may return ERR_PTR(-err), e.g. bad container, out of memory.
      */
-    struct microcode_patch *(*cpu_request_microcode)(
+    struct microcode_patch *(*parse)(
         const void *buf, size_t size, bool make_copy);
 
     /*
-- 
2.39.5

Re: [PATCH 3/5] x86/ucode: Rename the cpu_request_microcode() hook to parse()

[Jason Andryuk]

On 2024-10-24 09:22, Andrew Cooper wrote:
> cpu_request_microcode() was never a good name, and the microcode suffix is
> redundant.  Rename it to simply parse().
> 
> Introduce ucode_parse() and ucode_parse_dup() wrappers around the parse()
> hook, also abstracting away the make_copy parameter and associated
> const-correctness.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

[PATCH 4/5] x86/ucode: Rename the apply_microcode() hook to load()

[Andrew Cooper]

The microcode suffix is redundant, and "microcode loading" is the more common
term
---
 xen/arch/x86/cpu/microcode/amd.c     |  6 +++---
 xen/arch/x86/cpu/microcode/core.c    | 27 ++++++++++++++++-----------
 xen/arch/x86/cpu/microcode/intel.c   |  8 ++++----
 xen/arch/x86/cpu/microcode/private.h |  7 +++----
 4 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 3f147c10ca67..1845f51ba330 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -214,8 +214,8 @@ static enum microcode_match_result cf_check compare_patch(
     return compare_header(new, old);
 }
 
-static int cf_check apply_microcode(const struct microcode_patch *patch,
-                                    unsigned int flags)
+static int cf_check amd_ucode_load(const struct microcode_patch *patch,
+                                   unsigned int flags)
 {
     int hw_err;
     unsigned int cpu = smp_processor_id();
@@ -446,7 +446,7 @@ static struct microcode_patch *cf_check amd_ucode_parse(
 static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
     .parse                            = amd_ucode_parse,
     .collect_cpu_info                 = collect_cpu_info,
-    .apply_microcode                  = apply_microcode,
+    .load                             = amd_ucode_load,
     .compare_patch                    = compare_patch,
 };
 
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 29655a44ae62..f320ea87c1dc 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -229,6 +229,12 @@ static const struct microcode_patch *ucode_parse(const char *buf, size_t len)
     return alternative_call(ucode_ops.parse, buf, len, false);
 }
 
+/* Load a ucode blob.  Returns -errno. */
+static int ucode_load(const struct microcode_patch *patch, unsigned int flags)
+{
+    return alternative_call(ucode_ops.load, patch, flags);
+}
+
 static DEFINE_SPINLOCK(microcode_mutex);
 
 DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
@@ -333,11 +339,10 @@ static int microcode_update_cpu(const struct microcode_patch *patch,
 
     spin_lock(&microcode_mutex);
     if ( patch )
-        err = alternative_call(ucode_ops.apply_microcode, patch, flags);
+        err = ucode_load(patch, flags);
     else if ( microcode_cache )
     {
-        err = alternative_call(ucode_ops.apply_microcode, microcode_cache,
-                               flags);
+        err = ucode_load(microcode_cache, flags);
         if ( err == -EIO )
         {
             microcode_free_patch(microcode_cache);
@@ -388,7 +393,7 @@ static int primary_thread_work(const struct microcode_patch *patch,
     if ( !wait_for_state(LOADING_ENTER) )
         return -EBUSY;
 
-    ret = alternative_call(ucode_ops.apply_microcode, patch, flags);
+    ret = ucode_load(patch, flags);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -502,7 +507,7 @@ static int control_thread_fn(const struct microcode_patch *patch,
         goto out;
 
     /* Control thread loads ucode first while others are in NMI handler. */
-    ret = alternative_call(ucode_ops.apply_microcode, patch, flags);
+    ret = ucode_load(patch, flags);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -731,7 +736,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
     if ( flags & ~XENPF_UCODE_FORCE )
         return -EINVAL;
 
-    if ( !ucode_ops.apply_microcode )
+    if ( !ucode_ops.load )
         return -EINVAL;
 
     buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
@@ -783,7 +788,7 @@ int microcode_update_one(void)
     if ( ucode_ops.collect_cpu_info )
         alternative_vcall(ucode_ops.collect_cpu_info);
 
-    if ( !ucode_ops.apply_microcode )
+    if ( !ucode_ops.load )
         return -EOPNOTSUPP;
 
     return microcode_update_cpu(NULL, 0);
@@ -821,7 +826,7 @@ int __init microcode_init_cache(struct boot_info *bi)
 {
     int rc = 0;
 
-    if ( !ucode_ops.apply_microcode )
+    if ( !ucode_ops.load )
         return -ENODEV;
 
     if ( ucode_scan )
@@ -907,11 +912,11 @@ int __init early_microcode_init(struct boot_info *bi)
      *
      * Take the hint in either case and ignore the microcode interface.
      */
-    if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
+    if ( !ucode_ops.load || this_cpu(cpu_sig).rev == ~0 )
     {
         printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
-               ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
-        ucode_ops.apply_microcode = NULL;
+               ucode_ops.load ? "rev = ~0" : "HW toggle");
+        ucode_ops.load = NULL;
         return -ENODEV;
     }
 
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 3d3f7e57db80..5e6863fd8c9f 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -287,8 +287,8 @@ static enum microcode_match_result cf_check compare_patch(
     return compare_revisions(old->rev, new->rev);
 }
 
-static int cf_check apply_microcode(const struct microcode_patch *patch,
-                                    unsigned int flags)
+static int cf_check intel_ucode_load(const struct microcode_patch *patch,
+                                     unsigned int flags)
 {
     uint64_t msr_content;
     unsigned int cpu = smp_processor_id();
@@ -408,7 +408,7 @@ static bool __init can_load_microcode(void)
 static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
     .parse                            = intel_ucode_parse,
     .collect_cpu_info                 = collect_cpu_info,
-    .apply_microcode                  = apply_microcode,
+    .load                             = intel_ucode_load,
     .compare_patch                    = compare_patch,
 };
 
@@ -417,5 +417,5 @@ void __init ucode_probe_intel(struct microcode_ops *ops)
     *ops = intel_ucode_ops;
 
     if ( !can_load_microcode() )
-        ops->apply_microcode = NULL;
+        ops->load = NULL;
 }
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index e30acee1536b..9611efaa979c 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -50,8 +50,7 @@ struct microcode_ops {
      * Attempt to load the provided patch into the CPU.  Returns an error if
      * anything didn't go as expected.
      */
-    int (*apply_microcode)(const struct microcode_patch *patch,
-                           unsigned int flags);
+    int (*load)(const struct microcode_patch *patch, unsigned int flags);
 
     /*
      * Given two patches, are they both applicable to the current CPU, and is
@@ -68,8 +67,8 @@ struct microcode_ops {
  *   - Loading available
  *
  * These are encoded by (not) filling in ops->collect_cpu_info (i.e. no
- * support available) and (not) ops->apply_microcode (i.e. read only).
- * Otherwise, all hooks must be filled in.
+ * support available) and (not) ops->load (i.e. read only).  Otherwise, all
+ * hooks must be filled in.
  */
 #ifdef CONFIG_AMD
 void ucode_probe_amd(struct microcode_ops *ops);
-- 
2.39.5

Re: [PATCH 4/5] x86/ucode: Rename the apply_microcode() hook to load()

[Jason Andryuk]

On 2024-10-24 09:22, Andrew Cooper wrote:
> The microcode suffix is redundant, and "microcode loading" is the more common
> term

Missing your S-o-B.

With that:

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 4/5] x86/ucode: Rename the apply_microcode() hook to load()

[Jan Beulich]

On 24.10.2024 15:22, Andrew Cooper wrote:
> The microcode suffix is redundant, and "microcode loading" is the more common
> term

Just to mention it - much like the microcode suffix is redundant, ...

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -214,8 +214,8 @@ static enum microcode_match_result cf_check compare_patch(
>      return compare_header(new, old);
>  }
>  
> -static int cf_check apply_microcode(const struct microcode_patch *patch,
> -                                    unsigned int flags)
> +static int cf_check amd_ucode_load(const struct microcode_patch *patch,
> +                                   unsigned int flags)
>  {

... the ucode infix is as well. Arguably even the amd prefix is redundant
(with the file name). I won't insist on either adjustment, though.

Jan

[PATCH 5/5] x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

[Andrew Cooper]

This was true in the K10 days, but even back then the match registers were
really payload data rather than header data.

But, it's really model specific data, and these days typically part of the
signature, so is random data for all intents and purposes.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

The single difference from this is:

  @@ -207587,7 +207587,7 @@
   ffff82d0402ad261:	4c 89 ce             	mov    %r9,%rsi
   ffff82d0402ad264:	4c 39 c8             	cmp    %r9,%rax
   ffff82d0402ad267:	0f 82 c2 11 f6 ff    	jb     ffff82d04020e42f <amd_ucode_parse.cold+0x55>
  -ffff82d0402ad26d:	41 83 f9 3f          	cmp    $0x3f,%r9d
  +ffff82d0402ad26d:	41 83 f9 1f          	cmp    $0x1f,%r9d
   ffff82d0402ad271:	0f 86 b8 11 f6 ff    	jbe    ffff82d04020e42f <amd_ucode_parse.cold+0x55>
   ffff82d0402ad277:	85 ed                	test   %ebp,%ebp
   ffff82d0402ad279:	75 55                	jne    ffff82d0402ad2d0 <amd_ucode_parse+0x170>

which is "mc->len < sizeof(struct microcode_patch)" expression in
amd_ucode_parse().
---
 xen/arch/x86/cpu/microcode/amd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 1845f51ba330..54acd6928781 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -48,7 +48,6 @@ struct microcode_patch {
     uint8_t  sb_rev_id;
     uint8_t  bios_api_rev;
     uint8_t  reserved1[3];
-    uint32_t match_reg[8];
 };
 
 #define UCODE_MAGIC                0x00414d44
-- 
2.39.5

Re: [PATCH 5/5] x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

[Jason Andryuk]

On 2024-10-24 09:22, Andrew Cooper wrote:
> This was true in the K10 days, but even back then the match registers were
> really payload data rather than header data.
> 
> But, it's really model specific data, and these days typically part of the
> signature, so is random data for all intents and purposes.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 5/5] x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

[Jan Beulich]

On 24.10.2024 15:22, Andrew Cooper wrote:
> This was true in the K10 days, but even back then the match registers were
> really payload data rather than header data.
> 
> But, it's really model specific data, and these days typically part of the
> signature, so is random data for all intents and purposes.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The single difference from this is:
> 
>   @@ -207587,7 +207587,7 @@
>    ffff82d0402ad261:	4c 89 ce             	mov    %r9,%rsi
>    ffff82d0402ad264:	4c 39 c8             	cmp    %r9,%rax
>    ffff82d0402ad267:	0f 82 c2 11 f6 ff    	jb     ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>   -ffff82d0402ad26d:	41 83 f9 3f          	cmp    $0x3f,%r9d
>   +ffff82d0402ad26d:	41 83 f9 1f          	cmp    $0x1f,%r9d
>    ffff82d0402ad271:	0f 86 b8 11 f6 ff    	jbe    ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>    ffff82d0402ad277:	85 ed                	test   %ebp,%ebp
>    ffff82d0402ad279:	75 55                	jne    ffff82d0402ad2d0 <amd_ucode_parse+0x170>
> 
> which is "mc->len < sizeof(struct microcode_patch)" expression in
> amd_ucode_parse().

Yet is it correct to effectively relax that check, i.e. to accept something
we previously would have rejected?

Jan

Re: [PATCH 5/5] x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

[Andrew Cooper]

On 28/10/2024 1:18 pm, Jan Beulich wrote:
> On 24.10.2024 15:22, Andrew Cooper wrote:
>> This was true in the K10 days, but even back then the match registers were
>> really payload data rather than header data.
>>
>> But, it's really model specific data, and these days typically part of the
>> signature, so is random data for all intents and purposes.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> The single difference from this is:
>>
>>   @@ -207587,7 +207587,7 @@
>>    ffff82d0402ad261:	4c 89 ce             	mov    %r9,%rsi
>>    ffff82d0402ad264:	4c 39 c8             	cmp    %r9,%rax
>>    ffff82d0402ad267:	0f 82 c2 11 f6 ff    	jb     ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>>   -ffff82d0402ad26d:	41 83 f9 3f          	cmp    $0x3f,%r9d
>>   +ffff82d0402ad26d:	41 83 f9 1f          	cmp    $0x1f,%r9d
>>    ffff82d0402ad271:	0f 86 b8 11 f6 ff    	jbe    ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>>    ffff82d0402ad277:	85 ed                	test   %ebp,%ebp
>>    ffff82d0402ad279:	75 55                	jne    ffff82d0402ad2d0 <amd_ucode_parse+0x170>
>>
>> which is "mc->len < sizeof(struct microcode_patch)" expression in
>> amd_ucode_parse().
> Yet is it correct to effectively relax that check, i.e. to accept something
> we previously would have rejected?

Yes.  This is the bounds check about whether it's safe to look at fields
in the header.  It's not part of the other validity checks.

~Andrew

Re: [PATCH 5/5] x86/ucode: Drop the match_reg[] field from AMD's microcode_patch

[Jan Beulich]

On 25.02.2025 23:01, Andrew Cooper wrote:
> On 28/10/2024 1:18 pm, Jan Beulich wrote:
>> On 24.10.2024 15:22, Andrew Cooper wrote:
>>> This was true in the K10 days, but even back then the match registers were
>>> really payload data rather than header data.
>>>
>>> But, it's really model specific data, and these days typically part of the
>>> signature, so is random data for all intents and purposes.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> The single difference from this is:
>>>
>>>   @@ -207587,7 +207587,7 @@
>>>    ffff82d0402ad261:	4c 89 ce             	mov    %r9,%rsi
>>>    ffff82d0402ad264:	4c 39 c8             	cmp    %r9,%rax
>>>    ffff82d0402ad267:	0f 82 c2 11 f6 ff    	jb     ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>>>   -ffff82d0402ad26d:	41 83 f9 3f          	cmp    $0x3f,%r9d
>>>   +ffff82d0402ad26d:	41 83 f9 1f          	cmp    $0x1f,%r9d
>>>    ffff82d0402ad271:	0f 86 b8 11 f6 ff    	jbe    ffff82d04020e42f <amd_ucode_parse.cold+0x55>
>>>    ffff82d0402ad277:	85 ed                	test   %ebp,%ebp
>>>    ffff82d0402ad279:	75 55                	jne    ffff82d0402ad2d0 <amd_ucode_parse+0x170>
>>>
>>> which is "mc->len < sizeof(struct microcode_patch)" expression in
>>> amd_ucode_parse().
>> Yet is it correct to effectively relax that check, i.e. to accept something
>> we previously would have rejected?
> 
> Yes.  This is the bounds check about whether it's safe to look at fields
> in the header.  It's not part of the other validity checks.

Hmm, okay:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 0/5] x86/ucode: Minor cleanup

[Alejandro Vallejo]

On Thu Oct 24, 2024 at 2:22 PM BST, Andrew Cooper wrote:
> Misc cleanup without functional change.  Mostly to improve clarity.
>
> Andrew Cooper (5):
>   x86/ucode: Rename hypercall-context functions
>   x86/ucode: Drop the parse_blob() wrapper
>   x86/ucode: Rename the cpu_request_microcode() hook to parse()
>   x86/ucode: Rename the apply_microcode() hook to load()
>   x86/ucode: Drop the match_reg[] field from AMD's microcode_patch
>
>  xen/arch/x86/cpu/microcode/amd.c     | 11 ++---
>  xen/arch/x86/cpu/microcode/core.c    | 72 +++++++++++++++-------------
>  xen/arch/x86/cpu/microcode/intel.c   | 12 ++---
>  xen/arch/x86/cpu/microcode/private.h |  9 ++--
>  xen/arch/x86/include/asm/microcode.h |  4 +-
>  xen/arch/x86/platform_hypercall.c    |  6 +--
>  6 files changed, 59 insertions(+), 55 deletions(-)
>
>
> base-commit: a974725a87a1afc8056b41c56dfe7fe272a7169c

IMO, this change warrants renaming the parent directory to ucode and
microcode.h to ucode.h.

Cheers,
Alejandro