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

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

[Alejandro Vallejo]

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             | 23 ++++++++++++------
 xen/arch/x86/cpu/microcode/core.c     | 35 ++++++++++++++++++++++-----
 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/setup.c                  |  2 +-
 xen/arch/x86/tsx.c                    | 16 +++---------
 8 files changed, 75 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH v5 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>
---
 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 v5 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>
---
v5:
  * Style fix. Brace position.
---
 xen/arch/x86/cpu/microcode/core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index bec8b55db2..b620e3bfa6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -867,10 +867,22 @@ 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 v5 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>
---
v5:
 * Re-run early_cpu_init() after early_microcode_init() rather than
   reloading specific fields
 * Amended early_cpu_init() so it takes a `verbose` argument in order
   to skip printing the same information before and after early microcode
   updates
---
 xen/arch/x86/cpu/common.c         | 23 +++++++++++++++--------
 xen/arch/x86/cpu/microcode/core.c |  6 ++++++
 xen/arch/x86/setup.c              |  2 +-
 xen/arch/x86/tsx.c                | 16 ++++------------
 4 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace12..a1be0aa4bd 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -303,7 +303,7 @@ static inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
 
    WARNING: this function is only called on the BP.  Don't add code here
    that is supposed to run on all CPUs. */
-void __init early_cpu_init(void)
+void __init early_cpu_init(bool verbose)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 	u32 eax, ebx, ecx, edx;
@@ -324,9 +324,10 @@ void __init early_cpu_init(void)
 	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
 	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
 	default:
-		printk(XENLOG_ERR
-		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
-		       c->x86_vendor_id);
+		if (verbose)
+			printk(XENLOG_ERR
+			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
+			       c->x86_vendor_id);
 	}
 
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
@@ -340,10 +341,11 @@ void __init early_cpu_init(void)
 	c->x86_capability[FEATURESET_1d] = edx;
 	c->x86_capability[FEATURESET_1c] = ecx;
 
-	printk(XENLOG_INFO
-	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
-	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
-	       c->x86_model, c->x86_model, c->x86_mask, eax);
+	if (verbose)
+		printk(XENLOG_INFO
+		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
+		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
+		       c->x86_model, c->x86_model, c->x86_mask, eax);
 
 	if (c->cpuid_level >= 7) {
 		uint32_t max_subleaf;
@@ -352,6 +354,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 b620e3bfa6..98a5aebfe3 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -886,5 +886,11 @@ 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
+     * boot_cpu_data if so because they are needed in tsx_init().
+     */
+    early_cpu_init(false);
+
     return rc;
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 74e3915a4d..bdf66e80ac 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1211,7 +1211,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         panic("Bootloader provided no memory information\n");
 
     /* This must come before e820 code because it sets paddr_bits. */
-    early_cpu_init();
+    early_cpu_init(true);
 
     /* Choose shadow stack early, to set infrastructure up appropriately. */
     if ( !boot_cpu_has(X86_FEATURE_CET_SS) )
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 v5 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>
---
v5:
  * Removed __init on declaration
  * Minor style fix (2 spaces rather than 1 after "return")
---
 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 98a5aebfe3..982b278c9e 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;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 8d4d6574aa..060c529a6e 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..d80787205a 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 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 v5 3/4] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Jan Beulich]

On 29.06.2023 17:26, Alejandro Vallejo wrote:
> @@ -324,9 +324,10 @@ void __init early_cpu_init(void)
>  	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
>  	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
>  	default:
> -		printk(XENLOG_ERR
> -		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> -		       c->x86_vendor_id);
> +		if (verbose)
> +			printk(XENLOG_ERR
> +			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> +			       c->x86_vendor_id);

Just as a remark:

	if (!verbose)
		break;

would have been less of a delta and keeping all lines within the 80
chars limit.

> @@ -340,10 +341,11 @@ void __init early_cpu_init(void)
>  	c->x86_capability[FEATURESET_1d] = edx;
>  	c->x86_capability[FEATURESET_1c] = ecx;
>  
> -	printk(XENLOG_INFO
> -	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> -	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
> -	       c->x86_model, c->x86_model, c->x86_mask, eax);
> +	if (verbose)
> +		printk(XENLOG_INFO
> +		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> +		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
> +		       c->x86_model, c->x86_model, c->x86_mask, eax);

Since rearrangement to limit line length isn't really possible here,
the last two lines need re-flowing to stay within limits.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -886,5 +886,11 @@ 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
> +     * boot_cpu_data if so because they are needed in tsx_init().
> +     */
> +    early_cpu_init(false);

