[PATCH v4 0/4] Prevent attempting updates known to fail

[PATCH v4 0/4] Prevent attempting updates known to fail

[Alejandro Vallejo]

v4:
  * The refactor Andrew asked for to avoid a conditional check on
    early_microcode_init(). I'm not convinced it's clearer, but it's not
    much more complicated either, so I don't mind.
  * Removed microcode_ops builders in favour of a separate Intel-specific
    function to check whether the system can load new microcode. This keeps
    the static structs in the right sections.

Under certain conditions a CPU may not be able to perform microcode updates
even if hardware exists to that effect. In particular:

 * If Xen runs under certain hypervisors they won't allow microcode
   updates, and will signal this fact by reporting a microcode revision of
   -1.
 * If the DIS_MCU_LOAD bit is set, which is expected in some baremetal
   clouds where the owner may not trust the tenant, then the CPU is not
   capable of loading new microcode.

This series adds logic so that in both of these cases we don't needlessly
attempt updates that are not going to succeed. Patch summary:

Patch 1 Introduces the logic to print the microcode revision if at all
        possible

Patch 2 Ignores microcode facilities when the current microcode revision is -1

Patch 3 Moves the MSR_ARCH_CAPS read in tsx_init() to early_cpu_init() and
        early_microcode_init()

Patch 4 Adds the logic to detect microcode updates being disabled on Intel.

Alejandro Vallejo (4):
  x86/microcode: Allow reading microcode revision even if it can't be
    updated
  x86/microcode: Ignore microcode loading interface for revision = -1
  x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()
  x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is
    set

 xen/arch/x86/cpu/common.c             |  5 ++++
 xen/arch/x86/cpu/microcode/core.c     | 41 +++++++++++++++++++++++----
 xen/arch/x86/cpu/microcode/intel.c    | 13 +++++++++
 xen/arch/x86/cpu/microcode/private.h  |  7 +++++
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 ++++
 xen/arch/x86/tsx.c                    | 16 +++--------
 7 files changed, 70 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v4 1/4] x86/microcode: Allow reading microcode revision even if it can't be updated

[Alejandro Vallejo]

microcode_update_one() currently assumes all microcode handlers are set or
none are. That won't be the case in a future patch, as apply_microcode()
may not be set while the others are. Hence, this patch allows reading the
microcode revision even if updating it is unavailable.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4:
  * Only remove the assumptions on handler contents from
    microcode_update_one() and leave early_microcode_init() alone (Andrew)
---
 xen/arch/x86/cpu/microcode/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index c3fee62906..bec8b55db2 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -750,11 +750,12 @@ __initcall(microcode_init);
 /* Load a cached update to current cpu */
 int microcode_update_one(void)
 {
+    if ( ucode_ops.collect_cpu_info )
+        alternative_vcall(ucode_ops.collect_cpu_info);
+
     if ( !ucode_ops.apply_microcode )
         return -EOPNOTSUPP;
 
-    alternative_vcall(ucode_ops.collect_cpu_info);
-
     return microcode_update_cpu(NULL);
 }
 
-- 
2.34.1

[PATCH v4 2/4] x86/microcode: Ignore microcode loading interface for revision = -1

[Alejandro Vallejo]

Some hypervisors report ~0 as the microcode revision to mean "don't issue
microcode updates". Ignore the microcode loading interface in that case.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4:
  * Stop piggybacking on the existing early exit
---
 xen/arch/x86/cpu/microcode/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index bec8b55db2..e67d143c97 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -867,10 +867,21 @@ int __init early_microcode_init(unsigned long *module_map,
         return -ENODEV;
     }
 
-    microcode_grab_module(module_map, mbi);
-
     ucode_ops.collect_cpu_info();
 
+    /*
+     * Some hypervisors deliberately report a microcode revision of -1 to
+     * mean that they will not accept microcode updates. We take the hint
+     * and ignore the microcode interface in that case.
+     */
+    if ( this_cpu(cpu_sig).rev == ~0 ) {
+        printk(XENLOG_WARNING "Microcode loading disabled\n");
+        ucode_ops.apply_microcode = NULL;
+        return -ENODEV;
+    }
+
+    microcode_grab_module(module_map, mbi);
+
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
-- 
2.34.1

