* [PATCH v2 0/2] Add Kconfig option to remove microcode loading support
@ 2026-01-12 15:02 Alejandro Vallejo
2026-01-12 15:02 ` [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ Alejandro Vallejo
2026-01-12 15:02 ` [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading Alejandro Vallejo
0 siblings, 2 replies; 13+ messages in thread
From: Alejandro Vallejo @ 2026-01-12 15:02 UTC (permalink / raw)
To: xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Doug Goldstein, Stefano Stabellini,
Anthony PERARD, Michal Orzel, Julien Grall, Daniel P. Smith,
Marek Marczykowski-Górecki
Hi,
This is v2 of the series I sent back in November, now condensed in a single
patch with heavier use of DCE, and with earlycpio removed too.
While doing this I spotted a typo, thus the new patch1. Patch 2 has all the
meat, fat and bone of the old series at a fraction of the diffstat.
v1: https://lore.kernel.org/xen-devel/20251112162219.226075-1-alejandro.garciavallejo@amd.com/
pipeline (still running at time of send):
https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/2258163170
Cheers,
Alejandro
Alejandro Vallejo (2):
x86/ucode: Fix typo s/mitigiated/mitigated/
x86/ucode: Add Kconfig option to remove 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 | 28 ++++++++++++++++------------
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, 67 insertions(+), 32 deletions(-)
base-commit: a2a34d76643e49ccc949296c9a45888034e50b55
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ 2026-01-12 15:02 [PATCH v2 0/2] Add Kconfig option to remove microcode loading support Alejandro Vallejo @ 2026-01-12 15:02 ` Alejandro Vallejo 2026-01-12 15:44 ` Andrew Cooper 2026-01-12 15:02 ` [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading Alejandro Vallejo 1 sibling, 1 reply; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-12 15:02 UTC (permalink / raw) To: xen-devel Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper, Roger Pau Monné Not a functional change. Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> --- xen/arch/x86/cpu/microcode/amd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 2760ace921..71b041c84b 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -101,7 +101,7 @@ static const struct patch_digest { } patch_digests[] = { #include "amd-patch-digests.c" }; -static bool __ro_after_init entrysign_mitigiated_in_firmware; +static bool __ro_after_init entrysign_mitigated_in_firmware; static int cf_check cmp_patch_id(const void *key, const void *elem) { @@ -127,7 +127,7 @@ static bool check_digest(const struct container_microcode *mc) * the digest of the patch against a list of known provenance. */ if ( boot_cpu_data.family < 0x17 || boot_cpu_data.family > 0x1a || - entrysign_mitigiated_in_firmware || !opt_digest_check ) + entrysign_mitigated_in_firmware || !opt_digest_check ) return true; pd = bsearch(&patch->patch_id, patch_digests, ARRAY_SIZE(patch_digests), @@ -676,7 +676,7 @@ void __init amd_check_entrysign(void) */ if ( (uint8_t)curr_rev >= fixed_rev ) { - entrysign_mitigiated_in_firmware = true; + entrysign_mitigated_in_firmware = true; return; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ 2026-01-12 15:02 ` [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ Alejandro Vallejo @ 2026-01-12 15:44 ` Andrew Cooper 0 siblings, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2026-01-12 15:44 UTC (permalink / raw) To: Alejandro Vallejo, xen-devel Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné On 12/01/2026 3:02 pm, Alejandro Vallejo wrote: > Not a functional change. > > Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com> Oops. Clearly my fault... Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 15:02 [PATCH v2 0/2] Add Kconfig option to remove microcode loading support Alejandro Vallejo 2026-01-12 15:02 ` [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ Alejandro Vallejo @ 2026-01-12 15:02 ` Alejandro Vallejo 2026-01-12 17:15 ` Andrew Cooper 1 sibling, 1 reply; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-12 15:02 UTC (permalink / raw) To: xen-devel Cc: Alejandro Vallejo, Doug Goldstein, Stefano Stabellini, Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki 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.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 15:02 ` [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading Alejandro Vallejo @ 2026-01-12 17:15 ` Andrew Cooper 2026-01-12 18:47 ` Alejandro Vallejo 2026-01-13 8:58 ` Jan Beulich 0 siblings, 2 replies; 13+ messages in thread From: Andrew Cooper @ 2026-01-12 17:15 UTC (permalink / raw) To: Alejandro Vallejo, xen-devel Cc: Andrew Cooper, Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki 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. > 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 17:15 ` Andrew Cooper @ 2026-01-12 18:47 ` Alejandro Vallejo 2026-01-12 19:12 ` Alejandro Vallejo 2026-01-12 19:47 ` Andrew Cooper 2026-01-13 8:58 ` Jan Beulich 1 sibling, 2 replies; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-12 18:47 UTC (permalink / raw) To: Andrew Cooper, xen-devel Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 18:47 ` Alejandro Vallejo @ 2026-01-12 19:12 ` Alejandro Vallejo 2026-01-12 19:51 ` Andrew Cooper 2026-01-12 19:47 ` Andrew Cooper 1 sibling, 1 reply; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-12 19:12 UTC (permalink / raw) To: Alejandro Vallejo, Andrew Cooper, xen-devel Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 19:12 ` Alejandro Vallejo @ 2026-01-12 19:51 ` Andrew Cooper 0 siblings, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2026-01-12 19:51 UTC (permalink / raw) To: Alejandro Vallejo, xen-devel Cc: Andrew Cooper, Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 18:47 ` Alejandro Vallejo 2026-01-12 19:12 ` Alejandro Vallejo @ 2026-01-12 19:47 ` Andrew Cooper 2026-01-13 10:43 ` Alejandro Vallejo 1 sibling, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2026-01-12 19:47 UTC (permalink / raw) To: Alejandro Vallejo, xen-devel Cc: Andrew Cooper, Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 19:47 ` Andrew Cooper @ 2026-01-13 10:43 ` Alejandro Vallejo 0 siblings, 0 replies; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-13 10:43 UTC (permalink / raw) To: Andrew Cooper, xen-devel Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-12 17:15 ` Andrew Cooper 2026-01-12 18:47 ` Alejandro Vallejo @ 2026-01-13 8:58 ` Jan Beulich 2026-01-13 10:45 ` Alejandro Vallejo 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2026-01-13 8:58 UTC (permalink / raw) To: Andrew Cooper Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, Alejandro Vallejo, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-13 8:58 ` Jan Beulich @ 2026-01-13 10:45 ` Alejandro Vallejo 2026-01-13 10:49 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Alejandro Vallejo @ 2026-01-13 10:45 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading 2026-01-13 10:45 ` Alejandro Vallejo @ 2026-01-13 10:49 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2026-01-13 10:49 UTC (permalink / raw) To: Alejandro Vallejo, Andrew Cooper Cc: Doug Goldstein, Stefano Stabellini, Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné, Daniel P. Smith, Marek Marczykowski-Górecki, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-01-13 10:50 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-12 15:02 [PATCH v2 0/2] Add Kconfig option to remove microcode loading support Alejandro Vallejo 2026-01-12 15:02 ` [PATCH v2 1/2] x86/ucode: Fix typo s/mitigiated/mitigated/ Alejandro Vallejo 2026-01-12 15:44 ` Andrew Cooper 2026-01-12 15:02 ` [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading Alejandro Vallejo 2026-01-12 17:15 ` Andrew Cooper 2026-01-12 18:47 ` Alejandro Vallejo 2026-01-12 19:12 ` Alejandro Vallejo 2026-01-12 19:51 ` Andrew Cooper 2026-01-12 19:47 ` Andrew Cooper 2026-01-13 10:43 ` Alejandro Vallejo 2026-01-13 8:58 ` Jan Beulich 2026-01-13 10:45 ` Alejandro Vallejo 2026-01-13 10:49 ` Jan Beulich
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.