* [PATCH v6 0/3] Prevent attempting updates known to fail
@ 2023-07-24 16:52 Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1 Alejandro Vallejo
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-24 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
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 Ignores microcode facilities when the current microcode revision is -1
Patch 2 Moves the MSR_ARCH_CAPS read in tsx_init() to early_cpu_init() and
early_microcode_init()
Patch 3 Adds the logic to detect microcode updates being disabled on Intel.
Alejandro Vallejo (3):
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 | 20 ++++++++++----
xen/arch/x86/cpu/microcode/core.c | 38 ++++++++++++++++++++++++---
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/include/asm/setup.h | 2 +-
xen/arch/x86/setup.c | 2 +-
xen/arch/x86/tsx.c | 16 +++--------
9 files changed, 81 insertions(+), 23 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1
2023-07-24 16:52 [PATCH v6 0/3] Prevent attempting updates known to fail Alejandro Vallejo
@ 2023-07-24 16:52 ` Alejandro Vallejo
2023-07-25 6:40 ` Jan Beulich
2023-07-24 16:52 ` [PATCH v6 2/3] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init() Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 3/3] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set Alejandro Vallejo
2 siblings, 1 reply; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-24 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
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>
---
v6:
* Change message content and move severity WARNING -> INFO (Andrew)
NOTE: It has a convoluted argument to simplify the last patch, that
uses the same printk()
---
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 bec8b55db2..71e3944cf2 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s",
+ "HW toggle");
+ 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 2/3] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init()
2023-07-24 16:52 [PATCH v6 0/3] Prevent attempting updates known to fail Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1 Alejandro Vallejo
@ 2023-07-24 16:52 ` Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 3/3] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set Alejandro Vallejo
2 siblings, 0 replies; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-24 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v6:
* Invert logic of a verbosity-guard to reduce diff (Jan)
* Reflow last printk in v5/patch3:early_cpu_init() for line length (Jan)
* Rewrite comment for the early_cpu_init(false) statement (Jan)
---
xen/arch/x86/cpu/common.c | 20 +++++++++++++++-----
xen/arch/x86/cpu/microcode/core.c | 9 +++++++++
xen/arch/x86/include/asm/setup.h | 2 +-
xen/arch/x86/setup.c | 2 +-
xen/arch/x86/tsx.c | 16 ++++------------
5 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace12..2b9cc680ac 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,6 +324,8 @@ 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:
+ if (!verbose)
+ break;
printk(XENLOG_ERR
"Unrecognised or unsupported CPU vendor '%.12s'\n",
c->x86_vendor_id);
@@ -340,10 +342,13 @@ 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(c->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 +357,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 71e3944cf2..44bc0fafa3 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -887,5 +887,14 @@ int __init early_microcode_init(unsigned long *module_map,
if ( ucode_mod.mod_end || ucode_blob.size )
rc = early_microcode_update_cpu();
+ /*
+ * Some CPUID leaves and MSRs are only present after microcode updates
+ * on some processors. We take the chance here to make sure what little
+ * state we have already probed is re-probed in order to ensure we do
+ * not use stale values. tsx_init() in particular needs to have up to
+ * date MSR_ARCH_CAPS.
+ */
+ early_cpu_init(false);
+
return rc;
}
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index ae0dd3915a..df59a4cd28 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -15,7 +15,7 @@ extern uint64_t boot_tsc_stamp;
extern void *stack_start;
-void early_cpu_init(void);
+void early_cpu_init(bool verbose);
void early_time_init(void);
void set_nr_cpu_ids(unsigned int max_cpus);
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 3/3] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set
2023-07-24 16:52 [PATCH v6 0/3] Prevent attempting updates known to fail Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1 Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 2/3] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init() Alejandro Vallejo
@ 2023-07-24 16:52 ` Alejandro Vallejo
2 siblings, 0 replies; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-24 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
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 Intel64 CPU.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v6:
* Amended early exit comment to cover both !can_load and rev=~0 (Jan)
---
xen/arch/x86/cpu/microcode/core.c | 20 ++++++++++++++------
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, 40 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 44bc0fafa3..ec921f5b77 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;
}
@@ -871,13 +875,17 @@ int __init early_microcode_init(unsigned long *module_map,
/*
* 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.
+ * mean that they will not accept microcode updates.
+ *
+ * It's also possible the hardware might have built-in support to disable
+ * updates and someone (e.g: a baremetal cloud provider) disabled them.
+ *
+ * Take the hint in either case and ignore the microcode interface.
*/
- if ( this_cpu(cpu_sig).rev == ~0 )
+ if ( this_cpu(cpu_sig).rev == ~0 || !can_load )
{
printk(XENLOG_INFO "Microcode loading disabled due to: %s",
- "HW toggle");
+ can_load ? "rev = ~0" : "HW toggle");
ucode_ops.apply_microcode = NULL;
return -ENODEV;
}
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 8d4d6574aa..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 64e1dad225..5422baed27 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -193,6 +193,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 2382fc8e11..a5f730e9a6 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1
2023-07-24 16:52 ` [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1 Alejandro Vallejo
@ 2023-07-25 6:40 ` Jan Beulich
2023-07-25 6:43 ` Jan Beulich
2023-07-25 12:50 ` Alejandro Vallejo
0 siblings, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2023-07-25 6:40 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Xen-devel
On 24.07.2023 18:52, 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>
Hmm.
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s",
While we have tentatively agreed to adjust what _will_ be emitted by
default (subject to suitable justification in that change's
description), such a patch is yet to be sent. As it stands this message
will be invisible by default.
> + "HW toggle");
With the comment talking about hypervisors, what is this string supposed
to tell an observer of the message in a log file?
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1
2023-07-25 6:40 ` Jan Beulich
@ 2023-07-25 6:43 ` Jan Beulich
2023-07-25 12:50 ` Alejandro Vallejo
1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2023-07-25 6:43 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Xen-devel
On 25.07.2023 08:40, Jan Beulich wrote:
> On 24.07.2023 18:52, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s",
>
> While we have tentatively agreed to adjust what _will_ be emitted by
> default (subject to suitable justification in that change's
> description), such a patch is yet to be sent. As it stands this message
> will be invisible by default.
>
>> + "HW toggle");
>
> With the comment talking about hypervisors, what is this string supposed
> to tell an observer of the message in a log file?
Looking at patch 3 I get the impression that you put here the wrong of the
two messages there.
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1
2023-07-25 6:40 ` Jan Beulich
2023-07-25 6:43 ` Jan Beulich
@ 2023-07-25 12:50 ` Alejandro Vallejo
2023-07-28 11:45 ` Alejandro Vallejo
1 sibling, 1 reply; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-25 12:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Xen-devel
On Tue, Jul 25, 2023 at 08:40:31AM +0200, Jan Beulich wrote:
> On 24.07.2023 18:52, 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>
>
> Hmm.
>
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s",
>
> While we have tentatively agreed to adjust what _will_ be emitted by
> default (subject to suitable justification in that change's
> description), such a patch is yet to be sent.
Ugh, that was indeed mistagged. Sorry about that. I touched several parts
of this patch shortly before sending it and it crept in by mistake.
> As it stands this message
> will be invisible by default.
Arguably, that's not necessarily a bad thing. The fact that microcode
cannot be updated is expected behaviour and it makes little sense to warn
about it. If only because they should already be aware of it through their
agreement with their provider.
The case I can think of where a warning would be sensible is where the
system has a microcode blob more recent than the currently installed
revision. This is something the admin may want to be aware of in order to
pester their provider for updates. In the common case the machine won't
even need such an update, so sending unconditional warnings per boot seems
unwarranted.
>
> > + "HW toggle");
>
> With the comment talking about hypervisors, what is this string supposed
> to tell an observer of the message in a log file?
>
> Jan
Nothing good. As you noticed later, that's the wrong substring off the last
patch and should've been "rev = ~0"
Thanks,
Alejandro
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1
2023-07-25 12:50 ` Alejandro Vallejo
@ 2023-07-28 11:45 ` Alejandro Vallejo
0 siblings, 0 replies; 8+ messages in thread
From: Alejandro Vallejo @ 2023-07-28 11:45 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Xen-devel
On Tue, Jul 25, 2023 at 01:50:52PM +0100, Alejandro Vallejo wrote:
> On Tue, Jul 25, 2023 at 08:40:31AM +0200, Jan Beulich wrote:
> > > --- a/xen/arch/x86/cpu/microcode/core.c
> > > +++ b/xen/arch/x86/cpu/microcode/core.c
> > > @@ -867,10 +867,23 @@ 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_INFO "Microcode loading disabled due to: %s",
> >
> [snip]
>
> > As it stands this message
> > will be invisible by default.
> Arguably, that's not necessarily a bad thing. The fact that microcode
> cannot be updated is expected behaviour and it makes little sense to warn
> about it. If only because they should already be aware of it through their
> agreement with their provider.
>
> The case I can think of where a warning would be sensible is where the
> system has a microcode blob more recent than the currently installed
> revision. This is something the admin may want to be aware of in order to
> pester their provider for updates. In the common case the machine won't
> even need such an update, so sending unconditional warnings per boot seems
> unwarranted.
Actually, the previous message probably ought to be an INFO too. It's an
unconditional warning on old AMD and anything non AMD/Intel for no good
reason.
Alejandro
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-28 11:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-24 16:52 [PATCH v6 0/3] Prevent attempting updates known to fail Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 1/3] x86/microcode: Ignore microcode loading interface for revision = -1 Alejandro Vallejo
2023-07-25 6:40 ` Jan Beulich
2023-07-25 6:43 ` Jan Beulich
2023-07-25 12:50 ` Alejandro Vallejo
2023-07-28 11:45 ` Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 2/3] x86: Read MSR_ARCH_CAPS immediately after early_microcode_init() Alejandro Vallejo
2023-07-24 16:52 ` [PATCH v6 3/3] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set Alejandro Vallejo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.