[2/2] x86/ucode: load microcode earlier on boot CPU

Commit Message

Sergey Dyasli Dec. 8, 2022, 1:26 p.m. UTC

Call early_microcode_init() straight after multiboot modules become
accessible. Modify it to load the ucode directly from the blob bypassing
populating microcode_cache because xmalloc is still not available at
that point during Xen boot.

Introduce early_microcode_init_cache() for populating microcode_cache.
It needs to find the new virtual address of the ucode blob because it
changes during boot, e.g. from 0x00000000010802fc to 0xffff83204dac52fc.

Signed-off-by: Sergey Dyasli <[e-mail]>
---
 xen/arch/x86/cpu/microcode/core.c    | 61 ++++++++++++++++++++++++----
 xen/arch/x86/include/asm/microcode.h |  6 ++-
 xen/arch/x86/include/asm/setup.h     |  3 --
 xen/arch/x86/setup.c                 | 10 +++--
 4 files changed, 64 insertions(+), 16 deletions(-)

Comments

Jan Beulich Dec. 12, 2022, 4:29 p.m. UTC | #1

On 08.12.2022 14:26, Sergey Dyasli wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -198,7 +198,7 @@ void __init microcode_scan_module(
>          bootstrap_map(NULL);
>      }
>  }
> -void __init microcode_grab_module(
> +static void __init microcode_grab_module(

May I ask that you take the opportunity and add the missing blank line
between the two functions?

> @@ -733,9 +733,35 @@ int microcode_update_one(void)
>  }
>  
>  /* BSP calls this function to parse ucode blob and then apply an update. */
> -static int __init early_microcode_update_cpu(void)

I think the comment wants to stay with its function. In fact I think you
simply want to move down ...

> +static int __init early_microcode_update_cache(const void *data, size_t len)
>  {

... this function, which strictly is a helper of early_microcode_init_cache()
(and, being a static helper, could imo do with having a shorter name).

> @@ -754,7 +780,9 @@ static int __init early_microcode_update_cpu(void)
>      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, false);

I realize early_microcode_init() also uses alternative_vcall(), but I
doubt that of the two altcalls here are useful to have - they merely
bloat the binary afaict. Looking at Andrew's 8f473f92e531 ("x86/ucode:
Use altcall, and __initconst_cf_clobber") I also don't see any clear
justification - the __initconst_cf_clobber alone is sufficient for the
stated purpose, I think (as far as __init functions are concerned, of
course).

> @@ -765,15 +793,28 @@ static int __init early_microcode_update_cpu(void)
>      if ( !patch )
>          return -ENOENT;
>  
> -    spin_lock(&microcode_mutex);
> -    rc = microcode_update_cache(patch);
> -    spin_unlock(&microcode_mutex);
> -    ASSERT(rc);
> +    return microcode_update_cpu(patch);
> +}
> +
> +int __init early_microcode_init_cache(unsigned long *module_map,
> +                                      const multiboot_info_t *mbi)
> +{
> +    int rc = 0;
> +
> +    /* Need to rescan the modules because they might have been relocated */
> +    microcode_grab_module(module_map, mbi);

I'm afraid the function isn't safe to call twice; the only safe case looks
to be when "ucode=scan" is in use.

> --- a/xen/arch/x86/include/asm/microcode.h
> +++ b/xen/arch/x86/include/asm/microcode.h
> @@ -3,6 +3,7 @@
>  
>  #include <xen/types.h>
>  #include <xen/percpu.h>
> +#include <xen/multiboot.h>

I think dependencies like this are moving us in the wrong direction, wrt
our header dependencies nightmare. Could I talk you into adding a prereq
patch giving a proper tag to the struct which is typedef-ed to
multiboot_info_t, such that a forward declaration of that struct would
suffice ...

> @@ -21,7 +22,10 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
>  
>  void microcode_set_module(unsigned int idx);
>  int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len);
> -int early_microcode_init(void);
> +int early_microcode_init(unsigned long *module_map,
> +                         const multiboot_info_t *mbi);
> +int early_microcode_init_cache(unsigned long *module_map,
> +                               const multiboot_info_t *mbi);

... ahead of the two uses here?

Jan

Patch

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 924a2bd7b5..b04b30ce5e 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -198,7 +198,7 @@  void __init microcode_scan_module(
         bootstrap_map(NULL);
     }
 }
