Amend docs to reflect ucode= commands depend on MICROCODE_LOADING
being set.
While at it, rename UCODE_SCAN_DEFAULT to MICROCODE_SCAN_DEFAULT for
consistency.
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
v2:
* Rely aggressively on DCE and ifdef/IS_ENABLED rather than file
refactoring
* Add a note about CONFIG_MICROCODE_LOADING to xen-command-line.pandoc
* Rename UCODE to MICROCODE_LOADING
* Rename UCODE_SCAN_DEFAULT to MICROCODE_SCAN_DEFAULT in order to avoid
having some options with UCODE and some with MICROCODE.
* Remove earlycpio.init.o from the build when !CONFIG_MICROCODE_LOADING
---
automation/gitlab-ci/build.yaml | 2 +-
docs/misc/efi.pandoc | 2 ++
docs/misc/xen-command-line.pandoc | 4 ++--
xen/arch/x86/Kconfig | 14 +++++++++++++-
xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++++++---------
xen/arch/x86/cpu/microcode/core.c | 25 ++++++++++++++++++-------
xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
xen/arch/x86/efi/efi-boot.h | 3 ++-
xen/arch/x86/platform_hypercall.c | 2 ++
xen/common/Makefile | 3 ++-
10 files changed, 64 insertions(+), 29 deletions(-)
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index a6fc55c2d5..b69bad9202 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -310,7 +310,7 @@ alpine-3.18-gcc-debug:
CONFIG_ARGO=y
CONFIG_UBSAN=y
CONFIG_UBSAN_FATAL=y
- CONFIG_UCODE_SCAN_DEFAULT=y
+ CONFIG_MICROCODE_SCAN_DEFAULT=y
CONFIG_XHCI=y
debian-13-x86_64-gcc-debug:
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 11c1ac3346..c3fb1f3a30 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -104,6 +104,8 @@ Specifies an XSM module to load.
Specifies a CPU microcode blob to load. (x86 only)
+Only available when Xen is compiled with CONFIG_MICROCODE_LOADING.
+
###`dtb=<filename>`
Specifies a device tree file to load. The platform firmware may provide a
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 50d7edb248..866fa2f951 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2748,7 +2748,7 @@ performance.
### ucode
> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
- Applicability: x86
+ Applicability: x86 with CONFIG_MICROCODE_LOADING active
Default: `scan` is selectable via Kconfig, `nmi,digest-check`
Controls for CPU microcode loading. For early loading, this parameter can
@@ -2773,7 +2773,7 @@ microcode in the cpio name space must be:
- on AMD : kernel/x86/microcode/AuthenticAMD.bin
When using xen.efi, the `ucode=<filename>` config file setting takes
precedence over `scan`. The default value for `scan` is set with
-`CONFIG_UCODE_SCAN_DEFAULT`.
+`CONFIG_MICROCODE_SCAN_DEFAULT`.
'nmi' determines late loading is performed in NMI handler or just in
stop_machine context. In NMI handler, even NMIs are blocked, which is
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index c808c989fc..1353ec9a3f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -331,8 +331,20 @@ config REQUIRE_NX
was unavailable. However, if enabled, Xen will no longer boot on
any CPU which is lacking NX support.
-config UCODE_SCAN_DEFAULT
+config MICROCODE_LOADING
+ bool "Microcode loading"
+ default y
+ help
+ Support updating the microcode revision of available CPUs with a newer
+ vendor-provided microcode blob. Microcode updates address some classes of
+ silicon defects. It's a very common delivery mechanism for fixes or
+ workarounds for speculative execution vulnerabilities.
+
+ If unsure, say Y.
+
+config MICROCODE_SCAN_DEFAULT
bool "Scan for microcode by default"
+ depends on MICROCODE_LOADING
help
During boot, Xen can scan the multiboot images for a CPIO archive
containing CPU microcode to be loaded, which is Linux's mechanism for
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 71b041c84b..6abdc7813b 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -264,7 +264,7 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
return equiv.id == patch->processor_rev_id;
}
-static int cf_check amd_compare(
+static int cf_check __maybe_unused amd_compare(
const struct microcode_patch *old, const struct microcode_patch *new)
{
/* Both patches to compare are supposed to be applicable to local CPU. */
@@ -310,8 +310,8 @@ static bool check_min_rev(const struct microcode_patch *patch)
return this_cpu(cpu_sig).rev >= patch->min_rev;
}
-static int cf_check apply_microcode(const struct microcode_patch *patch,
- unsigned int flags)
+static int cf_check __maybe_unused apply_microcode(
+ const struct microcode_patch *patch, unsigned int flags)
{
int hw_err, result;
unsigned int cpu = smp_processor_id();
@@ -422,7 +422,7 @@ static int scan_equiv_cpu_table(const struct container_equiv_table *et)
return -ESRCH;
}
-static struct microcode_patch *cf_check cpu_request_microcode(
+static __maybe_unused struct microcode_patch *cf_check cpu_request_microcode(
const void *buf, size_t size, bool make_copy)
{
const struct microcode_patch *saved = NULL;
@@ -557,15 +557,17 @@ static struct microcode_patch *cf_check cpu_request_microcode(
return patch;
}
-static const char __initconst amd_cpio_path[] =
+static const char __initconst __maybe_unused amd_cpio_path[] =
"kernel/x86/microcode/AuthenticAMD.bin";
static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
- .cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
+#ifdef CONFIG_MICROCODE_LOADING
+ .cpu_request_microcode = cpu_request_microcode,
.apply_microcode = apply_microcode,
.compare = amd_compare,
.cpio_path = amd_cpio_path,
+#endif /* CONFIG_MICROCODE_LOADING */
};
void __init ucode_probe_amd(struct microcode_ops *ops)
@@ -574,7 +576,8 @@ void __init ucode_probe_amd(struct microcode_ops *ops)
* The Entrysign vulnerability (SB-7033, CVE-2024-36347) affects Zen1-5
* CPUs. Taint Xen if digest checking is turned off.
*/
- if ( boot_cpu_data.family >= 0x17 && boot_cpu_data.family <= 0x1a &&
+ if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) &&
+ boot_cpu_data.family >= 0x17 && boot_cpu_data.family <= 0x1a &&
!opt_digest_check )
{
printk(XENLOG_WARNING
@@ -614,8 +617,9 @@ void __init amd_check_entrysign(void)
unsigned int curr_rev;
uint8_t fixed_rev;
- if ( boot_cpu_data.vendor != X86_VENDOR_AMD ||
- boot_cpu_data.family < 0x17 ||
+ if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) ||
+ boot_cpu_data.vendor != X86_VENDOR_AMD ||
+ boot_cpu_data.family < 0x17 ||
boot_cpu_data.family > 0x1a )
return;
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index fe47c3a6c1..aec99834fd 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -44,6 +44,9 @@
#include "private.h"
+#define can_apply_microcode(ops) (IS_ENABLED(CONFIG_MICROCODE_LOADING) && \
+ (ops).apply_microcode)
+
/*
* Before performing a late microcode update on any thread, we
* rendezvous all cpus in stop_machine context. The timeout for
@@ -58,7 +61,7 @@
*/
#define MICROCODE_UPDATE_TIMEOUT_US 1000000
-static bool __initdata ucode_mod_forced;
+static bool __initdata __maybe_unused ucode_mod_forced;
static unsigned int nr_cores;
/*
@@ -101,9 +104,10 @@ static struct microcode_patch *microcode_cache;
* location we require that they are not both active together.
*/
static int __initdata opt_mod_idx;
-static bool __initdata opt_scan = IS_ENABLED(CONFIG_UCODE_SCAN_DEFAULT);
+static bool __initdata opt_scan = IS_ENABLED(CONFIG_MICROCODE_SCAN_DEFAULT);
bool __ro_after_init opt_digest_check = true;
+#ifdef CONFIG_MICROCODE_LOADING
/*
* Used by the EFI path only, when xen.cfg identifies an explicit microcode
* file. Overrides ucode=<int>|scan on the regular command line.
@@ -162,6 +166,7 @@ static int __init cf_check parse_ucode(const char *s)
return rc;
}
custom_param("ucode", parse_ucode);
+#endif /* CONFIG_MICROCODE_LOADING */
static struct microcode_ops __ro_after_init ucode_ops;
@@ -469,7 +474,7 @@ struct ucode_buf {
char buffer[];
};
-static long cf_check ucode_update_hcall_cont(void *data)
+static long cf_check __maybe_unused ucode_update_hcall_cont(void *data)
{
struct microcode_patch *patch = NULL;
int ret, result;
@@ -613,6 +618,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
return ret;
}
+#ifdef CONFIG_MICROCODE_LOADING
int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
unsigned long len, unsigned int flags)
{
@@ -622,7 +628,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
if ( flags & ~XENPF_UCODE_FORCE )
return -EINVAL;
- if ( !ucode_ops.apply_microcode )
+ if ( !can_apply_microcode(ucode_ops) )
return -EINVAL;
buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
@@ -645,6 +651,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
*/
return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
}
+#endif /* CONFIG_MICROCODE_LOADING */
/* Load a cached update to current cpu */
int microcode_update_one(void)
@@ -658,7 +665,7 @@ int microcode_update_one(void)
if ( ucode_ops.collect_cpu_info )
alternative_vcall(ucode_ops.collect_cpu_info);
- if ( !ucode_ops.apply_microcode )
+ if ( !can_apply_microcode(ucode_ops) )
return -EOPNOTSUPP;
spin_lock(µcode_mutex);
@@ -678,6 +685,7 @@ int microcode_update_one(void)
*/
static int __initdata early_mod_idx = -1;
+#ifdef CONFIG_MICROCODE_LOADING
static int __init cf_check microcode_init_cache(void)
{
struct boot_info *bi = &xen_boot_info;
@@ -734,6 +742,7 @@ static int __init cf_check microcode_init_cache(void)
return rc;
}
presmp_initcall(microcode_init_cache);
+#endif /* CONFIG_MICROCODE_LOADING */
/*
* There are several tasks:
@@ -907,10 +916,12 @@ int __init early_microcode_init(struct boot_info *bi)
*
* Take the hint in either case and ignore the microcode interface.
*/
- if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
+ if ( !can_apply_microcode(ucode_ops) || this_cpu(cpu_sig).rev == ~0 )
{
printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
- ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
+ !IS_ENABLED(CONFIG_MICROCODE_LOADING) ? "not compiled in" :
+ ucode_ops.apply_microcode ? "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 281993e725..d9895018b4 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -273,7 +273,7 @@ static bool microcode_fits_cpu(const struct microcode_patch *mc)
return false;
}
-static int cf_check intel_compare(
+static int __maybe_unused cf_check intel_compare(
const struct microcode_patch *old, const struct microcode_patch *new)
{
/*
@@ -286,8 +286,8 @@ static int cf_check intel_compare(
return compare_revisions(old->rev, new->rev);
}
-static int cf_check apply_microcode(const struct microcode_patch *patch,
- unsigned int flags)
+static int cf_check __maybe_unused apply_microcode(
+ const struct microcode_patch *patch, unsigned int flags)
{
uint64_t msr_content;
unsigned int cpu = smp_processor_id();
@@ -333,7 +333,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch,
return 0;
}
-static struct microcode_patch *cf_check cpu_request_microcode(
+static __maybe_unused struct microcode_patch *cf_check cpu_request_microcode(
const void *buf, size_t size, bool make_copy)
{
int error = 0;
@@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
}
-static const char __initconst intel_cpio_path[] =
+static const char __initconst __maybe_unused intel_cpio_path[] =
"kernel/x86/microcode/GenuineIntel.bin";
static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
- .cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
+#ifdef CONFIG_MICROCODE_LOADING
+ .cpu_request_microcode = cpu_request_microcode,
.apply_microcode = apply_microcode,
.compare = intel_compare,
.cpio_path = intel_cpio_path,
+#endif /* CONFIG_MICROCODE_LOADING */
};
void __init ucode_probe_intel(struct microcode_ops *ops)
{
*ops = intel_ucode_ops;
- if ( !can_load_microcode() )
+ if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
ops->apply_microcode = NULL;
}
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 0194720003..42a2c46b5e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -295,7 +295,8 @@ static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
{
union string name;
- if ( read_section(image, L"ucode", &ucode, NULL) )
+ if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) ||
+ read_section(image, L"ucode", &ucode, NULL) )
return;
name.s = get_value(&cfg, section, "ucode");
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 79bb99e0b6..5e83965d21 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -307,6 +307,7 @@ ret_t do_platform_op(
break;
}
+#ifdef CONFIG_MICROCODE_LOADING
case XENPF_microcode_update:
{
XEN_GUEST_HANDLE(const_void) data;
@@ -327,6 +328,7 @@ ret_t do_platform_op(
op->u.microcode2.flags);
break;
}
+#endif /* CONFIG_MICROCODE_LOADING */
case XENPF_platform_quirk:
{
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 92c97d641e..1e6c92e554 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -65,7 +65,8 @@ obj-y += wait.o
obj-bin-y += warning.init.o
obj-y += xmalloc_tlsf.o
-obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
+obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
+obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd,$(n).init.o)
obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
--
2.43.0On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
> automation/gitlab-ci/build.yaml | 2 +-
> docs/misc/efi.pandoc | 2 ++
> docs/misc/xen-command-line.pandoc | 4 ++--
> xen/arch/x86/Kconfig | 14 +++++++++++++-
> xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++++++---------
> xen/arch/x86/cpu/microcode/core.c | 25 ++++++++++++++++++-------
> xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
> xen/arch/x86/efi/efi-boot.h | 3 ++-
> xen/arch/x86/platform_hypercall.c | 2 ++
> xen/common/Makefile | 3 ++-
> 10 files changed, 64 insertions(+), 29 deletions(-)
Much nicer in terms of (non) invasiveness.
First, please split the rename of CONFIG_UCODE_SCAN_DEFAULT into an
earlier patch.
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 50d7edb248..866fa2f951 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2748,7 +2748,7 @@ performance.
> ### ucode
> > `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>
> - Applicability: x86
> + Applicability: x86 with CONFIG_MICROCODE_LOADING active
> Default: `scan` is selectable via Kconfig, `nmi,digest-check`
You want to include this too:
diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst
index a07e25802fab..91668e6f9b3f 100644
--- a/docs/admin-guide/microcode-loading.rst
+++ b/docs/admin-guide/microcode-loading.rst
@@ -23,6 +23,9 @@ TSX errata which necessitated disabling the feature entirely), or the addition
of brand new features (e.g. the Spectre v2 controls to work around speculative
execution vulnerabilities).
+Microcode loading support in Xen is controlled by the
+``CONFIG_MICROCODE_LOADING`` Kconfig option.
+
Boot time microcode loading
---------------------------
while changing the docs.
>
> Controls for CPU microcode loading. For early loading, this parameter can
> @@ -2773,7 +2773,7 @@ microcode in the cpio name space must be:
> - on AMD : kernel/x86/microcode/AuthenticAMD.bin
> When using xen.efi, the `ucode=<filename>` config file setting takes
> precedence over `scan`. The default value for `scan` is set with
> -`CONFIG_UCODE_SCAN_DEFAULT`.
> +`CONFIG_MICROCODE_SCAN_DEFAULT`.
>
> 'nmi' determines late loading is performed in NMI handler or just in
> stop_machine context. In NMI handler, even NMIs are blocked, which is
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index c808c989fc..1353ec9a3f 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -331,8 +331,20 @@ config REQUIRE_NX
> was unavailable. However, if enabled, Xen will no longer boot on
> any CPU which is lacking NX support.
>
> -config UCODE_SCAN_DEFAULT
> +config MICROCODE_LOADING
> + bool "Microcode loading"
> + default y
> + help
> + Support updating the microcode revision of available CPUs with a newer
> + vendor-provided microcode blob. Microcode updates address some classes of
> + silicon defects. It's a very common delivery mechanism for fixes or
> + workarounds for speculative execution vulnerabilities.
> +
> + If unsure, say Y.
Please don't re-iterate the default. It's a waste. As to the main
text, that's not great for the target audience of a distro packager /
power user. How about:
--8<--
Microcode updates for CPUs fix errata and provide new functionality for
software to work around bugs, such as the speculative execution
vulnerabilities. It is common for OSes to carry updated microcode as
software tends to get updated more frequently than firmware.
For embedded environments where a full firmware/software stack is being
provided, it might not be necessary for Xen to need to load microcode
itself.
--8<--
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index fe47c3a6c1..aec99834fd 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -44,6 +44,9 @@
>
> #include "private.h"
>
> +#define can_apply_microcode(ops) (IS_ENABLED(CONFIG_MICROCODE_LOADING) && \
> + (ops).apply_microcode)
I don't think this is a useful macro. It's used 3 times, despite ...
> @@ -613,6 +618,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
> return ret;
> }
>
> +#ifdef CONFIG_MICROCODE_LOADING
> int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
> unsigned long len, unsigned int flags)
> {
> @@ -622,7 +628,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
> if ( flags & ~XENPF_UCODE_FORCE )
> return -EINVAL;
>
> - if ( !ucode_ops.apply_microcode )
> + if ( !can_apply_microcode(ucode_ops) )
> return -EINVAL;
>
> buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
> @@ -645,6 +651,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
> */
> return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
> }
> +#endif /* CONFIG_MICROCODE_LOADING */
... this use being redundant. Just expand it for the two other cases
and consistently use IS_ENABLED() in view.
> @@ -907,10 +916,12 @@ int __init early_microcode_init(struct boot_info *bi)
> *
> * Take the hint in either case and ignore the microcode interface.
> */
> - if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
> + if ( !can_apply_microcode(ucode_ops) || this_cpu(cpu_sig).rev == ~0 )
> {
> printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
> - ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
> + !IS_ENABLED(CONFIG_MICROCODE_LOADING) ? "not compiled in" :
> + ucode_ops.apply_microcode ? "rev = ~0" :
> + "HW toggle");
> ucode_ops.apply_microcode = NULL;
> return -ENODEV;
> }
Don't complicate this messy printk() further. Just exit early; it's not
interesting to read "not loading microcode because it's not compiled in".
This is a rare example of a subsystem where it remains partially
compiled in, and it's even possible to write such a printk().
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index 281993e725..d9895018b4 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
> }
>
> -static const char __initconst intel_cpio_path[] =
> +static const char __initconst __maybe_unused intel_cpio_path[] =
> "kernel/x86/microcode/GenuineIntel.bin";
>
> static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
> - .cpu_request_microcode = cpu_request_microcode,
> .collect_cpu_info = collect_cpu_info,
> +#ifdef CONFIG_MICROCODE_LOADING
> + .cpu_request_microcode = cpu_request_microcode,
> .apply_microcode = apply_microcode,
> .compare = intel_compare,
> .cpio_path = intel_cpio_path,
> +#endif /* CONFIG_MICROCODE_LOADING */
> };
>
> void __init ucode_probe_intel(struct microcode_ops *ops)
> {
> *ops = intel_ucode_ops;
>
> - if ( !can_load_microcode() )
> + if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
> ops->apply_microcode = NULL;
> }
! ||, surely?
> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> index 79bb99e0b6..5e83965d21 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -307,6 +307,7 @@ ret_t do_platform_op(
> break;
> }
>
> +#ifdef CONFIG_MICROCODE_LOADING
> case XENPF_microcode_update:
> {
> XEN_GUEST_HANDLE(const_void) data;
> @@ -327,6 +328,7 @@ ret_t do_platform_op(
> op->u.microcode2.flags);
> break;
> }
> +#endif /* CONFIG_MICROCODE_LOADING */
You mustn't #ifdef out a case like this, because it causes the op to
fall into the default case, and some of the default chains go a long way
and make unwise assumptions, like hitting a BUG().
Always use this form:
if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) )
// EOPNOTSUPP
and leave the case intact.
>
> case XENPF_platform_quirk:
> {
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 92c97d641e..1e6c92e554 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -65,7 +65,8 @@ obj-y += wait.o
> obj-bin-y += warning.init.o
> obj-y += xmalloc_tlsf.o
>
> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
> +obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd,$(n).init.o)
>
> obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
>
In a prereq patch, please move earlycpio out of common/ into xen/lib/.
It shouldn't be tied to CONFIG_MICROCODE_LOADING like this, and it can
simply be discarded at link time when it's librified and unreferenced.
~Andrew
On 12.01.2026 18:15, Andrew Cooper wrote: > On 12/01/2026 3:02 pm, Alejandro Vallejo wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -331,8 +331,20 @@ config REQUIRE_NX >> was unavailable. However, if enabled, Xen will no longer boot on >> any CPU which is lacking NX support. >> >> -config UCODE_SCAN_DEFAULT >> +config MICROCODE_LOADING >> + bool "Microcode loading" >> + default y >> + help >> + Support updating the microcode revision of available CPUs with a newer >> + vendor-provided microcode blob. Microcode updates address some classes of >> + silicon defects. It's a very common delivery mechanism for fixes or >> + workarounds for speculative execution vulnerabilities. >> + >> + If unsure, say Y. > > Please don't re-iterate the default. It's a waste. Well, first of all we should be consistent: Either we always have such a brief sentence in the help texts of boolean options, or we never have. Who knows - cleaning this up thoughout the tree may even address some anomalies (where the sentence and the default setting disagree). Jan
On Tue Jan 13, 2026 at 9:58 AM CET, Jan Beulich wrote: > On 12.01.2026 18:15, Andrew Cooper wrote: >> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote: >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -331,8 +331,20 @@ config REQUIRE_NX >>> was unavailable. However, if enabled, Xen will no longer boot on >>> any CPU which is lacking NX support. >>> >>> -config UCODE_SCAN_DEFAULT >>> +config MICROCODE_LOADING >>> + bool "Microcode loading" >>> + default y >>> + help >>> + Support updating the microcode revision of available CPUs with a newer >>> + vendor-provided microcode blob. Microcode updates address some classes of >>> + silicon defects. It's a very common delivery mechanism for fixes or >>> + workarounds for speculative execution vulnerabilities. >>> + >>> + If unsure, say Y. >> >> Please don't re-iterate the default. It's a waste. > > Well, first of all we should be consistent: Either we always have such a brief > sentence in the help texts of boolean options, or we never have. Who knows - > cleaning this up thoughout the tree may even address some anomalies (where the > sentence and the default setting disagree). > > Jan Is that a request to add missing ones while fixing existing mismatches or remove them? Not as part of this series in any case, but do you have agreement on the course of action? Cheers, Alejandro
On 13.01.2026 11:45, Alejandro Vallejo wrote: > On Tue Jan 13, 2026 at 9:58 AM CET, Jan Beulich wrote: >> On 12.01.2026 18:15, Andrew Cooper wrote: >>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote: >>>> --- a/xen/arch/x86/Kconfig >>>> +++ b/xen/arch/x86/Kconfig >>>> @@ -331,8 +331,20 @@ config REQUIRE_NX >>>> was unavailable. However, if enabled, Xen will no longer boot on >>>> any CPU which is lacking NX support. >>>> >>>> -config UCODE_SCAN_DEFAULT >>>> +config MICROCODE_LOADING >>>> + bool "Microcode loading" >>>> + default y >>>> + help >>>> + Support updating the microcode revision of available CPUs with a newer >>>> + vendor-provided microcode blob. Microcode updates address some classes of >>>> + silicon defects. It's a very common delivery mechanism for fixes or >>>> + workarounds for speculative execution vulnerabilities. >>>> + >>>> + If unsure, say Y. >>> >>> Please don't re-iterate the default. It's a waste. >> >> Well, first of all we should be consistent: Either we always have such a brief >> sentence in the help texts of boolean options, or we never have. Who knows - >> cleaning this up thoughout the tree may even address some anomalies (where the >> sentence and the default setting disagree). > > Is that a request to add missing ones while fixing existing mismatches or remove > them? Not as part of this series in any case, but do you have agreement on the > course of action? While I agree with Andrew that these statements are redundant, I wouldn't call this "agreement" across all maintainers, at least not until a little more time has passed. Jan
On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>> automation/gitlab-ci/build.yaml | 2 +-
>> docs/misc/efi.pandoc | 2 ++
>> docs/misc/xen-command-line.pandoc | 4 ++--
>> xen/arch/x86/Kconfig | 14 +++++++++++++-
>> xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++++++---------
>> xen/arch/x86/cpu/microcode/core.c | 25 ++++++++++++++++++-------
>> xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
>> xen/arch/x86/efi/efi-boot.h | 3 ++-
>> xen/arch/x86/platform_hypercall.c | 2 ++
>> xen/common/Makefile | 3 ++-
>> 10 files changed, 64 insertions(+), 29 deletions(-)
>
> Much nicer in terms of (non) invasiveness.
>
> First, please split the rename of CONFIG_UCODE_SCAN_DEFAULT into an
> earlier patch.
Sure
>
>> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
>> index 50d7edb248..866fa2f951 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2748,7 +2748,7 @@ performance.
>> ### ucode
>> > `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>>
>> - Applicability: x86
>> + Applicability: x86 with CONFIG_MICROCODE_LOADING active
>> Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>
> You want to include this too:
>
> diff --git a/docs/admin-guide/microcode-loading.rst b/docs/admin-guide/microcode-loading.rst
> index a07e25802fab..91668e6f9b3f 100644
> --- a/docs/admin-guide/microcode-loading.rst
> +++ b/docs/admin-guide/microcode-loading.rst
> @@ -23,6 +23,9 @@ TSX errata which necessitated disabling the feature entirely), or the addition
> of brand new features (e.g. the Spectre v2 controls to work around speculative
> execution vulnerabilities).
>
> +Microcode loading support in Xen is controlled by the
> +``CONFIG_MICROCODE_LOADING`` Kconfig option.
Ack
> +
>
> Boot time microcode loading
> ---------------------------
>
>
> while changing the docs.
>
>>
>> Controls for CPU microcode loading. For early loading, this parameter can
>> @@ -2773,7 +2773,7 @@ microcode in the cpio name space must be:
>> - on AMD : kernel/x86/microcode/AuthenticAMD.bin
>> When using xen.efi, the `ucode=<filename>` config file setting takes
>> precedence over `scan`. The default value for `scan` is set with
>> -`CONFIG_UCODE_SCAN_DEFAULT`.
>> +`CONFIG_MICROCODE_SCAN_DEFAULT`.
>>
>> 'nmi' determines late loading is performed in NMI handler or just in
>> stop_machine context. In NMI handler, even NMIs are blocked, which is
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index c808c989fc..1353ec9a3f 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -331,8 +331,20 @@ config REQUIRE_NX
>> was unavailable. However, if enabled, Xen will no longer boot on
>> any CPU which is lacking NX support.
>>
>> -config UCODE_SCAN_DEFAULT
>> +config MICROCODE_LOADING
>> + bool "Microcode loading"
>> + default y
>> + help
>> + Support updating the microcode revision of available CPUs with a newer
>> + vendor-provided microcode blob. Microcode updates address some classes of
>> + silicon defects. It's a very common delivery mechanism for fixes or
>> + workarounds for speculative execution vulnerabilities.
>> +
>> + If unsure, say Y.
>
> Please don't re-iterate the default. It's a waste. As to the main
> text, that's not great for the target audience of a distro packager /
> power user. How about:
>
> --8<--
> Microcode updates for CPUs fix errata and provide new functionality for
> software to work around bugs, such as the speculative execution
> vulnerabilities. It is common for OSes to carry updated microcode as
> software tends to get updated more frequently than firmware.
>
> For embedded environments where a full firmware/software stack is being
> provided, it might not be necessary for Xen to need to load microcode
> itself.
> --8<--
Sure. I don't mind.
>
>> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
>> index fe47c3a6c1..aec99834fd 100644
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -44,6 +44,9 @@
>>
>> #include "private.h"
>>
>> +#define can_apply_microcode(ops) (IS_ENABLED(CONFIG_MICROCODE_LOADING) && \
>> + (ops).apply_microcode)
>
> I don't think this is a useful macro. It's used 3 times, despite ...
>
>> @@ -613,6 +618,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_MICROCODE_LOADING
>> int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>> unsigned long len, unsigned int flags)
>> {
>> @@ -622,7 +628,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>> if ( flags & ~XENPF_UCODE_FORCE )
>> return -EINVAL;
>>
>> - if ( !ucode_ops.apply_microcode )
>> + if ( !can_apply_microcode(ucode_ops) )
>> return -EINVAL;
>>
>> buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
>> @@ -645,6 +651,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>> */
>> return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
>> }
>> +#endif /* CONFIG_MICROCODE_LOADING */
>
> ... this use being redundant. Just expand it for the two other cases
> and consistently use IS_ENABLED() in view.
Ack.
>
>> @@ -907,10 +916,12 @@ int __init early_microcode_init(struct boot_info *bi)
>> *
>> * Take the hint in either case and ignore the microcode interface.
>> */
>> - if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
>> + if ( !can_apply_microcode(ucode_ops) || this_cpu(cpu_sig).rev == ~0 )
>> {
>> printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
>> - ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
>> + !IS_ENABLED(CONFIG_MICROCODE_LOADING) ? "not compiled in" :
>> + ucode_ops.apply_microcode ? "rev = ~0" :
>> + "HW toggle");
>> ucode_ops.apply_microcode = NULL;
>> return -ENODEV;
>> }
>
> Don't complicate this messy printk() further. Just exit early; it's not
> interesting to read "not loading microcode because it's not compiled in".
>
> This is a rare example of a subsystem where it remains partially
> compiled in, and it's even possible to write such a printk().
Ack.
>
>> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
>> index 281993e725..d9895018b4 100644
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
>> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>> }
>>
>> -static const char __initconst intel_cpio_path[] =
>> +static const char __initconst __maybe_unused intel_cpio_path[] =
>> "kernel/x86/microcode/GenuineIntel.bin";
>>
>> static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>> - .cpu_request_microcode = cpu_request_microcode,
>> .collect_cpu_info = collect_cpu_info,
>> +#ifdef CONFIG_MICROCODE_LOADING
>> + .cpu_request_microcode = cpu_request_microcode,
>> .apply_microcode = apply_microcode,
>> .compare = intel_compare,
>> .cpio_path = intel_cpio_path,
>> +#endif /* CONFIG_MICROCODE_LOADING */
>> };
>>
>> void __init ucode_probe_intel(struct microcode_ops *ops)
>> {
>> *ops = intel_ucode_ops;
>>
>> - if ( !can_load_microcode() )
>> + if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
>> ops->apply_microcode = NULL;
>> }
>
> ! ||, surely?
When !CONFIG_MICROCODE_LOADING apply_microcode is already NULL. It's a needless
assignment. This is strictly so the compiler can avoid assigning anything.
Functionally it's irrelevant.
>
>
>> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
>> index 79bb99e0b6..5e83965d21 100644
>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -307,6 +307,7 @@ ret_t do_platform_op(
>> break;
>> }
>>
>> +#ifdef CONFIG_MICROCODE_LOADING
>> case XENPF_microcode_update:
>> {
>> XEN_GUEST_HANDLE(const_void) data;
>> @@ -327,6 +328,7 @@ ret_t do_platform_op(
>> op->u.microcode2.flags);
>> break;
>> }
>> +#endif /* CONFIG_MICROCODE_LOADING */
>
> You mustn't #ifdef out a case like this, because it causes the op to
> fall into the default case, and some of the default chains go a long way
> and make unwise assumptions, like hitting a BUG().
It's normally more convenient for us (AMD) to physically remove code where
possible for coverage reasons, but in this case it probably doesn't matter.
That said, I think we can both agree if dom0 can crash the hypervisor requesting
a non existing op the bug is probably in such a BUG() statement and not
elsewhere. Note CONFIG_VIDEO already removes an op in this way in this very
file. The default case returns with ENOSYS, with BUG() being in helpers for
other data, as far as I can see.
>
> Always use this form:
>
> if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) )
> // EOPNOTSUPP
>
> and leave the case intact.
... but sure.
>
>>
>> case XENPF_platform_quirk:
>> {
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index 92c97d641e..1e6c92e554 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -65,7 +65,8 @@ obj-y += wait.o
>> obj-bin-y += warning.init.o
>> obj-y += xmalloc_tlsf.o
>>
>> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
>> +obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
>> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd,$(n).init.o)
>>
>> obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
>>
>
> In a prereq patch, please move earlycpio out of common/ into xen/lib/.
> It shouldn't be tied to CONFIG_MICROCODE_LOADING like this, and it can
> simply be discarded at link time when it's librified and unreferenced.
>
> ~Andrew
That would preclude having it in the init section though, AIUI.
Cheers,
Alejandro
On 12/01/2026 6:47 pm, Alejandro Vallejo wrote:
> On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>>> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
>>> index 281993e725..d9895018b4 100644
>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>> @@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
>>> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>>> }
>>>
>>> -static const char __initconst intel_cpio_path[] =
>>> +static const char __initconst __maybe_unused intel_cpio_path[] =
>>> "kernel/x86/microcode/GenuineIntel.bin";
>>>
>>> static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>>> - .cpu_request_microcode = cpu_request_microcode,
>>> .collect_cpu_info = collect_cpu_info,
>>> +#ifdef CONFIG_MICROCODE_LOADING
>>> + .cpu_request_microcode = cpu_request_microcode,
>>> .apply_microcode = apply_microcode,
>>> .compare = intel_compare,
>>> .cpio_path = intel_cpio_path,
>>> +#endif /* CONFIG_MICROCODE_LOADING */
>>> };
>>>
>>> void __init ucode_probe_intel(struct microcode_ops *ops)
>>> {
>>> *ops = intel_ucode_ops;
>>>
>>> - if ( !can_load_microcode() )
>>> + if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
>>> ops->apply_microcode = NULL;
>>> }
>> ! ||, surely?
> When !CONFIG_MICROCODE_LOADING apply_microcode is already NULL. It's a needless
> assignment. This is strictly so the compiler can avoid assigning anything.
>
> Functionally it's irrelevant.
Oh, that's subtle.
As can_load_microcode() is a local static function anyway, it might be
better to have an early return false in there.
That will get the same DCE, but be easier to follow.
>
>>
>>> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
>>> index 79bb99e0b6..5e83965d21 100644
>>> --- a/xen/arch/x86/platform_hypercall.c
>>> +++ b/xen/arch/x86/platform_hypercall.c
>>> @@ -307,6 +307,7 @@ ret_t do_platform_op(
>>> break;
>>> }
>>>
>>> +#ifdef CONFIG_MICROCODE_LOADING
>>> case XENPF_microcode_update:
>>> {
>>> XEN_GUEST_HANDLE(const_void) data;
>>> @@ -327,6 +328,7 @@ ret_t do_platform_op(
>>> op->u.microcode2.flags);
>>> break;
>>> }
>>> +#endif /* CONFIG_MICROCODE_LOADING */
>> You mustn't #ifdef out a case like this, because it causes the op to
>> fall into the default case, and some of the default chains go a long way
>> and make unwise assumptions, like hitting a BUG().
> It's normally more convenient for us (AMD) to physically remove code where
> possible for coverage reasons, but in this case it probably doesn't matter.
>
> That said, I think we can both agree if dom0 can crash the hypervisor requesting
> a non existing op the bug is probably in such a BUG() statement and not
> elsewhere. Note CONFIG_VIDEO already removes an op in this way in this very
> file. The default case returns with ENOSYS, with BUG() being in helpers for
> other data, as far as I can see.
The existing bad practice are the ones I haven't had time to fix yet.
As I recall, we did have a guest reachable BUG_ON() at one point caused
by this pattern, hence the "never again" position.
>>>
>>> case XENPF_platform_quirk:
>>> {
>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>> index 92c97d641e..1e6c92e554 100644
>>> --- a/xen/common/Makefile
>>> +++ b/xen/common/Makefile
>>> @@ -65,7 +65,8 @@ obj-y += wait.o
>>> obj-bin-y += warning.init.o
>>> obj-y += xmalloc_tlsf.o
>>>
>>> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
>>> +obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
>>> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd,$(n).init.o)
>>>
>>> obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
>>>
>> In a prereq patch, please move earlycpio out of common/ into xen/lib/.
>> It shouldn't be tied to CONFIG_MICROCODE_LOADING like this, and it can
>> simply be discarded at link time when it's librified and unreferenced.
>>
>> ~Andrew
> That would preclude having it in the init section though, AIUI.
There's already lib stuff placed in init. It works fine.
(What does get complicated is conditionally-init, conditionally-not, but
that's complicated irrespective of lib/)
~Andrew
Hi,
On Mon Jan 12, 2026 at 8:47 PM CET, Andrew Cooper wrote:
> On 12/01/2026 6:47 pm, Alejandro Vallejo wrote:
>> On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
>>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>>>> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
>>>> index 281993e725..d9895018b4 100644
>>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>>> @@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
>>>> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>>>> }
>>>>
>>>> -static const char __initconst intel_cpio_path[] =
>>>> +static const char __initconst __maybe_unused intel_cpio_path[] =
>>>> "kernel/x86/microcode/GenuineIntel.bin";
>>>>
>>>> static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>>>> - .cpu_request_microcode = cpu_request_microcode,
>>>> .collect_cpu_info = collect_cpu_info,
>>>> +#ifdef CONFIG_MICROCODE_LOADING
>>>> + .cpu_request_microcode = cpu_request_microcode,
>>>> .apply_microcode = apply_microcode,
>>>> .compare = intel_compare,
>>>> .cpio_path = intel_cpio_path,
>>>> +#endif /* CONFIG_MICROCODE_LOADING */
>>>> };
>>>>
>>>> void __init ucode_probe_intel(struct microcode_ops *ops)
>>>> {
>>>> *ops = intel_ucode_ops;
>>>>
>>>> - if ( !can_load_microcode() )
>>>> + if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
>>>> ops->apply_microcode = NULL;
>>>> }
>>> ! ||, surely?
>> When !CONFIG_MICROCODE_LOADING apply_microcode is already NULL. It's a needless
>> assignment. This is strictly so the compiler can avoid assigning anything.
>>
>> Functionally it's irrelevant.
>
> Oh, that's subtle.
>
> As can_load_microcode() is a local static function anyway, it might be
> better to have an early return false in there.
>
> That will get the same DCE, but be easier to follow.
>
Sure
>
>>
>>>
>>>> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
>>>> index 79bb99e0b6..5e83965d21 100644
>>>> --- a/xen/arch/x86/platform_hypercall.c
>>>> +++ b/xen/arch/x86/platform_hypercall.c
>>>> @@ -307,6 +307,7 @@ ret_t do_platform_op(
>>>> break;
>>>> }
>>>>
>>>> +#ifdef CONFIG_MICROCODE_LOADING
>>>> case XENPF_microcode_update:
>>>> {
>>>> XEN_GUEST_HANDLE(const_void) data;
>>>> @@ -327,6 +328,7 @@ ret_t do_platform_op(
>>>> op->u.microcode2.flags);
>>>> break;
>>>> }
>>>> +#endif /* CONFIG_MICROCODE_LOADING */
>>> You mustn't #ifdef out a case like this, because it causes the op to
>>> fall into the default case, and some of the default chains go a long way
>>> and make unwise assumptions, like hitting a BUG().
>> It's normally more convenient for us (AMD) to physically remove code where
>> possible for coverage reasons, but in this case it probably doesn't matter.
>>
>> That said, I think we can both agree if dom0 can crash the hypervisor requesting
>> a non existing op the bug is probably in such a BUG() statement and not
>> elsewhere. Note CONFIG_VIDEO already removes an op in this way in this very
>> file. The default case returns with ENOSYS, with BUG() being in helpers for
>> other data, as far as I can see.
>
> The existing bad practice are the ones I haven't had time to fix yet.
>
> As I recall, we did have a guest reachable BUG_ON() at one point caused
> by this pattern, hence the "never again" position.
>
Fine.
>
>>>>
>>>> case XENPF_platform_quirk:
>>>> {
>>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>>> index 92c97d641e..1e6c92e554 100644
>>>> --- a/xen/common/Makefile
>>>> +++ b/xen/common/Makefile
>>>> @@ -65,7 +65,8 @@ obj-y += wait.o
>>>> obj-bin-y += warning.init.o
>>>> obj-y += xmalloc_tlsf.o
>>>>
>>>> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
>>>> +obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
>>>> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd,$(n).init.o)
>>>>
>>>> obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
>>>>
>>> In a prereq patch, please move earlycpio out of common/ into xen/lib/.
>>> It shouldn't be tied to CONFIG_MICROCODE_LOADING like this, and it can
>>> simply be discarded at link time when it's librified and unreferenced.
>>>
>>> ~Andrew
>> That would preclude having it in the init section though, AIUI.
>
> There's already lib stuff placed in init. It works fine.
>
> (What does get complicated is conditionally-init, conditionally-not, but
> that's complicated irrespective of lib/)
It handles __init fine, but not "lib-y += foo.init.o", AFAICS. It may be 3 lines
worth of fix, but seeing how earlycpio.c has a single function already tagged
with __init, I'll just drop the .init.o part and let the compiler place that
single function in the right section.
Cheers,
Alejandro
Hi,
On Mon Jan 12, 2026 at 7:47 PM CET, Alejandro Vallejo wrote:
> On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>>> automation/gitlab-ci/build.yaml | 2 +-
>>> docs/misc/efi.pandoc | 2 ++
>>> docs/misc/xen-command-line.pandoc | 4 ++--
>>> xen/arch/x86/Kconfig | 14 +++++++++++++-
>>> xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++++++---------
>>> xen/arch/x86/cpu/microcode/core.c | 25 ++++++++++++++++++-------
>>> xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
>>> xen/arch/x86/efi/efi-boot.h | 3 ++-
>>> xen/arch/x86/platform_hypercall.c | 2 ++
>>> xen/common/Makefile | 3 ++-
>>> 10 files changed, 64 insertions(+), 29 deletions(-)
>>
>> Much nicer in terms of (non) invasiveness.
An interesting fact came to my attention. If you set a function pointer as
IS_ENABLED(x) ? foo : NULL, rather than ifdeffing out the compiler doesn't even
need __maybe_unused to eliminate the statics.
I'm adjusting as needed and creating something so that...
custom_param_if("ucode", parse_ucode, IS_ENABLED(CONFIG_MICROCODE_LOADING));
... does the right thing. I'm sure it'll have uses outside this (minor) patch to
remove a number of cmdline handlers when the feature they control isn't active.
Cheers,
Alejandro
On 12/01/2026 7:12 pm, Alejandro Vallejo wrote:
> Hi,
>
> On Mon Jan 12, 2026 at 7:47 PM CET, Alejandro Vallejo wrote:
>> On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
>>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>>>> automation/gitlab-ci/build.yaml | 2 +-
>>>> docs/misc/efi.pandoc | 2 ++
>>>> docs/misc/xen-command-line.pandoc | 4 ++--
>>>> xen/arch/x86/Kconfig | 14 +++++++++++++-
>>>> xen/arch/x86/cpu/microcode/amd.c | 22 +++++++++++++---------
>>>> xen/arch/x86/cpu/microcode/core.c | 25 ++++++++++++++++++-------
>>>> xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
>>>> xen/arch/x86/efi/efi-boot.h | 3 ++-
>>>> xen/arch/x86/platform_hypercall.c | 2 ++
>>>> xen/common/Makefile | 3 ++-
>>>> 10 files changed, 64 insertions(+), 29 deletions(-)
>>> Much nicer in terms of (non) invasiveness.
> An interesting fact came to my attention. If you set a function pointer as
> IS_ENABLED(x) ? foo : NULL, rather than ifdeffing out the compiler doesn't even
> need __maybe_unused to eliminate the statics.
Oh, yes. I'd forgotten that trick when I suggested __maybe_unused. Sorry.
>
> I'm adjusting as needed and creating something so that...
>
> custom_param_if("ucode", parse_ucode, IS_ENABLED(CONFIG_MICROCODE_LOADING));
>
> ... does the right thing. I'm sure it'll have uses outside this (minor) patch to
> remove a number of cmdline handlers when the feature they control isn't active.
This I'm rather less sure about. The lockdown patches are also
competing for a 3rd parameter in the param() APIs.
Again, I think microcode is a weird (i.e. rare) subsystem where we're
only compiling out part of it. Personally I'd leave it as you had in
this patch. It was minimally invasive.
~Andrew
© 2016 - 2026 Red Hat, Inc.