[PATCH v4 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo]

Move MSR_ARCH_CAPS read code from tsx_init() to early_cpu_init(). Because
microcode updates might make them that MSR to appear/have different values
we also must reload it after a microcode update in early_microcode_init().

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v4:
  * Read MSR_ARCH_CAPS in early_cpu_init(). Otherwise tsx_init() doesn't
    have current values in the case where microcode wasn't updated (Jan)
---
 xen/arch/x86/cpu/common.c         |  5 +++++
 xen/arch/x86/cpu/microcode/core.c | 13 +++++++++++++
 xen/arch/x86/tsx.c                | 16 ++++------------
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace12..2f895e7c7c 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -352,6 +352,11 @@ void __init early_cpu_init(void)
 			    &c->x86_capability[FEATURESET_7c0],
 			    &c->x86_capability[FEATURESET_7d0]);
 
+		if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability))
+			rdmsr(MSR_ARCH_CAPABILITIES,
+			      c->x86_capability[FEATURESET_m10Al],
+			      c->x86_capability[FEATURESET_m10Ah]);
+
 		if (max_subleaf >= 1)
 			cpuid_count(7, 1, &eax, &ebx, &ecx,
 				    &c->x86_capability[FEATURESET_7d1]);
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index e67d143c97..dda6f03f7d 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map,
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
+    /*
+     * MSR_ARCH_CAPS may have appeared after the microcode update.
+     * Reload relevant fields in boot_cpu_data if so because they are
+     * needed in tsx_init().
+     */
+    if ( boot_cpu_data.cpuid_level >= 7 )
+        boot_cpu_data.x86_capability[FEATURESET_7d0]
+            = cpuid_count_edx(7, 0);
+    if ( cpu_has_arch_caps )
+        rdmsr(MSR_ARCH_CAPABILITIES,
+              boot_cpu_data.x86_capability[FEATURESET_m10Al],
+              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
+
     return rc;
 }