-void __init microcode_grab_module(
+static void __init microcode_grab_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
 {
@@ -733,9 +733,35 @@  int microcode_update_one(void)
 }
 
 /* BSP calls this function to parse ucode blob and then apply an update. */
-static int __init early_microcode_update_cpu(void)
+static int __init early_microcode_update_cache(const void *data, size_t len)
 {
     int rc = 0;
+    const struct microcode_patch *patch;
+
+    if ( !data )
+        return -ENOMEM;
+
+    patch = parse_blob(data, len);
+    if ( IS_ERR(patch) )
+    {
+        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
+               PTR_ERR(patch));
+        return PTR_ERR(patch);
+    }
+
+    if ( !patch )
+        return -ENOENT;
+
+    spin_lock(&microcode_mutex);
+    rc = microcode_update_cache(patch);
+    spin_unlock(&microcode_mutex);
+    ASSERT(rc);
+
+    return rc;
+}
+
+static int __init early_microcode_update_cpu(void)
+{
     const void *data = NULL;
     size_t len;
     const struct microcode_patch *patch;
@@ -754,7 +780,9 @@  static int __init early_microcode_update_cpu(void)
     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, false);
     if ( IS_ERR(patch) )
     {
         printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
@@ -765,15 +793,28 @@  static int __init early_microcode_update_cpu(void)
     if ( !patch )
         return -ENOENT;
 
-    spin_lock(&microcode_mutex);
-    rc = microcode_update_cache(patch);
-    spin_unlock(&microcode_mutex);
-    ASSERT(rc);
+    return microcode_update_cpu(patch);
+}
+
+int __init early_microcode_init_cache(unsigned long *module_map,
+                                      const multiboot_info_t *mbi)
+{
+    int rc = 0;
+
+    /* Need to rescan the modules because they might have been relocated */
+    microcode_grab_module(module_map, mbi);
+
+    if ( ucode_mod.mod_end )
+        rc = early_microcode_update_cache(bootstrap_map(&ucode_mod),
+                                          ucode_mod.mod_end);
+    else if ( ucode_blob.size )
+        rc = early_microcode_update_cache(ucode_blob.data, ucode_blob.size);
 
-    return microcode_update_one();
+    return rc;
 }
 
-int __init early_microcode_init(void)
+int __init early_microcode_init(unsigned long *module_map,
+                                const multiboot_info_t *mbi)
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     int rc = 0;
@@ -797,6 +838,8 @@  int __init early_microcode_init(void)
         return -ENODEV;
     }
 
+    microcode_grab_module(module_map, mbi);
+
     alternative_vcall(ucode_ops.collect_cpu_info);
 
     if ( ucode_mod.mod_end || ucode_blob.size )
diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h
index 3b0234e9fa..c5f9897535 100644
--- a/xen/arch/x86/include/asm/microcode.h
+++ b/xen/arch/x86/include/asm/microcode.h
@@ -3,6 +3,7 @@ 
 
 #include <xen/types.h>
 #include <xen/percpu.h>
+#include <xen/multiboot.h>
 
 #include <public/xen.h>
 
@@ -21,7 +22,10 @@  DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 
 void microcode_set_module(unsigned int idx);
 int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len);
-int early_microcode_init(void);
+int early_microcode_init(unsigned long *module_map,
+                         const multiboot_info_t *mbi);
+int early_microcode_init_cache(unsigned long *module_map,
+                               const multiboot_info_t *mbi);
 int microcode_update_one(void);
 
 #endif /* ASM_X86__MICROCODE_H */
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 21037b7f31..82ee51c2dc 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -45,9 +45,6 @@  void *bootstrap_map(const module_t *mod);
 
 int xen_in_range(unsigned long mfn);
 
-void microcode_grab_module(
-    unsigned long *, const multiboot_info_t *);
-
 extern uint8_t kbd_shift_flags;
 
 #ifdef NDEBUG
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index e05189f649..9955e1e6fa 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1178,6 +1178,12 @@  void __init noreturn __start_xen(unsigned long mbi_p)
         mod[i].reserved = 0;
     }
 
+    /*
+     * TODO: load ucode earlier once multiboot modules become accessible
+     * at an earlier stage.
+     */
+    early_microcode_init(module_map, mbi);
+
     if ( xen_phys_start )
     {
         relocated = true;
@@ -1774,11 +1780,9 @@  void __init noreturn __start_xen(unsigned long mbi_p)
 
     init_IRQ();
 
-    microcode_grab_module(module_map, mbi);
-
     timer_init();
 
-    early_microcode_init();
+    early_microcode_init_cache(module_map, mbi);
 
     tsx_init(); /* Needs microcode.  May change HLE/RTM feature bits. */