I think the comment would better talk of ARCH_CAPS as an example of what
may newly appear with a ucode update.

With at least the middle item taken care of (which I'd be happy to
do while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Jan Beulich]

On 29.06.2023 17:26, Alejandro Vallejo wrote:
> --- 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 )

While not too bad, the addition brings code and comment slightly out
of sync.

> --- 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);

While one would hope that feature bit and MSR access working come in
matched pairs, I still wonder whether - just to be on the safe side -
the caller wouldn't better avoid calling here when rev == ~0 (and
hence we won't try to load ucode anyway). I would envision can_load's
initializer to become this_cpu(cpu_sig).rev != ~0, with other logic
adjusted as necessary in early_microcode_init().

Jan

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

[Alejandro Vallejo]

On Wed, Jul 05, 2023 at 12:43:27PM +0200, Jan Beulich wrote:
> On 29.06.2023 17:26, Alejandro Vallejo wrote:
> > @@ -324,9 +324,10 @@ void __init early_cpu_init(void)
> >  	case X86_VENDOR_SHANGHAI: this_cpu = &shanghai_cpu_dev; break;
> >  	case X86_VENDOR_HYGON:    this_cpu = &hygon_cpu_dev;    break;
> >  	default:
> > -		printk(XENLOG_ERR
> > -		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> > -		       c->x86_vendor_id);
> > +		if (verbose)
> > +			printk(XENLOG_ERR
> > +			       "Unrecognised or unsupported CPU vendor '%.12s'\n",
> > +			       c->x86_vendor_id);
> 
> Just as a remark:
> 
> 	if (!verbose)
> 		break;
> 
> would have been less of a delta and keeping all lines within the 80
> chars limit.
Very true, that looks nicer.

> > @@ -340,10 +341,11 @@ void __init early_cpu_init(void)
> >  	c->x86_capability[FEATURESET_1d] = edx;
> >  	c->x86_capability[FEATURESET_1c] = ecx;
> >  
> > -	printk(XENLOG_INFO
> > -	       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> > -	       x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
> > -	       c->x86_model, c->x86_model, c->x86_mask, eax);
> > +	if (verbose)
> > +		printk(XENLOG_INFO
> > +		       "CPU Vendor: %s, Family %u (%#x), Model %u (%#x), Stepping %u (raw %08x)\n",
> > +		       x86_cpuid_vendor_to_str(boot_cpu_data->x86_vendor), c->x86, c->x86,
> > +		       c->x86_model, c->x86_model, c->x86_mask, eax);
> 
> Since rearrangement to limit line length isn't really possible here,
> the last two lines need re-flowing to stay within limits.
I assumed they could could share the length of the printk string. I don't
mind either way.

> 
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -886,5 +886,11 @@ 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
> > +     * boot_cpu_data if so because they are needed in tsx_init().
> > +     */
> > +    early_cpu_init(false);
> 
> I think the comment would better talk of ARCH_CAPS as an example of what
> may newly appear with a ucode update.
I just started writing a paragraph stating that it's unlikely anything else
will just appear, but thinking it through you're definitely right. A new
MSR_NEW_SPEC_MITIGATIONS might very well appear.

Something along this lines would be better?
```
          * Microcode updates may change CPUID or MSRs. We need to reload
          * the early subset boot_cpu_data before continuing. Notably tsx_init()
          * needs an up to date MSR_ARCH_CAPS.
```

> 
> With at least the middle item taken care of (which I'd be happy to
> do while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan
Thanks. I'm happy with all 3 changes being done on commit.

Alejandro

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

[Alejandro Vallejo]

On Wed, Jul 05, 2023 at 02:12:58PM +0100, Alejandro Vallejo wrote:
> Something along this lines would be better?
> ```
>           * Microcode updates may change CPUID or MSRs. We need to reload
>           * the early subset boot_cpu_data before continuing. Notably tsx_init()
>           * needs an up to date MSR_ARCH_CAPS.
> ```
That makes no sense. I sent the email before completing the sentence. I
meant:

```
          * Microcode updates may change CPUID or MSRs. We need to reload
          * the subset of boot_cpu_data we got on early_cpu_init() before
          * continuing. Notably tsx_init() needs an up to date MSR_ARCH_CAPS.
```

Alejandro

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

[Andrew Cooper]

On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
> 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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Alejandro Vallejo]

