diff mbox series

[v3,2/5] x86/microcode: Create per-vendor microcode_ops builders

Message ID 20230615154834.959-3-alejandro.vallejo@cloud.com (mailing list archive)
State Superseded
Headers show
Series Prevent attempting updates known to fail | expand

Commit Message

Alejandro Vallejo June 15, 2023, 3:48 p.m. UTC 
Replace the ucode_ops assignments in core.c for per-vendor calls.
This is in preparation for another patch that adds Intel-specific
conditions.

While moving the code around, also remove the family check on Intel, as
microcode loading is present on every Intel 64 machine.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v3:
  * Subsumes v2/patch1
  * Removes previous long comment on rationale for skipping family checks (Jan/Andrew)
  * Isolates vendor-specific code in ${VENDOR}.c (Jan, from v2/patch4)
---
 xen/arch/x86/cpu/microcode/amd.c     | 16 ++++++++++------
 xen/arch/x86/cpu/microcode/core.c    | 10 +++-------
 xen/arch/x86/cpu/microcode/intel.c   | 13 +++++++------
 xen/arch/x86/cpu/microcode/private.h | 19 ++++++++++++++++++-
 4 files changed, 38 insertions(+), 20 deletions(-)

Comments

Jan Beulich June 19, 2023, 3:45 p.m. UTC | #1 
On 15.06.2023 17:48, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -432,9 +432,13 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>      return patch;
>  }
>  
> -const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {

Something will want to be done to retain the cf_clobber aspect,
i.e. to be able to get rid of no longer necessary ENDBR64 after
alternatives patching is finished. I guess I first need to see
what further patches do in order to maybe come up with a
suggestion.

Jan
Alejandro Vallejo June 22, 2023, 2:34 p.m. UTC | #2 
On Mon, Jun 19, 2023 at 05:45:14PM +0200, Jan Beulich wrote:
> On 15.06.2023 17:48, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -432,9 +432,13 @@ static struct microcode_patch *cf_check cpu_request_microcode(
> >      return patch;
> >  }
> >  
> > -const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
> 
> Something will want to be done to retain the cf_clobber aspect,
> i.e. to be able to get rid of no longer necessary ENDBR64 after
> alternatives patching is finished. I guess I first need to see
> what further patches do in order to maybe come up with a
> suggestion.
> 
> Jan
I (mistakenly) thought the clobber tag was simply making sure the static
contents were themselves clobbered. It seems like it's actually aiding the
alternatives machinery with function pointer cleanup, so this was blatantly
wrong on my part. Sigh...

Either way, I'm bringing the static structs back and dealing with this
in another way. Cheers for the pointer.

Alejandro
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index a9a5557835..7c9f311454 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -432,9 +432,13 @@  static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
-    .cpu_request_microcode            = cpu_request_microcode,
-    .collect_cpu_info                 = collect_cpu_info,
-    .apply_microcode                  = apply_microcode,
-    .compare_patch                    = compare_patch,
-};
+void __init amd_get_ucode_ops(struct microcode_ops *ops)
+{
+    if ( boot_cpu_data.x86 < 0x10 )
+        return;
+
+    ops->cpu_request_microcode = cpu_request_microcode;
+    ops->collect_cpu_info      = collect_cpu_info;
+    ops->apply_microcode       = apply_microcode;
+    ops->compare_patch         = compare_patch;
+}
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index df7e1df870..530e3e8267 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -845,19 +845,15 @@  static int __init early_microcode_update_cpu(void)
 int __init early_microcode_init(unsigned long *module_map,
                                 const struct multiboot_info *mbi)
 {
-    const struct cpuinfo_x86 *c = &boot_cpu_data;
     int rc = 0;
 
-    switch ( c->x86_vendor )
+    switch ( boot_cpu_data.x86_vendor )
     {
     case X86_VENDOR_AMD:
-        if ( c->x86 >= 0x10 )
-            ucode_ops = amd_ucode_ops;
+        amd_get_ucode_ops(&ucode_ops);
         break;
-
     case X86_VENDOR_INTEL:
-        if ( c->x86 >= 6 )
-            ucode_ops = intel_ucode_ops;
+        intel_get_ucode_ops(&ucode_ops);
         break;
     }
 
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 8d4d6574aa..a99e402b98 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -385,9 +385,10 @@  static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
-    .cpu_request_microcode            = cpu_request_microcode,
-    .collect_cpu_info                 = collect_cpu_info,
-    .apply_microcode                  = apply_microcode,
-    .compare_patch                    = compare_patch,
-};
+void __init intel_get_ucode_ops(struct microcode_ops *ops)
+{
+    ops->cpu_request_microcode = cpu_request_microcode;
+    ops->collect_cpu_info      = collect_cpu_info;
+    ops->apply_microcode       = apply_microcode;
+    ops->compare_patch         = compare_patch;
+}
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 626aeb4d08..13f0c7fb8e 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -60,6 +60,23 @@  struct microcode_ops {
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
-extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops;
+/**
+ * Retrieve the vendor-specific microcode management handlers
+ *
+ * Note that this is not an static set of handlers and may change from
+ * system to system depending on the presence of certain runtime features.
+ * even for the same 
+ *
+ *   - If the system has no microcode facilities, no handler is set.
+ *   - If the system has unrestricted microcode facilities, all handlers
+ *     are set
+ *   - If the system has microcode facilities, but they can't be used to
+ *     update the revision, then all handlers except for apply_microcode()
+ *     are set
+ *
+ * @param[out] ops Set of vendor-specific microcode handlers to overwrite
+ */
+void intel_get_ucode_ops(struct microcode_ops *ops);
+void amd_get_ucode_ops(struct microcode_ops *ops);
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */