From: Thomas Gleixner <tglx@linutronix.de>
To: Borislav Petkov <bp@alien8.de>, X86 ML <x86@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] x86/microcode: Add a "microcode=" command line option
Date: Mon, 12 Jun 2023 17:26:28 +0200 [thread overview]
Message-ID: <87ilbs7lcr.ffs@tglx> (raw)
In-Reply-To: <20230605141332.25948-2-bp@alien8.de>
On Mon, Jun 05 2023 at 16:13, 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.
This has a distinct void of information here.
Why is it suddenly required to prevent late loading on SMT threads?
That's the exact opposite of what e7ad18d1169c ("x86/microcode/AMD:
Apply the patch early on every logical thread") is doing.
I'm seriously confused.
>
> + 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.
no_late_all is a horrible option name.
Also the explanation is not mentioning that this is only relevant for
late loading.
Aside of that why is this a kernel side chicken bit and not communicated
by the microcode header?
How should an administrator know which microcode versions need this
treatment and which do not? How is that supposed to work on a fleet?
> --- 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;
global variable name without a proper prefix. Moo.
> +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;
> +}
Where is the documentation which tells me what I'm supposed to write
into this file? Also this is a generic file, right? So what's the
meaning for non AMD? I can write this bit into it successfully and
nothing happens, right?
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __X86_MICROCODE_INTERNAL_H__
> +#define __X86_MICROCODE_INTERNAL_H__
> +
> +extern unsigned long control;
> +
> +/* Loader control flags. */
> +enum control_flags {
> + __LATE_ALL_THREADS = 0,
> + __CONTROL_FLAGS_NUM,
> +};
> +
> +#define LATE_ALL_THREADS BIT_ULL(__LATE_ALL_THREADS)
> +#define CONTROL_FLAGS_MASK ~(BIT_ULL(__CONTROL_FLAGS_NUM) - 1)
Why ULL bits for a unsigned long variable?
Thanks,
tglx
next prev parent reply other threads:[~2023-06-12 15:26 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
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 ` Thomas Gleixner [this message]
2023-06-12 15:42 ` [PATCH 2/2] " 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=87ilbs7lcr.ffs@tglx \
--to=tglx@linutronix.de \
--cc=bp@alien8.de \
--cc=linux-kernel@vger.kernel.org \
--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).