From: Thomas Gleixner <tglx@linutronix.de>
To: Borislav Petkov <bp@alien8.de>
Cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] x86/microcode: Add a "microcode=" command line option
Date: Mon, 12 Jun 2023 19:23:19 +0200 [thread overview]
Message-ID: <87a5x47fy0.ffs@tglx> (raw)
In-Reply-To: <20230612154246.GLZIc89v6Q2THgsY8N@fat_crate.local>
On Mon, Jun 12 2023 at 17:42, Borislav Petkov wrote:
> On Mon, Jun 12, 2023 at 05:26:28PM +0200, Thomas Gleixner wrote:
>> Why is it suddenly required to prevent late loading on SMT threads?
>
> The intent is, like a chicken bit, to revert to the *old* behavior which
> would not load on both threads. In *case* some old configuration of CPU
> and microcode cannot handle loading on both threads. Which is from
> Bulldozer onwards but I don't think anyone uses Bulldozer anymore.
So why not making the late loading thing depend on >= bulldozer?
Also I'm failing to see how this is different from the early loader:
>> That's the exact opposite of what e7ad18d1169c ("x86/microcode/AMD:
>> Apply the patch early on every logical thread") is doing.
That changelogs says:
"Btw, change only the early paths. On the late loading paths, there's
no point in doing per-thread modification because if is it some case
like in the bugzilla below - removing a CPUID flag - the kernel cannot
go and un-use features it has detected are there early. For that, one
should use early loading anyway."
which makes sense to some extent, but obviously there is a use case for
late loading on both threads. So what are you worried about breaking
here?
If the late load does a behavioural change, then it does not matter
whether you make sure both threads are hosed in the same way or not. If
the late load is harmless and just addressing some correctness issue
then loading on the secondary thread should be a noop, right?
> No, see patch 1 - it does exactly the same what this commit does but for
> late loading.
Sorry no. Patch 1 brings the late loading decision in line with the
early loading decision, i.e. ensure that microcode is loaded on both
threads. That condition
/* need to apply patch? */
- if (rev >= mc_amd->hdr.patch_id) {
+ if (rev > mc_amd->hdr.patch_id) {
really could do with a proper comment which explains why loading the
same revision makes sense.
> Bottomline: on AMD, we should load on both threads by default.
Fine. Then what is this about? If it survives early loading on both
threads then why do we need a chickenbit for late loading?
So if someone turns that on needlessly then in the worst case the
primary thread behaves differently than the secondary thread, right?
>> Aside of that why is this a kernel side chicken bit and not communicated
>> by the microcode header?
>
> See above. This chicken bit is there, just in case, to help in the case
> where the user cannot do anything else. It should not be used, judging
> by all the combinations I've tested here.
If it should not be used, then where is the big fat warning emitted when
it is actually enabled?
The more I read this the less I'm convinced that this makes any sense at
all:
1) You did not add a chicken bit when you made this change for the
early loader. Why needs late loading suddenly one?
2) Late loading is "use at your own peril" up to the point where the
minimum revision field is in place. So why do we need a bandaid
chickenbit for something which is considered unsafe anyway?
3) The microcode people @AMD should be able to figure out whether
unconditionally (late) loading on the secondary thread is safe or
not.
I told Intel to make microcode loading something sane. It's not any
different for AMD. This hastily cobbled together chickenbit thing
without a fundamental technical justification definitely does not
quality for sane.
>> Why ULL bits for a unsigned long variable?
>
> There's no BIT_UL() macro.
git grep '#define BIT(' include/
Thanks,
tglx
next prev parent reply other threads:[~2023-06-12 17:23 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 ` [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 [this message]
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=87a5x47fy0.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).