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

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

[Alejandro Vallejo]

  [This series spun out of a previous patch with the same name, with this
   series adding more related enhancements. Kept as v2 to preserve the
   linkage with previous suggestions.]

v2:
  * 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 removes a redundant family check in the Intel init path.

Patch 2 moves an early read of MSR_ARCH_CAPS from tsx_init() back to
        immediately after the early microcode load.

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

Patch 4 Adds support for DIS_MCU_LOAD during init time.

Alejandro Vallejo (4):
  x86/microcode: Remove Intel's family check on early_microcode_init()
  x86: Read MSR_ARCH_CAPS after early_microcode_init()
  x86/microcode: Ignore microcode loading interface for revision = -1
  x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

 xen/arch/x86/cpu/microcode/core.c     | 79 +++++++++++++++++++++++++--
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 ++
 xen/arch/x86/tsx.c                    | 15 +----
 4 files changed, 82 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] x86/microcode: Remove Intel's family check on early_microcode_init()

[Alejandro Vallejo]

Intel only suuports 64bits on families 6 and 15, so we can always assume
microcode loading facilities are present and skip the check.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2:
  * New addition
---
 xen/arch/x86/cpu/microcode/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index c1033f3bc2..29ff38f35c 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -854,8 +854,14 @@ int __init early_microcode_init(unsigned long *module_map,
         break;
 
     case X86_VENDOR_INTEL:
-        if ( c->x86 >= 6 )
-            ucode_ops = intel_ucode_ops;
+        /*
+         * Intel introduced microcode loading with family 6. Because we
+         * don't support compiling Xen for 32bit machines we're guaranteed
+         * that at this point we're either in family 15 (Pentium 4) or 6
+         * (everything since then), so microcode facilities are always
+         * present.
+         */
+        ucode_ops = intel_ucode_ops;
         break;
     }
 
-- 
2.34.1

[PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Alejandro Vallejo]

tsx_init() has some ad-hoc code to read MSR_ARCH_CAPS if present. In order
to suuport DIS_MCU_UPDATE we need access to it earlier, so this patch moves
early read to the tail of early_microcode_init(), after the early microcode
update.

The read of the 7d0 CPUID leaf is left in a helper because it's reused in a
later patch.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
I suspect there was an oversight in tsx_init() by which
boot_cpu_data.cpuid_level was never read? The first read I can
see is in identify_cpu(), which happens after tsx_init().

v2:
  * New addition
---
 xen/arch/x86/cpu/microcode/core.c | 21 +++++++++++++++++++++
 xen/arch/x86/tsx.c                | 15 +++------------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 29ff38f35c..892bcec901 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void)
     return microcode_update_cpu(patch);
 }
 