diff --git a/xen/arch/x86/tsx.c b/xen/arch/x86/tsx.c
index 80c6f4cedd..50d8059f23 100644
--- a/xen/arch/x86/tsx.c
+++ b/xen/arch/x86/tsx.c
@@ -39,9 +39,10 @@ void tsx_init(void)
     static bool __read_mostly once;
 
     /*
-     * This function is first called between microcode being loaded, and CPUID
-     * being scanned generally.  Read into boot_cpu_data.x86_capability[] for
-     * the cpu_has_* bits we care about using here.
+     * This function is first called between microcode being loaded, and
+     * CPUID being scanned generally. early_cpu_init() has already prepared
+     * the feature bits needed here. And early_microcode_init() has ensured
+     * they are not stale after the microcode update.
      */
     if ( unlikely(!once) )
     {
@@ -49,15 +50,6 @@ void tsx_init(void)
 
         once = true;
 
-        if ( boot_cpu_data.cpuid_level >= 7 )
-            boot_cpu_data.x86_capability[FEATURESET_7d0]
-                = cpuid_count_edx(7, 0);
-
-        if ( cpu_has_arch_caps )
-            rdmsr(MSR_ARCH_CAPABILITIES,
-                  boot_cpu_data.x86_capability[FEATURESET_m10Al],
-                  boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
-
         has_rtm_always_abort = cpu_has_rtm_always_abort;
 
         if ( cpu_has_tsx_ctrl && cpu_has_srbds_ctrl )
-- 
2.34.1

[PATCH v4 4/4] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Alejandro Vallejo]

If IA32_MSR_MCU_CONTROL exists then it's possible a CPU may be unable to
perform microcode updates. This is controlled through the DIS_MCU_LOAD bit
and is intended for baremetal clouds where the owner may not trust the
tenant to choose the microcode version in use. If we notice that bit being
set then simply disable the "apply_microcode" handler so we can't even try
to perform update (as it's known to be silently dropped).

While at it, remove the Intel family check, as microcode loading is
supported on every Intel 64 CPU.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v4:
  * Moved the Intel family-check removal here. Previously integrated in
    other patches
  * Moved DIS_MCU_LOAD logic into a vendor-specific
    intel_can_load_microcode() function.
---
 xen/arch/x86/cpu/microcode/core.c     | 10 +++++++---
 xen/arch/x86/cpu/microcode/intel.c    | 13 +++++++++++++
 xen/arch/x86/cpu/microcode/private.h  |  7 +++++++
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 +++++
 5 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index dda6f03f7d..de0e61d3dd 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -847,17 +847,21 @@ int __init early_microcode_init(unsigned long *module_map,
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     int rc = 0;
+    bool can_load = false;
 
     switch ( c->x86_vendor )
     {
     case X86_VENDOR_AMD:
         if ( c->x86 >= 0x10 )
+        {
             ucode_ops = amd_ucode_ops;
+            can_load = true;
+        }
         break;
 
     case X86_VENDOR_INTEL:
-        if ( c->x86 >= 6 )
-            ucode_ops = intel_ucode_ops;
+        ucode_ops = intel_ucode_ops;
+        can_load = intel_can_load_microcode();
         break;
     }
 
@@ -874,7 +878,7 @@ int __init early_microcode_init(unsigned long *module_map,
      * mean that they will not accept microcode updates. We take the hint
      * and ignore the microcode interface in that case.
      */
-    if ( this_cpu(cpu_sig).rev == ~0 ) {
+    if ( this_cpu(cpu_sig).rev == ~0 || !can_load ) {
         printk(XENLOG_WARNING "Microcode loading disabled\n");
         ucode_ops.apply_microcode = NULL;
         return -ENODEV;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 8d4d6574aa..c51757da90 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -385,6 +385,19 @@ static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
+bool __init intel_can_load_microcode(void)
+{
+    uint64_t mcu_ctrl;
+
+    if ( !cpu_has_mcu_ctrl )
+        return true;
+
+    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
+
+    /* If DIS_MCU_LOAD is set applying microcode updates won't work */
+    return  !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
+}
+
 const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 626aeb4d08..87fcf84373 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -60,6 +60,13 @@ struct microcode_ops {
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
+/**
+ * Checks whether we can perform microcode updates on this Intel system
+ *
+ * @return True iff the microcode update facilities are enabled
+ */
+bool __init intel_can_load_microcode(void);
+
 extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops;
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index e2cb8f3cc7..608bc4dce0 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -192,6 +192,7 @@ static inline bool boot_cpu_has(unsigned int feat)
 #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO)
 #define cpu_has_tsx_ctrl        boot_cpu_has(X86_FEATURE_TSX_CTRL)
 #define cpu_has_taa_no          boot_cpu_has(X86_FEATURE_TAA_NO)
+#define cpu_has_mcu_ctrl        boot_cpu_has(X86_FEATURE_MCU_CTRL)
 #define cpu_has_fb_clear        boot_cpu_has(X86_FEATURE_FB_CLEAR)
 #define cpu_has_rrsba           boot_cpu_has(X86_FEATURE_RRSBA)
 
diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
index 2749e433d2..5c1350b5f9 100644
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -165,6 +165,11 @@
 #define  PASID_PASID_MASK                   0x000fffff
 #define  PASID_VALID                        (_AC(1, ULL) << 31)
 
+#define MSR_MCU_CONTROL                     0x00001406
+#define  MCU_CONTROL_LOCK                   (_AC(1, ULL) <<  0)
+#define  MCU_CONTROL_DIS_MCU_LOAD           (_AC(1, ULL) <<  1)
+#define  MCU_CONTROL_EN_SMM_BYPASS          (_AC(1, ULL) <<  2)
+
 #define MSR_UARCH_MISC_CTRL                 0x00001b01
 #define  UARCH_CTRL_DOITM                   (_AC(1, ULL) <<  0)
 
-- 
2.34.1

Re: [PATCH v4 2/4] x86/microcode: Ignore microcode loading interface for revision = -1

[Jan Beulich]

On 22.06.2023 19:42, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -867,10 +867,21 @@ int __init early_microcode_init(unsigned long *module_map,
>          return -ENODEV;
>      }
>  
> -    microcode_grab_module(module_map, mbi);
> -
>      ucode_ops.collect_cpu_info();
>  
> +    /*
> +     * Some hypervisors deliberately report a microcode revision of -1 to
> +     * mean that they will not accept microcode updates. We take the hint
> +     * and ignore the microcode interface in that case.
> +     */
> +    if ( this_cpu(cpu_sig).rev == ~0 ) {

Nit: Style (brace placement). Can probably be fixed up while
committing, if no other issues arise.

Jan

Re: [PATCH v4 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Jan Beulich]

On 22.06.2023 19:42, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map,
>      if ( ucode_mod.mod_end || ucode_blob.size )
>          rc = early_microcode_update_cpu();
>  
> +    /*
> +     * MSR_ARCH_CAPS may have appeared after the microcode update.
> +     * Reload relevant fields in boot_cpu_data if so because they are
> +     * needed in tsx_init().
> +     */
> +    if ( boot_cpu_data.cpuid_level >= 7 )
> +        boot_cpu_data.x86_capability[FEATURESET_7d0]
> +            = cpuid_count_edx(7, 0);
> +    if ( cpu_has_arch_caps )
> +        rdmsr(MSR_ARCH_CAPABILITIES,
> +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
> +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
> +
>      return rc;
>  }

Did you consider simply calling early_cpu_init() a 2nd time, and then
perhaps from setup.c and only if ucode load didn't report an error?
There's a printk() in there which will want avoiding on the 2nd pass,
but otherwise this would look more future-proof to me.

Jan

Re: [PATCH v4 4/4] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Jan Beulich]

On 22.06.2023 19:42, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -385,6 +385,19 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>      return patch;
>  }
>  
> +bool __init intel_can_load_microcode(void)
> +{
> +    uint64_t mcu_ctrl;
> +
> +    if ( !cpu_has_mcu_ctrl )
> +        return true;
> +
> +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> +
> +    /* If DIS_MCU_LOAD is set applying microcode updates won't work */
> +    return  !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);

And we have to honor this, i.e. we shouldn't make an attempt
at clearing the bit?

Also nit: One too many blank after "return".

> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -60,6 +60,13 @@ struct microcode_ops {
>          const struct microcode_patch *new, const struct microcode_patch *old);
>  };
>  
> +/**
> + * Checks whether we can perform microcode updates on this Intel system
> + *
> + * @return True iff the microcode update facilities are enabled
> + */
> +bool __init intel_can_load_microcode(void);

No __init please on declarations; they only matter on definitions.

Jan

Re: [PATCH v4 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo]

On Fri, Jun 23, 2023 at 09:33:56AM +0200, Jan Beulich wrote:
> On 22.06.2023 19:42, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -885,5 +885,18 @@ int __init early_microcode_init(unsigned long *module_map,
> >      if ( ucode_mod.mod_end || ucode_blob.size )
> >          rc = early_microcode_update_cpu();
> >  
> > +    /*
> > +     * MSR_ARCH_CAPS may have appeared after the microcode update.
> > +     * Reload relevant fields in boot_cpu_data if so because they are
> > +     * needed in tsx_init().
> > +     */
> > +    if ( boot_cpu_data.cpuid_level >= 7 )
> > +        boot_cpu_data.x86_capability[FEATURESET_7d0]
> > +            = cpuid_count_edx(7, 0);
> > +    if ( cpu_has_arch_caps )
> > +        rdmsr(MSR_ARCH_CAPABILITIES,
> > +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
> > +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
> > +
> >      return rc;
> >  }
> 
> Did you consider simply calling early_cpu_init() a 2nd time, and then
> perhaps from setup.c and only if ucode load didn't report an error?
> There's a printk() in there which will want avoiding on the 2nd pass,
> but otherwise this would look more future-proof to me.
> 
> Jan
I had, but avoiding the internal printk() was annoying. I've simply created
a boolean "verbosity" flag on the new version for early_cpu_init() and
called it at the end of early_microcode_init().

Alejandro