On Wed, Jul 05, 2023 at 12:51:47PM +0200, Jan Beulich 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);
> 
> While one would hope that feature bit and MSR access working come in
> matched pairs, I still wonder whether - just to be on the safe side -
> the caller wouldn't better avoid calling here when rev == ~0 (and
> hence we won't try to load ucode anyway). I would envision can_load's
> initializer to become this_cpu(cpu_sig).rev != ~0, with other logic
> adjusted as necessary in early_microcode_init().
> 
> Jan
We only know about the ucode revision after the collect_cpu_info() call,
and we can only make that call after the vendor-specific section that sets
the function pointers up (and calls intel_can_load_microcode()).

One could imagine turning can_load into a function pointer so that its
execution is deferred until after the revision check (and skipped
altogether if `rev==~0`).

Alejandro

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

[Andrew Cooper]

On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index bec8b55db2..b620e3bfa6 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -867,10 +867,22 @@ 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");

XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
virt\n"

It's normal (and not a warning) when running under other hypervisors,
and just "loading disabled" is too little information.

Happy to fix on commit.  Everything else looks ok.

~Andrew

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

[Jan Beulich]

On 05.07.2023 16:13, Andrew Cooper wrote:
> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>> index bec8b55db2..b620e3bfa6 100644
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -867,10 +867,22 @@ 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");
> 
> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
> virt\n"
> 
> It's normal (and not a warning) when running under other hypervisors,

Except that INFO won't be visible by default in release configurations.

Jan

> and just "loading disabled" is too little information.
> 
> Happy to fix on commit.  Everything else looks ok.
> 
> ~Andrew

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

[Andrew Cooper]

On 05/07/2023 3:24 pm, Jan Beulich wrote:
> On 05.07.2023 16:13, Andrew Cooper wrote:
>> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>> index bec8b55db2..b620e3bfa6 100644
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -867,10 +867,22 @@ 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");
>> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
>> virt\n"
>>
>> It's normal (and not a warning) when running under other hypervisors,
> Except that INFO won't be visible by default in release configurations.

Well that's not a bug with microcode then, is it...

I can't believe I'm having to say no to emitting messages at the wrong
log level to work around a bug with selecting the default log level in
the first place.

~Andrew

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

[Jan Beulich]

On 05.07.2023 16:03, Alejandro Vallejo wrote:
> On Wed, Jul 05, 2023 at 12:51:47PM +0200, Jan Beulich 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);
>>
>> While one would hope that feature bit and MSR access working come in
>> matched pairs, I still wonder whether - just to be on the safe side -
>> the caller wouldn't better avoid calling here when rev == ~0 (and
>> hence we won't try to load ucode anyway). I would envision can_load's
>> initializer to become this_cpu(cpu_sig).rev != ~0, with other logic
>> adjusted as necessary in early_microcode_init().
>>
> We only know about the ucode revision after the collect_cpu_info() call,
> and we can only make that call after the vendor-specific section that sets
> the function pointers up (and calls intel_can_load_microcode()).

Hmm, right, that wasn't quite visible from looking at patch and
current tree, because of what earlier patches in the series do.

> One could imagine turning can_load into a function pointer so that its
> execution is deferred until after the revision check (and skipped
> altogether if `rev==~0`).

Perhaps not worth going this far, and instead stay with what you
have until we know (if ever) that further tweaking is necessary.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
(maybe with an adjustment to the comment, as mentioned in the
earlier reply)

Jan

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

[Jan Beulich]

On 05.07.2023 16:28, Andrew Cooper wrote:
> On 05/07/2023 3:24 pm, Jan Beulich wrote:
>> On 05.07.2023 16:13, Andrew Cooper wrote:
>>> On 29/06/2023 4:26 pm, Alejandro Vallejo wrote:
>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>>> index bec8b55db2..b620e3bfa6 100644
>>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>>> @@ -867,10 +867,22 @@ 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");
>>> XENLOG_INFO "Found microcode revision ~0;  Disabling loading because of
>>> virt\n"
>>>
>>> It's normal (and not a warning) when running under other hypervisors,
>> Except that INFO won't be visible by default in release configurations.
> 
> Well that's not a bug with microcode then, is it...
> 
> I can't believe I'm having to say no to emitting messages at the wrong
> log level to work around a bug with selecting the default log level in
> the first place.

Hmm, I think not emitting info level messages is quite right. If I'm
not mistaken we emit various others at warning level to "account" for
this (you might instead call it "to work around this").

Furthermore, as to your suggestions here: What would you expect patch
4 to do? Emit yet another message, instead of having both conditions
fold to just one of them?

Jan

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

[Alejandro Vallejo]

On Wed, Jul 05, 2023 at 04:30:02PM +0200, Jan Beulich wrote:
> (maybe with an adjustment to the comment, as mentioned in the
> earlier reply)
> 
> Jan
Yes, that sounds good to me. Thanks.

Alejandro