+static void __init early_read_cpuid_7d0(void)
+{
+    boot_cpu_data.cpuid_level = cpuid_eax(0);
+
+    if ( boot_cpu_data.cpuid_level >= 7 )
+        boot_cpu_data.x86_capability[FEATURESET_7d0]
+            = cpuid_count_edx(7, 0);
+}
+
 int __init early_microcode_init(unsigned long *module_map,
                                 const struct multiboot_info *mbi)
 {
@@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
+    early_read_cpuid_7d0();
+
+    /*
+     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
+     * populates boot_cpu_data, so we read it here to centralize early
+     * CPUID/MSR reads in the same place.
+     */
+    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..0501e181bf 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.
+     * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES
+     * and leaf 7d0 have already been read if present after early microcode
+     * loading time. So we can assume _those_ are present.
      */
     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 v2 3/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>
---
v2:
  * New addition
---
 xen/arch/x86/cpu/microcode/core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 892bcec901..4f60d96d98 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -874,6 +874,21 @@ int __init early_microcode_init(unsigned long *module_map,
         break;
     }
 
+    if ( ucode_ops.collect_cpu_info )
+        ucode_ops.collect_cpu_info();
+
+    /*
+     * This is a special case for virtualized Xen. 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 )
+    {
+        this_cpu(cpu_sig) = (struct cpu_signature){ 0 };
+        ucode_ops = (struct microcode_ops){ 0 };
+    }
+
     if ( !ucode_ops.apply_microcode )
     {
         printk(XENLOG_WARNING "Microcode loading not available\n");
@@ -882,8 +897,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 v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD 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. This patch makes sure we
only expose the microcode loading interface when it can be actually used,
while also allowing reads of current microcode versions.

Patch summary:
 * Read CPUID leaf 7d0 early so DIS_MCU_LOAD can be checked.
 * Hide microcode loading handlers if DIS_MCU_LOAD is set except for
   collect_cpu_info()
 * Update microcode_update_one() so APs can read their
   microcode version even if DIS_MCU_LOAD is set.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

---
v2:
  * Moved check from apply time to init time.
---
 xen/arch/x86/cpu/microcode/core.c     | 31 +++++++++++++++++++++++++--
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/include/asm/msr-index.h  |  5 +++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 4f60d96d98..a4c123118b 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -749,11 +749,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);
 }
 
@@ -849,12 +850,25 @@ static void __init early_read_cpuid_7d0(void)
             = cpuid_count_edx(7, 0);
 }
 
+static bool __init this_cpu_can_install_update(void)
+{
+    uint64_t mcu_ctrl;
+
+    if ( !cpu_has_mcu_ctrl )
+        return true;
+
+    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
+    return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
+}
+
 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;
 
+    early_read_cpuid_7d0();
+
     switch ( c->x86_vendor )
     {
     case X86_VENDOR_AMD:
@@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long *module_map,
          * present.
          */
         ucode_ops = intel_ucode_ops;
+
+        /*
+         * In the case where microcode updates are blocked by the
+         * DIS_MCU_LOAD bit we can still read the microcode version even if
+         * we can't change it.
+         */
+        if ( !this_cpu_can_install_update() )
+            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
+                                    intel_ucode_ops.collect_cpu_info };
         break;
     }
 
@@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map,
     if ( ucode_mod.mod_end || ucode_blob.size )
         rc = early_microcode_update_cpu();
 
+    /*
+     * We just updated microcode so we must reload the boot_cpu_data bits
+     * we read before because they might be stale after the updata.
+     */
     early_read_cpuid_7d0();
 
     /*
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 1/4] x86/microcode: Remove Intel's family check on early_microcode_init()

[Jan Beulich]

On 05.06.2023 19:08, Alejandro Vallejo wrote:

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -854,8 +854,14 @@ int __init early_microcode_init(unsigned long *module_map,
>          break;
>  
>      case X86_VENDOR_INTEL:
> -        if ( c->x86 >= 6 )
> -            ucode_ops = intel_ucode_ops;
> +        /*
> +         * Intel introduced microcode loading with family 6. Because we
> +         * don't support compiling Xen for 32bit machines we're guaranteed
> +         * that at this point we're either in family 15 (Pentium 4) or 6
> +         * (everything since then), so microcode facilities are always
> +         * present.
> +         */
> +        ucode_ops = intel_ucode_ops;
>          break;
>      }

There are many places where we make such connections / assumptions without
long comments. I'd be okay with a brief one, but I'm not convinced we need
one at all.

Jan

Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Jan Beulich]

On 05.06.2023 19:08, Alejandro Vallejo wrote:
> tsx_init() has some ad-hoc code to read MSR_ARCH_CAPS if present. In order
> to suuport DIS_MCU_UPDATE we need access to it earlier, so this patch moves
> early read to the tail of early_microcode_init(), after the early microcode
> update.
> 
> The read of the 7d0 CPUID leaf is left in a helper because it's reused in a
> later patch.
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> I suspect there was an oversight in tsx_init() by which
> boot_cpu_data.cpuid_level was never read? The first read I can
> see is in identify_cpu(), which happens after tsx_init().

