[PATCH v2 0/5] Prevent attempting updates known to fail

[PATCH v2 0/5] Prevent attempting updates known to fail

[Alejandro Vallejo]

v3:
  * Lots of hunks moved around. Individually mentioned in each patch
  * Removed a redundant check
  * Ignore microcode interface if the revision is -1
  * Perform the DIS_MCU_LOAD checks during init rather than apply time


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 Does the refactors to allow collecting cpu info on systems with
        microcode updates disabled

Patch 2 Isolates early_microcode_init() per-vendor logic in per-vendor
        functions

Patch 3 Recognizes microcode revision of -1 as a hint meaning "don't use the
        microcode interface".

Patch 4 Moves the MSR_ARCH_CAPS read from tsx_init() to
        early_microcode_init()

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

Alejandro Vallejo (5):
  x86/microcode: Allow reading microcode revision even if it can't be
    updated
  x86/microcode: Create per-vendor microcode_ops builders
  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/amd.c      | 16 +++++++----
 xen/arch/x86/cpu/microcode/core.c     | 41 ++++++++++++++++++++-------
 xen/arch/x86/cpu/microcode/intel.c    | 27 ++++++++++++++----
 xen/arch/x86/cpu/microcode/private.h  | 19 ++++++++++++-
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 ++++
 xen/arch/x86/tsx.c                    | 15 ++--------
 8 files changed, 93 insertions(+), 36 deletions(-)

-- 
2.34.1

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

[Alejandro Vallejo]

The code 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>
---
v3:
  * Hunks taken from v2/patch4 (Jan)
---
 xen/arch/x86/cpu/microcode/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index e65af4b82e..df7e1df870 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);
 }
 
@@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
         break;
     }
 
