|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |