From: Ashok Raj <ashok.raj@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
Dave Hansen <dave.hansen@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH 2/2] x86/microcode: Add a "microcode=" command line option
Date: Wed, 7 Jun 2023 18:55:39 -0700 [thread overview]
Message-ID: <ZIE1G9UBAT36kPhJ@a4bf019067fa.jf.intel.com> (raw)
In-Reply-To: <20230605141332.25948-2-bp@alien8.de>
Hi
On Mon, Jun 05, 2023 at 04:13:32PM +0200, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
>
> It will be used to control microcode loader behavior. Add the first
> chicken bit: to control whether the AMD side should load microcode late
> on all logical SMT threads.
>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
> .../admin-guide/kernel-parameters.txt | 7 +++
> arch/x86/kernel/cpu/microcode/amd.c | 5 ++-
> arch/x86/kernel/cpu/microcode/core.c | 44 +++++++++++++++++++
> arch/x86/kernel/cpu/microcode/internal.h | 16 +++++++
> 4 files changed, 71 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/kernel/cpu/microcode/internal.h
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9e5bab29685f..b88ff022402c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3228,6 +3228,13 @@
>
> mga= [HW,DRM]
>
> + microcode= [X86] Control the behavior of the microcode
> + loader. Available options:
> +
> + no_late_all - do not load on all SMT threads on
> + AMD. Loading on all logical threads is enabled by
> + default.
> +
The default behavior is that the reload happens on all threads for both
early and late.
The chicken bit in cmdline and the sysfs/control is to opt-out just in case
they want to change the default behavior?
When end user changes the behavior, isn't it against the design specification? And if
so, should that result in kernel being tainted after a reload?
Is this reload on all threads required by all models, or only certain
models? I was wondering if the forced reload could be limited to only
affected CPUs instead of doing it on all unconditionally.
> min_addr=nn[KMG] [KNL,BOOT,IA-64] All physical memory below this
> physical address is ignored.
>
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 87208e46f7ed..76b530697951 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -36,6 +36,8 @@
> #include <asm/cpu.h>
> #include <asm/msr.h>
>
> +#include "internal.h"
> +
> static struct equiv_cpu_table {
> unsigned int num_entries;
> struct equiv_cpu_entry *entry;
> @@ -700,7 +702,8 @@ static enum ucode_state apply_microcode_amd(int cpu)
> rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
>
> /* need to apply patch? */
> - if (rev > mc_amd->hdr.patch_id) {
> + if ((rev > mc_amd->hdr.patch_id) ||
> + (rev == mc_amd->hdr.patch_id && !(control & LATE_ALL_THREADS))) {
> ret = UCODE_OK;
> goto out;
> }
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 3afcf3de0dd4..5f3185d2814c 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -40,11 +40,15 @@
> #include <asm/cmdline.h>
> #include <asm/setup.h>
>
> +#include "internal.h"
> +
> #define DRIVER_VERSION "2.2"
>
> static struct microcode_ops *microcode_ops;
> static bool dis_ucode_ldr = true;
>
> +unsigned long control = LATE_ALL_THREADS;
> +
> bool initrd_gone;
>
> LIST_HEAD(microcode_cache);
> @@ -522,8 +526,32 @@ static ssize_t processor_flags_show(struct device *dev,
> return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
> }
>
> +static ssize_t control_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "0x%lx\n", control);
> +}
> +
> +static ssize_t control_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned long val;
> +
> + if (kstrtoul(buf, 0, &val) < 0)
> + return -ERANGE;
> +
> + if (val & CONTROL_FLAGS_MASK)
> + return -EINVAL;
> +
> + control = val;
> +
> + return count;
> +}
> +
> static DEVICE_ATTR_RO(version);
> static DEVICE_ATTR_RO(processor_flags);
> +static DEVICE_ATTR_ADMIN_RW(control);
>
> static struct attribute *mc_default_attrs[] = {
> &dev_attr_version.attr,
> @@ -622,6 +650,7 @@ static struct attribute *cpu_root_microcode_attrs[] = {
> #ifdef CONFIG_MICROCODE_LATE_LOADING
> &dev_attr_reload.attr,
> #endif
> + &dev_attr_control.attr,
Shouldn't the "control" be under LATE_LOADING? Since this only controls
late-loading behavior?
next prev parent reply other threads:[~2023-06-08 1:55 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-05 14:13 [PATCH 1/2] x86/microcode/AMD: Load late on both threads too Borislav Petkov
2023-06-05 14:13 ` [PATCH 2/2] x86/microcode: Add a "microcode=" command line option Borislav Petkov
2023-06-08 1:55 ` Ashok Raj [this message]
2023-06-09 12:28 ` Borislav Petkov
2023-06-09 15:37 ` Ashok Raj
2023-06-12 9:06 ` Borislav Petkov
2023-06-12 9:20 ` [tip: x86/microcode] " tip-bot2 for Borislav Petkov (AMD)
2023-06-12 15:26 ` [PATCH 2/2] " Thomas Gleixner
2023-06-12 15:42 ` Borislav Petkov
2023-06-12 16:04 ` Borislav Petkov
2023-06-12 17:23 ` Thomas Gleixner
2023-06-13 8:32 ` Borislav Petkov
2023-06-07 19:36 ` [PATCH 1/2] x86/microcode/AMD: Load late on both threads too Dave Hansen
2023-06-07 20:03 ` Borislav Petkov
2023-06-07 20:15 ` Dave Hansen
2023-08-16 20:17 ` Jim Mattson
2023-08-16 21:18 ` Borislav Petkov
2023-08-16 21:23 ` Jim Mattson
2023-08-16 21:30 ` Borislav Petkov
2023-08-16 21:36 ` Jim Mattson
2023-08-16 21:58 ` Borislav Petkov
2023-08-16 22:37 ` Jim Mattson
2023-08-17 15:40 ` Borislav Petkov
2023-08-17 18:02 ` Peter Shier
2023-08-18 8:43 ` Borislav Petkov
2023-06-12 9:20 ` [tip: x86/microcode] " tip-bot2 for Borislav Petkov (AMD)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZIE1G9UBAT36kPhJ@a4bf019067fa.jf.intel.com \
--to=ashok.raj@intel.com \
--cc=bp@alien8.de \
--cc=dave.hansen@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).