sys/x86/x86/ucode.c: Do not use wrmsr_early_safe() for Intel

[jrm]

Summary

Intel's MSR_BIOS_UPDT_TRIG does not issue #GP, making the
wrmsr_early_safe() fault-handling unnecessary on Intel. The mechanism
was introduced to handle a specific AMD problem where certain EPYC CPUs
issue #GP when the OS attempts to load microcode before a minimum
BIOS-level microcode version is already applied.

Make the EARLY case fall through to the bare wrmsr() path,
restoring the pre-d9f03a43f2fe behaviour for Intel.

PR: 294630
Fixes: d9f03a43f2fe (ucode: use wrmsr_early_safe() for early CPU microcode update)

[imp]

What are the risks this a avoids?

[jrm]

In D57054#1307284, @imp wrote:

What are the risks this a avoids?

I meant this in a very general sense, in that changing the way this is applied *seems* to have introduced the booting problem as people are describing in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=294630.
As I said (or implied) in the bug report, I'm unsure.

My money is on d9f03a43f2fec, especially since the commit messages say, "This has been tested only on AMD booted in EFI mode."
I'll add glebius@, kib@, markj@ (and emaste@) because they worked in this area and understand the code.

[kib]

In D57054#1307284, @imp wrote:

What are the risks this a avoids?

I would even ask, why it matters. What is the behavior that makes this change required?