See early_cpu_init(). (I have to admit that I was also struggling with
your use of "read": Aiui you mean the field was never written / set,
and "read" really refers to the reading of the corresponding CPUID
leaf.)

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void)
>      return microcode_update_cpu(patch);
>  }
>  
> +static void __init early_read_cpuid_7d0(void)
> +{
> +    boot_cpu_data.cpuid_level = cpuid_eax(0);

As per above I don't think this is needed.

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

This is actually filled in early_cpu_init() as well, so doesn't need
re-doing here unless because of a suspected change to the value (but
then other CPUID output may have changed, too). At which point ...

> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
>      if ( ucode_mod.mod_end || ucode_blob.size )
>          rc = early_microcode_update_cpu();
>  
> +    early_read_cpuid_7d0();
> +
> +    /*
> +     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
> +     * populates boot_cpu_data, so we read it here to centralize early
> +     * CPUID/MSR reads in the same place.
> +     */
> +    if ( cpu_has_arch_caps )
> +        rdmsr(MSR_ARCH_CAPABILITIES,
> +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
> +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);

... "centralize" aspect goes away, and hence the comment needs adjusting.

> --- 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.
> +     * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES
> +     * and leaf 7d0 have already been read if present after early microcode
> +     * loading time. So we can assume _those_ are present.
>       */
>      if ( unlikely(!once) )
>      {

I think I'd like to see at least the initial part of the original comment
retained here.

Jan

Re: [PATCH v2 1/4] x86/microcode: Remove Intel's family check on early_microcode_init()

[Andrew Cooper]

On 12/06/2023 4:16 pm, Jan Beulich wrote:
> On 05.06.2023 19:08, Alejandro Vallejo wrote:
>
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -854,8 +854,14 @@ int __init early_microcode_init(unsigned long *module_map,
>>          break;
>>  
>>      case X86_VENDOR_INTEL:
>> -        if ( c->x86 >= 6 )
>> -            ucode_ops = intel_ucode_ops;
>> +        /*
>> +         * Intel introduced microcode loading with family 6. Because we
>> +         * don't support compiling Xen for 32bit machines we're guaranteed
>> +         * that at this point we're either in family 15 (Pentium 4) or 6
>> +         * (everything since then), so microcode facilities are always
>> +         * present.
>> +         */
>> +        ucode_ops = intel_ucode_ops;
>>          break;
>>      }
> There are many places where we make such connections / assumptions without
> long comments. I'd be okay with a brief one, but I'm not convinced we need
> one at all.

I agree.  I don't think we need a comment here.

I'd also tweak the commit message to say "All 64bit-capable Intel CPUs
are supported as far as microcode loading goes" or similar.  It's subtly
different IMO.

The Intel microcode driver already relies on 64bit-ness to exclude and
early case (on 32bit CPUs only) which lack Platform Flags.

I'm happy to fix both of these up on commit.

~Andrew

Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Andrew Cooper]

On 12/06/2023 4:46 pm, Jan Beulich wrote:
> On 05.06.2023 19:08, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void)
>>      return microcode_update_cpu(patch);
>>  }
>>  
>> +static void __init early_read_cpuid_7d0(void)
>> +{
>> +    boot_cpu_data.cpuid_level = cpuid_eax(0);
> As per above I don't think this is needed.
>
>> +    if ( boot_cpu_data.cpuid_level >= 7 )
>> +        boot_cpu_data.x86_capability[FEATURESET_7d0]
>> +            = cpuid_count_edx(7, 0);
> This is actually filled in early_cpu_init() as well, so doesn't need
> re-doing here unless because of a suspected change to the value (but
> then other CPUID output may have changed, too).

Hmm, yes.  I suspect that is due to the CET series (which needed to know
7d0 much earlier than previously), and me forgetting to clean up tsx_init().

>  At which point ...
>
>> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
>>      if ( ucode_mod.mod_end || ucode_blob.size )
>>          rc = early_microcode_update_cpu();
>>  
>> +    early_read_cpuid_7d0();
>> +
>> +    /*
>> +     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
>> +     * populates boot_cpu_data, so we read it here to centralize early
>> +     * CPUID/MSR reads in the same place.
>> +     */
>> +    if ( cpu_has_arch_caps )
>> +        rdmsr(MSR_ARCH_CAPABILITIES,
>> +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
>> +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
> ... "centralize" aspect goes away, and hence the comment needs adjusting.

I find it weird splitting apart the various reads into x86_capability[],
but in light of the feedback, only the rdmsr() needs to stay.

>
>> --- 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.
>> +     * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES
>> +     * and leaf 7d0 have already been read if present after early microcode
>> +     * loading time. So we can assume _those_ are present.
>>       */
>>      if ( unlikely(!once) )
>>      {
> I think I'd like to see at least the initial part of the original comment
> retained here.

The first sentence needs to stay as-is.  That's still relevant even with
the feature handling moved out.

The second sentence wants to say something like "However,
microcode_init() has already prepared the feature bits we need." because
it's the justification of why we don't do it here.

~Andrew

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

[Andrew Cooper]

On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index 892bcec901..4f60d96d98 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -874,6 +874,21 @@ int __init early_microcode_init(unsigned long *module_map,
>          break;
>      }
>  
> +    if ( ucode_ops.collect_cpu_info )
> +        ucode_ops.collect_cpu_info();
> +
> +    /*
> +     * This is a special case for virtualized Xen.

I'm not sure this first sentence is useful.  I'd just start with "Some
hypervisors ..."

>  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 )
> +    {
> +        this_cpu(cpu_sig) = (struct cpu_signature){ 0 };
> +        ucode_ops = (struct microcode_ops){ 0 };

I think we want to retain XENPF_get_ucode_revision's ability to see this ~0.

As with the following patch, we want to retain the ability to query, so
leave cpu_sig alone and only remove the apply_microcode hook.  In turn,
that probably means this wants to be an else if in the next clause down.

Moving it down also means you can drop the check for collect_cpu_info,
because it's a mandatory hook if ucode_ops was filled in.

~Andrew

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

Re: [PATCH v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

[Andrew Cooper]

On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index 4f60d96d98..a4c123118b 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long *module_map,
>           * present.
>           */
>          ucode_ops = intel_ucode_ops;
> +
> +        /*
> +         * In the case where microcode updates are blocked by the
> +         * DIS_MCU_LOAD bit we can still read the microcode version even if
> +         * we can't change it.
> +         */
> +        if ( !this_cpu_can_install_update() )
> +            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> +                                    intel_ucode_ops.collect_cpu_info };

I don't see how this (the logic in this_cpu_can_install_update()) can
work, as ...

>          break;
>      }
>  
> @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map,
>      if ( ucode_mod.mod_end || ucode_blob.size )
>          rc = early_microcode_update_cpu();
>  
> +    /*
> +     * We just updated microcode so we must reload the boot_cpu_data bits
> +     * we read before because they might be stale after the updata.
> +     */
>      early_read_cpuid_7d0();
>  
>      /*

... MSR_ARCH_CAPS is read out-of-context down here.

In hindsight, I think swapping patch 2 and 3 might be wise.  The rev ==
~0 case doesn't need any of the cpu_has_* shuffling, and it already
starts to build up the if/else chain of cases where we decide to clobber
the apply_microcode() hook.

The call to this_cpu_can_install_update() should be lower down.  In
principle it's not Intel-specific.

~Andrew

Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Jan Beulich]

On 12.06.2023 20:25, Andrew Cooper wrote:
> On 12/06/2023 4:46 pm, Jan Beulich wrote:
>> On 05.06.2023 19:08, Alejandro Vallejo wrote:
>>> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
>>>      if ( ucode_mod.mod_end || ucode_blob.size )
>>>          rc = early_microcode_update_cpu();
>>>  
>>> +    early_read_cpuid_7d0();
>>> +
>>> +    /*
>>> +     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
>>> +     * populates boot_cpu_data, so we read it here to centralize early
>>> +     * CPUID/MSR reads in the same place.
>>> +     */
>>> +    if ( cpu_has_arch_caps )
>>> +        rdmsr(MSR_ARCH_CAPABILITIES,
>>> +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
>>> +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
>> ... "centralize" aspect goes away, and hence the comment needs adjusting.
> 
> I find it weird splitting apart the various reads into x86_capability[],
> but in light of the feedback, only the rdmsr() needs to stay.

Hmm, wait: When updating a CPU from a pre-arch-caps ucode level on one
that supports arch-caps, don't we need to re-read 7d0 here? (I.e. the
call to early_read_cpuid_7d0() needs to stay, but for a reason
different from the one presently stated in the description, and
possibly even worth a brief comment.)

Jan

Re: [PATCH v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

[Jan Beulich]

On 05.06.2023 19:08, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -749,11 +749,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);
>  }