+    if ( ucode_ops.collect_cpu_info )
+        ucode_ops.collect_cpu_info();
+
     if ( !ucode_ops.apply_microcode )
     {
         printk(XENLOG_WARNING "Microcode loading not available\n");
@@ -868,8 +872,6 @@ int __init early_microcode_init(unsigned long *module_map,
 
     microcode_grab_module(module_map, mbi);
 
-    ucode_ops.collect_cpu_info();
-
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
-- 
2.34.1

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

[Alejandro Vallejo]

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

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 */
-- 
2.34.1

[PATCH v3 3/5] 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>
---
v3:
  * Moved from v2/patch3 (Andrew)
---
 xen/arch/x86/cpu/microcode/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 530e3e8267..1554fa38eb 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -860,6 +860,14 @@ int __init early_microcode_init(unsigned long *module_map,
     if ( ucode_ops.collect_cpu_info )
         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 )
+        ucode_ops.apply_microcode = NULL;
+
     if ( !ucode_ops.apply_microcode )
     {
         printk(XENLOG_WARNING "Microcode loading not available\n");
-- 
2.34.1

[PATCH v3 4/5] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo]

Move MSR_ARCH_CAPS read code from tsx_init() to immediately after the
early microcode update. This helps keep the reload closer to its cause
and is the earliest point we can read it, as it might be exposed only after
a microcode update.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v3:
  * Replaces v2/patch2. Moved after the "rev == ~0" check (Andrew)
---
 xen/arch/x86/cpu/microcode/core.c | 13 +++++++++++++
 xen/arch/x86/tsx.c                | 15 +++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 1554fa38eb..ef3c94018c 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -879,5 +879,18 @@ int __init early_microcode_init(unsigned long *module_map,
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
+    /*
+     * We might have exposed MSR_ARCH_CAPS 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..11e9471180 100644
--- a/xen/arch/x86/tsx.c
+++ b/xen/arch/x86/tsx.c
@@ -39,9 +39,9 @@ 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_microcode_init() has already
+     * prepared the feature bits needed here after the microcode update.
      */
     if ( unlikely(!once) )
     {
@@ -49,15 +49,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 v3 5/5] 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).

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v3:
  * Removed microcode_update_one() hunk (Jan, from v2/patch4)
  * Read MSR_ARCH_CAPS in early_cpu_init(), missing in v2/patch4 (Andy)
  * Moved the MSR_ARCH_CAPS after-update read (it's on v3/p3 now)
  * Logic previouslyin this_cpu_can_install_update() is now integrated in
    intel_get_ucode_ops() (Jan, from v2/p4)
---
 xen/arch/x86/cpu/common.c             |  5 +++++
 xen/arch/x86/cpu/microcode/intel.c    | 14 ++++++++++++++
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 +++++
 4 files changed, 25 insertions(+)

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/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index a99e402b98..abcfdc460d 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode(
 
 void __init intel_get_ucode_ops(struct microcode_ops *ops)
 {
+    uint64_t mcu_ctrl;
+
     ops->cpu_request_microcode = cpu_request_microcode;
     ops->collect_cpu_info      = collect_cpu_info;
     ops->apply_microcode       = apply_microcode;
     ops->compare_patch         = compare_patch;
+
+    if ( cpu_has_mcu_ctrl )
+    {
+        rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
+        /*
+         * If DIS_MCU_LOAD is set applying microcode updates won't work. We
+         * can still query the current version and things like that, so
+         * we'll leave the other handlers in place.
+         */
+        if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD )
+            ops->apply_microcode = NULL;
+    }
 }
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index ace31e3b1f..0118171d7e 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)
 
 /* Synthesized. */
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 v2 0/5] Prevent attempting updates known to fail

[Alejandro Vallejo]

On Thu, Jun 15, 2023 at 04:48:29PM +0100, Alejandro Vallejo wrote:
> v3:
>   * Lots of hunks moved around. Individually mentioned in each patch
>   * Removed a redundant check
>   * Ignore microcode interface if the revision is -1
>   * Perform the DIS_MCU_LOAD checks during init rather than apply time

You might think that looks a whole lot like the enumeration in v2, and
you'd be right :) . I sent this ahead of time and that's incorrect. The
actual summary is:

  * Lots of hunks moved around. Individually mentioned in each patch
  * MSR_ARCH_CAPS read in early_cpu_init()
  * Per-vendor functions used to encapsulate vendor-specific logic in
    early_microcode_init()
  * Inlined previous helper for realoading 7d0

It addresses most concerns raised, but the final look is a bit different
from what was reviewed before. I think it's a lot cleaner and allows future
per-vendor logic to be neatly integrated into their per-vendor files.

Alejandro

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

[Jan Beulich]

On 15.06.2023 17:48, Alejandro Vallejo wrote:
> The code 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>

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

[Jan Beulich]

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

Re: [PATCH v3 3/5] x86/microcode: Ignore microcode loading interface for revision = -1

[Jan Beulich]

On 15.06.2023 17:48, Alejandro Vallejo wrote:
> 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>

Aiui this is independent of patch 2, and hence could go in ahead of it?

Jan

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

[Andrew Cooper]

On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index e65af4b82e..df7e1df870 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -750,11 +750,12 @@ __initcall(microcode_init);
> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
>          break;
>      }
>  
> +    if ( ucode_ops.collect_cpu_info )
> +        ucode_ops.collect_cpu_info();
> +

I still think this wants to be the other side of "ucode loading fully
unavailable", just below.

I'm confident it will result in easier-to-follow logic.

~Andrew

>      if ( !ucode_ops.apply_microcode )
>      {
>          printk(XENLOG_WARNING "Microcode loading not available\n");
>

Re: [PATCH v3 4/5] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Jan Beulich]