[FWIW #GP is allowed for any MSR access]

[kib]

I looked at the referenced bug. Could you please conduct an experiment where the failing machine is booting kernel with just the WRMSR write to actually update the ucode is commented out? I mean, remove only the instruction, but keep the setup for wrmsr_safe intact. Does the machine boot in this case?

[jrm]

In D57054#1307326, @kib wrote:

I looked at the referenced bug. Could you please conduct an experiment where the failing machine is booting kernel with just the WRMSR write to actually update the ucode is commented out? I mean, remove only the instruction, but keep the setup for wrmsr_safe intact. Does the machine boot in this case?

I'll try to answer both questions here.

My understanding of this is not much beyond this: Commit d9f03a43f2fe seemed the most likely change to have caused a problem, especially since @glebius said Intel wasn't tested. I should have repeated what I said in the bug report to make it clear that this is, in large part, not much beyond an experiment to see if restoring the old behaviour fixes the problem. Unfortunately, as I said to @cperciva on IRC, I don't have access to any affected system. I'll ask the reporters in the bug report if they can run your experiment.

Comments/questions for you @kib:

I'm trying to follow the trail. I think it goes like this.

• From hammer_time() in machdep.c we call ucode_load_bsp()
• From ucode_load_bsp(), we call ucode_loader->match()
• If a match is found, we call ucode_loader->load(match, EARLY, ...) (ucode_intel_load())
• ucode_intel_load() takes case EARLY: and calls wrmsr_early_safe_start()
• wrmsr_early_safe_start() calls sidt(&wrmsr_early_safe_orig_efi_idt)

Is the IDT ready at that point? I don't think setidt() has been called yet. Is that a problem?

[jrm]

In D57054#1307326, @kib wrote:

I looked at the referenced bug. Could you please conduct an experiment where the failing machine is booting kernel with just the WRMSR write to actually update the ucode is commented out? I mean, remove only the instruction, but keep the setup for wrmsr_safe intact. Does the machine boot in this case?

@kib, can you confirm this is what you're asking for?

diff --git a/sys/x86/x86/ucode.c b/sys/x86/x86/ucode.c
index 72133de211f8..457da9db3c29 100644
--- a/sys/x86/x86/ucode.c
+++ b/sys/x86/x86/ucode.c
@@ -135,9 +135,10 @@ ucode_intel_load(const void *data, ucode_load_how how, uint64_t *nrevp,
    case EARLY:
 #ifdef __amd64__
        wrmsr_early_safe_start();
-       if (wrmsr_early_safe(MSR_BIOS_UPDT_TRIG,
+       /* if (wrmsr_early_safe(MSR_BIOS_UPDT_TRIG,
            (uint64_t)(uintptr_t)data) != 0)
            ucode_error = LOAD_FAILED;
+                */
        wrmsr_early_safe_end();
        break;
 #endif

[kib]

In D57054#1307333, @jrm wrote:

In D57054#1307326, @kib wrote:

I looked at the referenced bug. Could you please conduct an experiment where the failing machine is booting kernel with just the WRMSR write to actually update the ucode is commented out? I mean, remove only the instruction, but keep the setup for wrmsr_safe intact. Does the machine boot in this case?

I'll try to answer both questions here.

My understanding of this is not much beyond this: Commit d9f03a43f2fe seemed the most likely change to have caused a problem, especially since @glebius said Intel wasn't tested.

I do not know what glebius@ tested. I checked both Intel and AMD hardware when I developed the wrmsr_early_safe(), not with the ucode updater, but with a custom code specifically to access MSR which caused #GP.

I should have repeated what I said in the bug report to make it clear that this is, in large part, not much beyond an experiment to see if restoring the old behaviour fixes the problem. Unfortunately, as I said to @cperciva on IRC, I don't have access to any affected system. I'll ask the reporters in the bug report if they can run your experiment.

Comments/questions for you @kib:

I'm trying to follow the trail. I think it goes like this.

• From hammer_time() in machdep.c we call ucode_load_bsp()
• From ucode_load_bsp(), we call ucode_loader->match()
• If a match is found, we call ucode_loader->load(match, EARLY, ...) (ucode_intel_load())
• ucode_intel_load() takes case EARLY: and calls wrmsr_early_safe_start()
• wrmsr_early_safe_start() calls sidt(&wrmsr_early_safe_orig_efi_idt)

Is the IDT ready at that point? I don't think setidt() has been called yet. Is that a problem?

There is some IDT that was left by BIOS, otherwise UEFI cannot properly function. But this does not matter much because we switch to some temporal IDT for the scope of the wrmsr_early_safe() dance. After it is done, we switch back to the UEFI IDT, whatever it was. The later is not strictly needed, but it is good to have since a UEFI debugger would be able to catch early exceptions in hammer_time().

In D57054#1307373, @jrm wrote:

In D57054#1307326, @kib wrote:

I looked at the referenced bug. Could you please conduct an experiment where the failing machine is booting kernel with just the WRMSR write to actually update the ucode is commented out? I mean, remove only the instruction, but keep the setup for wrmsr_safe intact. Does the machine boot in this case?

@kib, can you confirm this is what you're asking for?

diff --git a/sys/x86/x86/ucode.c b/sys/x86/x86/ucode.c
index 72133de211f8..457da9db3c29 100644
--- a/sys/x86/x86/ucode.c
+++ b/sys/x86/x86/ucode.c
@@ -135,9 +135,10 @@ ucode_intel_load(const void *data, ucode_load_how how, uint64_t *nrevp,
    case EARLY:
 #ifdef __amd64__
        wrmsr_early_safe_start();
-       if (wrmsr_early_safe(MSR_BIOS_UPDT_TRIG,
+       /* if (wrmsr_early_safe(MSR_BIOS_UPDT_TRIG,
            (uint64_t)(uintptr_t)data) != 0)
            ucode_error = LOAD_FAILED;
+                */
        wrmsr_early_safe_end();
        break;
 #endif

No, this removes the whole call to wrmsr_early_safe(). I want to keep it, but not do WRMSR.

diff --git a/sys/amd64/amd64/support.S b/sys/amd64/amd64/support.S
index 09d4ef85b087..989390abaadb 100644
--- a/sys/amd64/amd64/support.S
+++ b/sys/amd64/amd64/support.S
@@ -1570,7 +1570,7 @@ ENTRY(wrmsr_early_safe)
 	movl	%esi,%eax
 	sarq	$32,%rsi
 	movl	%esi,%edx
-	wrmsr
+/*	wrmsr */
 	xorl	%eax,%eax
 wrmsr_early_faulted:
 	ret