This adjustment (and related logic below) doesn't really fit the purpose
that the title states. I wonder if the two things wouldn't better be
split.

> @@ -849,12 +850,25 @@ static void __init early_read_cpuid_7d0(void)
>              = cpuid_count_edx(7, 0);
>  }
>  
> +static bool __init this_cpu_can_install_update(void)
> +{
> +    uint64_t mcu_ctrl;
> +
> +    if ( !cpu_has_mcu_ctrl )
> +        return true;
> +
> +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> +    return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
> +}

As Andrew says, in principle AMD could have a means as well. Surely that
would be a different one, so I wonder if this shouldn't be a new hook.

> @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long *module_map,
>           * present.
>           */
>          ucode_ops = intel_ucode_ops;
> +
> +        /*
> +         * In the case where microcode updates are blocked by the
> +         * DIS_MCU_LOAD bit we can still read the microcode version even if
> +         * we can't change it.
> +         */
> +        if ( !this_cpu_can_install_update() )
> +            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> +                                    intel_ucode_ops.collect_cpu_info };

Similarly I'm not happy to see a direct reference to some vendor specific
field. I think it wants to be the hook to return such an override struct.

Jan

Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Andrew Cooper]

On 13/06/2023 7:40 am, Jan Beulich wrote:
> On 12.06.2023 20:25, Andrew Cooper wrote:
>> On 12/06/2023 4:46 pm, Jan Beulich wrote:
>>> On 05.06.2023 19:08, Alejandro Vallejo wrote:
>>>> @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
>>>>      if ( ucode_mod.mod_end || ucode_blob.size )
>>>>          rc = early_microcode_update_cpu();
>>>>  
>>>> +    early_read_cpuid_7d0();
>>>> +
>>>> +    /*
>>>> +     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
>>>> +     * populates boot_cpu_data, so we read it here to centralize early
>>>> +     * CPUID/MSR reads in the same place.
>>>> +     */
>>>> +    if ( cpu_has_arch_caps )
>>>> +        rdmsr(MSR_ARCH_CAPABILITIES,
>>>> +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
>>>> +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
>>> ... "centralize" aspect goes away, and hence the comment needs adjusting.
>> I find it weird splitting apart the various reads into x86_capability[],
>> but in light of the feedback, only the rdmsr() needs to stay.
> Hmm, wait: When updating a CPU from a pre-arch-caps ucode level on one
> that supports arch-caps, don't we need to re-read 7d0 here? (I.e. the
> call to early_read_cpuid_7d0() needs to stay, but for a reason
> different from the one presently stated in the description, and
> possibly even worth a brief comment.)