On 15.06.2023 17:48, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -879,5 +879,18 @@ int __init early_microcode_init(unsigned long *module_map,
>      if ( ucode_mod.mod_end || ucode_blob.size )
>          rc = early_microcode_update_cpu();
>  
> +    /*
> +     * We might have exposed MSR_ARCH_CAPS after the microcode update.

I'm struggling a little with this sentence, but not being a native speaker
it may be me, not the sentence. I would perhaps have said "MSR_ARCH_CAPS
may have appeared with the microcode update."

> +     * Reload relevant fields in boot_cpu_data if so because they are
> +     * needed in tsx_init()

Nit: Missing full stop.

I also wonder whether you wouldn't want to insert "e.g.", since iirc with
the next patch tsx_init() isn't going to be the only user anymore.

> +     */
> +    if ( boot_cpu_data.cpuid_level >= 7 )
> +        boot_cpu_data.x86_capability[FEATURESET_7d0]
> +            = cpuid_count_edx(7, 0);

I take it we assume the maximum CPUID level won't go from below 7 to 7
or higher with the ucode update?

> --- a/xen/arch/x86/tsx.c
> +++ b/xen/arch/x86/tsx.c
> @@ -39,9 +39,9 @@ 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_microcode_init() has already
> +     * prepared the feature bits needed here after the microcode update.

Is this true in all cases? early_microcode_init() may have bailed
early, so I think you need to further transform early_microcode_init()
(and as a personal request of mine preferably without goto).

Jan

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

[Jan Beulich]

On 19.06.2023 17:49, Andrew Cooper wrote:
> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>> index e65af4b82e..df7e1df870 100644
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -750,11 +750,12 @@ __initcall(microcode_init);
>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
>>          break;
>>      }
>>  
>> +    if ( ucode_ops.collect_cpu_info )
>> +        ucode_ops.collect_cpu_info();
>> +
> 
> I still think this wants to be the other side of "ucode loading fully
> unavailable", just below.
> 
> I'm confident it will result in easier-to-follow logic.

Yet wouldn't that be against the purpose of obtaining the ucode
revision even if loading isn't possible?

Jan

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

[Andrew Cooper]

On 19/06/2023 4:58 pm, Jan Beulich wrote:
> On 19.06.2023 17:49, Andrew Cooper wrote:
>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>> index e65af4b82e..df7e1df870 100644
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -750,11 +750,12 @@ __initcall(microcode_init);
>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
>>>          break;
>>>      }
>>>  
>>> +    if ( ucode_ops.collect_cpu_info )
>>> +        ucode_ops.collect_cpu_info();
>>> +
>> I still think this wants to be the other side of "ucode loading fully
>> unavailable", just below.
>>
>> I'm confident it will result in easier-to-follow logic.
> Yet wouldn't that be against the purpose of obtaining the ucode
> revision even if loading isn't possible?

No.  The logical order of checks is:

if ( !ops.apply )
    return; // Fully unavailable

collect_cpu_info();

if ( rev == ~0 || !can_load )
    return; // Loading unavailable

// search for blob to load


This form has fewer misleading NULL checks, and doesn't get
printk(XENLOG_WARNING "Microcode loading not available\n"); after
successful microcode actions.

~Andrew

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

[Jan Beulich]

On 19.06.2023 18:06, Andrew Cooper wrote:
> On 19/06/2023 4:58 pm, Jan Beulich wrote:
>> On 19.06.2023 17:49, Andrew Cooper wrote:
>>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>>> index e65af4b82e..df7e1df870 100644
>>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>>> @@ -750,11 +750,12 @@ __initcall(microcode_init);
>>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
>>>>          break;
>>>>      }
>>>>  
>>>> +    if ( ucode_ops.collect_cpu_info )
>>>> +        ucode_ops.collect_cpu_info();
>>>> +
>>> I still think this wants to be the other side of "ucode loading fully
>>> unavailable", just below.
>>>
>>> I'm confident it will result in easier-to-follow logic.
>> Yet wouldn't that be against the purpose of obtaining the ucode
>> revision even if loading isn't possible?
> 
> No.  The logical order of checks is:
> 
> if ( !ops.apply )
>     return; // Fully unavailable
> 
> collect_cpu_info();
> 
> if ( rev == ~0 || !can_load )
>     return; // Loading unavailable
> 
> // search for blob to load
> 
> 
> This form has fewer misleading NULL checks, and doesn't get
> printk(XENLOG_WARNING "Microcode loading not available\n"); after
> successful microcode actions.

But from the earlier version and from what I've seen in patches 1-4
so far, I expect patch 5 will introduce a case with ops.apply being
NULL but ops.collect being non-NULL. Otherwise I don't see why patch
2 does what it does (sacrificing cf_clobber, albeit likely not really
intentionally).

Jan

Re: [PATCH v3 5/5] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Jan Beulich]

On 15.06.2023 17:48, Alejandro Vallejo wrote:
> --- 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]);

