Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
In-Reply-To: <20230612154246.GLZIc89v6Q2THgsY8N@fat_crate.local>
References: <20230605141332.25948-1-bp@alien8.de>
 <20230605141332.25948-2-bp@alien8.de> <87ilbs7lcr.ffs@tglx>
 <20230612154246.GLZIc89v6Q2THgsY8N@fat_crate.local>
Date:   Mon, 12 Jun 2023 19:23:19 +0200
Message-ID: <87a5x47fy0.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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