Urgh yes.  We do have situations where this ucode load will cause
MSR_ARCH_CAPS (and features there-within) to appear.

I'll rethink the safety here when I've got some breathing room from
other tasks.

~Andrew

Re: [PATCH v2 1/4] x86/microcode: Remove Intel's family check on early_microcode_init()

[Alejandro Vallejo]

On Mon, Jun 12, 2023 at 06:31:13PM +0100, Andrew Cooper wrote:
> On 12/06/2023 4:16 pm, Jan Beulich wrote:
> > There are many places where we make such connections / assumptions without
> > long comments. I'd be okay with a brief one, but I'm not convinced we need
> > one at all.
> 
> I agree.  I don't think we need a comment here.
> 
> I'd also tweak the commit message to say "All 64bit-capable Intel CPUs
> are supported as far as microcode loading goes" or similar.  It's subtly
> different IMO.
> 
> The Intel microcode driver already relies on 64bit-ness to exclude and
> early case (on 32bit CPUs only) which lack Platform Flags.
> 
> I'm happy to fix both of these up on commit.
> 
> ~Andrew

Sure, I don't have a strong preference either way. I'll remove that for the
new series.

Alejandro

Re: [PATCH v2 2/4] x86: Read MSR_ARCH_CAPS after early_microcode_init()