Is this still going to be needed if early_microcode_init() uniformly
does this, for things to be correct for tsx_init() (as pointed out
in the other patch)?

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>  
>  void __init intel_get_ucode_ops(struct microcode_ops *ops)
>  {
> +    uint64_t mcu_ctrl;
> +
>      ops->cpu_request_microcode = cpu_request_microcode;
>      ops->collect_cpu_info      = collect_cpu_info;
>      ops->apply_microcode       = apply_microcode;
>      ops->compare_patch         = compare_patch;
> +
> +    if ( cpu_has_mcu_ctrl )
> +    {
> +        rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> +        /*
> +         * If DIS_MCU_LOAD is set applying microcode updates won't work. We
> +         * can still query the current version and things like that, so
> +         * we'll leave the other handlers in place.
> +         */
> +        if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD )
> +            ops->apply_microcode = NULL;
> +    }

While this won't address Andrew's request (afaict), but only the
cf_clobber requirement, I think you want to drop removing of the struct
instances from patch 2, return pointers from the new per-vendor
functions, and simply introduce another static instance of struct
microcode_ops here, with the one hook simply left unset. This lives in
init data, so the size increase is of no major concern.

Jan

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

[Jan Beulich]

On 19.06.2023 18:10, Jan Beulich wrote:
> On 19.06.2023 18:06, Andrew Cooper wrote:
>> On 19/06/2023 4:58 pm, Jan Beulich wrote:
>>> On 19.06.2023 17:49, Andrew Cooper wrote:
>>>> On 15/06/2023 4:48 pm, Alejandro Vallejo wrote:
>>>>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>>>>> index e65af4b82e..df7e1df870 100644
>>>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>>>> @@ -750,11 +750,12 @@ __initcall(microcode_init);
>>>>> @@ -860,6 +861,9 @@ int __init early_microcode_init(unsigned long *module_map,
>>>>>          break;
>>>>>      }
>>>>>  
>>>>> +    if ( ucode_ops.collect_cpu_info )
>>>>> +        ucode_ops.collect_cpu_info();
>>>>> +
>>>> I still think this wants to be the other side of "ucode loading fully
>>>> unavailable", just below.
>>>>
>>>> I'm confident it will result in easier-to-follow logic.
>>> Yet wouldn't that be against the purpose of obtaining the ucode
>>> revision even if loading isn't possible?
>>
>> No.  The logical order of checks is:
>>
>> if ( !ops.apply )
>>     return; // Fully unavailable
>>
>> collect_cpu_info();
>>
>> if ( rev == ~0 || !can_load )
>>     return; // Loading unavailable
>>
>> // search for blob to load
>>
>>
>> This form has fewer misleading NULL checks, and doesn't get
>> printk(XENLOG_WARNING "Microcode loading not available\n"); after
>> successful microcode actions.
> 
> But from the earlier version and from what I've seen in patches 1-4
> so far, I expect patch 5 will introduce a case with ops.apply being
> NULL but ops.collect being non-NULL. Otherwise I don't see why patch
> 2 does what it does (sacrificing cf_clobber, albeit likely not really
> intentionally).

As expected with patch 5 ops.apply can be NULL without indicating
"fully unavailable". collect_cpu_info being NULL could be taken as
indicator of "fully unavailable", though.

Jan

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

[Alejandro Vallejo]

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

Re: [PATCH v3 4/5] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Alejandro Vallejo]

On Mon, Jun 19, 2023 at 05:57:14PM +0200, Jan Beulich wrote:
> > +     * We might have exposed MSR_ARCH_CAPS after the microcode update.
> 
> I'm struggling a little with this sentence, but not being a native speaker
> it may be me, not the sentence. I would perhaps have said "MSR_ARCH_CAPS
> may have appeared with the microcode update."
Sure, works for me.

> I also wonder whether you wouldn't want to insert "e.g.", since iirc with
> the next patch tsx_init() isn't going to be the only user anymore.
tsx_init() is the only user, as far as I have seen. DIS_MCU_LOAD is checked
before the update, using the cached data read in early_cpu_init()