[Alejandro Vallejo]

On Mon, Jun 12, 2023 at 05:46:07PM +0200, Jan Beulich wrote:
> See early_cpu_init().
Ah, I missed that. I didn't expect several leafs to be read at once.
I see now that that function takes care of 

> (I have to admit that I was also struggling with
> your use of "read": Aiui you mean the field was never written / set,
> and "read" really refers to the reading of the corresponding CPUID
> leaf.)
Correct, though in retrospect that does sound misleading. I meant read from
the HW CPUID leaf.

> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -840,6 +840,15 @@ static int __init early_microcode_update_cpu(void)
> >      return microcode_update_cpu(patch);
> >  }
> >  
> > +static void __init early_read_cpuid_7d0(void)
> > +{
> > +    boot_cpu_data.cpuid_level = cpuid_eax(0);
> 
> As per above I don't think this is needed.
> 
> > +    if ( boot_cpu_data.cpuid_level >= 7 )
> > +        boot_cpu_data.x86_capability[FEATURESET_7d0]
> > +            = cpuid_count_edx(7, 0);
> 
> This is actually filled in early_cpu_init() as well, so doesn't need
> re-doing here unless because of a suspected change to the value (but
> then other CPUID output may have changed, too). At which point ...
MSR_ARCH_CAPS may genuinely appear only after a microcode update, so
re-reading 7d0 and the MSR itself is probably the sane thing to do.

> 
> > @@ -878,5 +887,17 @@ int __init early_microcode_init(unsigned long *module_map,
> >      if ( ucode_mod.mod_end || ucode_blob.size )
> >          rc = early_microcode_update_cpu();
> >  
> > +    early_read_cpuid_7d0();
> > +
> > +    /*
> > +     * tsx_init() needs MSR_ARCH_CAPS, but it runs before identify_cpu()
> > +     * populates boot_cpu_data, so we read it here to centralize early
> > +     * CPUID/MSR reads in the same place.
> > +     */
> > +    if ( cpu_has_arch_caps )
> > +        rdmsr(MSR_ARCH_CAPABILITIES,
> > +              boot_cpu_data.x86_capability[FEATURESET_m10Al],
> > +              boot_cpu_data.x86_capability[FEATURESET_m10Ah]);
> 
> ... "centralize" aspect goes away, and hence the comment needs adjusting.
Indeed. I'll rewrite that.

> > --- 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.
> > +     * While MSRs/CPUID haven't yet been scanned, MSR_ARCH_CAPABILITIES
> > +     * and leaf 7d0 have already been read if present after early microcode
> > +     * loading time. So we can assume _those_ are present.
> >       */
> >      if ( unlikely(!once) )
> >      {
> 
> I think I'd like to see at least the initial part of the original comment
> retained here.
Ack. Sure.

Alejandro

Re: [PATCH v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

[Alejandro Vallejo]

On Mon, Jun 12, 2023 at 07:54:00PM +0100, Andrew Cooper wrote:
> On 05/06/2023 6:08 pm, Alejandro Vallejo wrote:
> > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> > index 4f60d96d98..a4c123118b 100644
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long *module_map,
> >           * present.
> >           */
> >          ucode_ops = intel_ucode_ops;
> > +
> > +        /*
> > +         * In the case where microcode updates are blocked by the
> > +         * DIS_MCU_LOAD bit we can still read the microcode version even if
> > +         * we can't change it.
> > +         */
> > +        if ( !this_cpu_can_install_update() )
> > +            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> > +                                    intel_ucode_ops.collect_cpu_info };
> 
> I don't see how this (the logic in this_cpu_can_install_update()) can
> work, as ...
> 
> >          break;
> >      }
> >  
> > @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map,
> >      if ( ucode_mod.mod_end || ucode_blob.size )
> >          rc = early_microcode_update_cpu();
> >  
> > +    /*
> > +     * We just updated microcode so we must reload the boot_cpu_data bits
> > +     * we read before because they might be stale after the updata.
> > +     */
> >      early_read_cpuid_7d0();
> >  
> >      /*
> 
> ... MSR_ARCH_CAPS is read out-of-context down here.
Seeing how the minimal CPU state is read in early_cpu_init() I'll stash the
read to MSR_ARCH_CAPS there too. Then it's a matter of reloading
potentially changing leafs/MSRs after the update, which is a lot clearer
rather than adding reads/writes ad-hoc elsewhere.

Alejandro

Re: [PATCH v2 4/4] x86/microcode: Prevent attempting updates if DIS_MCU_LOAD is set

[Alejandro Vallejo]

On Tue, Jun 13, 2023 at 08:57:06AM +0200, Jan Beulich wrote:
> On 05.06.2023 19:08, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -749,11 +749,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);
> >  }
> 
> This adjustment (and related logic below) doesn't really fit the purpose
> that the title states. I wonder if the two things wouldn't better be
> split.
Well, before this patch collect_cpu_info() and apply_microcode() were both
set or cleared together , whereas after this patch that's no longer the
case (hence the change). I can submit it standalone patch earlier in v3,
seeing as it does no harm. The commit message could also do with better
wording.

> 
> > @@ -849,12 +850,25 @@ static void __init early_read_cpuid_7d0(void)
> >              = cpuid_count_edx(7, 0);
> >  }
> >  
> > +static bool __init this_cpu_can_install_update(void)
> > +{
> > +    uint64_t mcu_ctrl;
> > +
> > +    if ( !cpu_has_mcu_ctrl )
> > +        return true;
> > +
> > +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> > +    return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
> > +}
> 
> As Andrew says, in principle AMD could have a means as well. Surely that
> would be a different one, so I wonder if this shouldn't be a new hook.
> 
> > @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long *module_map,
> >           * present.
> >           */
> >          ucode_ops = intel_ucode_ops;
> > +
> > +        /*
> > +         * In the case where microcode updates are blocked by the
> > +         * DIS_MCU_LOAD bit we can still read the microcode version even if
> > +         * we can't change it.
> > +         */
> > +        if ( !this_cpu_can_install_update() )
> > +            ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> > +                                    intel_ucode_ops.collect_cpu_info };
> 
> Similarly I'm not happy to see a direct reference to some vendor specific
> field. I think it wants to be the hook to return such an override struct.
> 
> Jan
I'm moving things around a little in v3. We'll have accessors for both AMD
and Intel that provide the right thing, encapsulating vendor-specifics
(including family checks) inside ${VENDOR}.c

Alejandro