> 
> > +     */
> > +    if ( boot_cpu_data.cpuid_level >= 7 )
> > +        boot_cpu_data.x86_capability[FEATURESET_7d0]
> > +            = cpuid_count_edx(7, 0);
> 
> I take it we assume the maximum CPUID level won't go from below 7 to 7
> or higher with the ucode update?
Do you mean from >=7 to <7 instead? Otherwise it just works and I don't
undertand the concern.

If so, that's an impossibly unlikely case and the current code does not try
to clean up in that case.

> 
> > --- a/xen/arch/x86/tsx.c
> > +++ b/xen/arch/x86/tsx.c
> > @@ -39,9 +39,9 @@ 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_microcode_init() has already
> > +     * prepared the feature bits needed here after the microcode update.
> 
> Is this true in all cases? early_microcode_init() may have bailed
> early, so I think you need to further transform early_microcode_init()
> (and as a personal request of mine preferably without goto).
> 
> Jan
The series is eventually correct because MSR_ARCH_CAPS are also collected
in early_cpu_init(). Alas, that's not the case here. You're right. I'll
move the early MSR_ARCH_CAPS read to this patch as well.

Alejandro

Re: [PATCH v3 5/5] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Alejandro Vallejo]

On Tue, Jun 20, 2023 at 11:51:00AM +0200, Jan Beulich wrote:
> On 15.06.2023 17:48, Alejandro Vallejo wrote:
> > --- 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]);
> 
> Is this still going to be needed if early_microcode_init() uniformly
> does this, for things to be correct for tsx_init() (as pointed out
> in the other patch)?
Yes. This is needed so MSR_ARCH_CAPS are available in order to check
DIS_MCU_LOAD before the microcode update itself. early_cpu_init() does the
read before the microcode update, while early_microcode_init() refreshes
the fields that might have been modified after the update AND are needed
before the general CPU detection logic.

> 
> > --- a/xen/arch/x86/cpu/microcode/intel.c
> > +++ b/xen/arch/x86/cpu/microcode/intel.c
> > @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode(
> >  
> >  void __init intel_get_ucode_ops(struct microcode_ops *ops)
> >  {
> > +    uint64_t mcu_ctrl;
> > +
> >      ops->cpu_request_microcode = cpu_request_microcode;
> >      ops->collect_cpu_info      = collect_cpu_info;
> >      ops->apply_microcode       = apply_microcode;
> >      ops->compare_patch         = compare_patch;
> > +
> > +    if ( cpu_has_mcu_ctrl )
> > +    {
> > +        rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> > +        /*
> > +         * If DIS_MCU_LOAD is set applying microcode updates won't work. We
> > +         * can still query the current version and things like that, so
> > +         * we'll leave the other handlers in place.
> > +         */
> > +        if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD )
> > +            ops->apply_microcode = NULL;
> > +    }
> 
> While this won't address Andrew's request (afaict), but only the
> cf_clobber requirement, I think you want to drop removing of the struct
> instances from patch 2, return pointers from the new per-vendor
> functions, and simply introduce another static instance of struct
> microcode_ops here, with the one hook simply left unset. This lives in
> init data, so the size increase is of no major concern.
> 
> Jan
As mentioned in other email. This is my bad for not having understood the
ultimate readon for cf_clobber. I'll just leave the structs as is.

Alejandro

Re: [PATCH v3 4/5] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()

[Jan Beulich]

On 22.06.2023 16:55, Alejandro Vallejo wrote:
> On Mon, Jun 19, 2023 at 05:57:14PM +0200, Jan Beulich wrote:
>>> +    if ( boot_cpu_data.cpuid_level >= 7 )
>>> +        boot_cpu_data.x86_capability[FEATURESET_7d0]
>>> +            = cpuid_count_edx(7, 0);
>>
>> I take it we assume the maximum CPUID level won't go from below 7 to 7
>> or higher with the ucode update?
> Do you mean from >=7 to <7 instead? Otherwise it just works and I don't
> undertand the concern.

No, I mean going from <7 to >=7. In such a case the earlier
recorded .cpuid_level will prevent the leaf 7 read, and hence also
the possible discovery of ARCH_CAPS having appeared.

I actually wonder whether it wouldn't be better to re-run the whole
of early_cpu_init() from here again.

Jan