2023-10-02 13:56:59

by Thomas Gleixner

[permalink] [raw]
Subject: [patch V4 15/30] x86/microcode: Remove pointless apply() invocation

Microcode is applied on the APs during early bringup. There is no point in
trying to apply the microcode again during the hotplug operations and
neither at the point where the microcode device is initialized.

Collect CPU info and microcode revision in setup_online_cpu() for now. This
will move to the CPU hotplug callback in the next step.

Signed-off-by: Thomas Gleixner <[email protected]>
---
V2: New patch
---
arch/x86/kernel/cpu/microcode/core.c | 34 ++++++----------------------------
include/linux/cpuhotplug.h | 1 -
2 files changed, 6 insertions(+), 29 deletions(-)

--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -490,17 +490,6 @@ static void microcode_fini_cpu(int cpu)
microcode_ops->microcode_fini_cpu(cpu);
}

-static enum ucode_state microcode_init_cpu(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- memset(uci, 0, sizeof(*uci));
-
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
-
- return microcode_ops->apply_microcode(cpu);
-}
-
/**
* microcode_bsp_resume - Update boot CPU microcode during resume.
*/
@@ -519,15 +508,6 @@ static struct syscore_ops mc_syscore_ops
.resume = microcode_bsp_resume,
};

-static int mc_cpu_starting(unsigned int cpu)
-{
- enum ucode_state err = microcode_ops->apply_microcode(cpu);
-
- pr_debug("%s: CPU%d, err: %d\n", __func__, cpu, err);
-
- return err == UCODE_ERROR;
-}
-
static int mc_cpu_online(unsigned int cpu)
{
struct device *dev = get_cpu_device(cpu);
@@ -555,14 +535,14 @@ static int mc_cpu_down_prep(unsigned int
static void setup_online_cpu(struct work_struct *work)
{
int cpu = smp_processor_id();
- enum ucode_state err;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- err = microcode_init_cpu(cpu);
- if (err == UCODE_ERROR) {
- pr_err("Error applying microcode on CPU%d\n", cpu);
- return;
- }
+ memset(uci, 0, sizeof(*uci));

+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ cpu_data(cpu).microcode = uci->cpu_sig.rev;
+ if (!cpu)
+ boot_cpu_data.microcode = uci->cpu_sig.rev;
mc_cpu_online(cpu);
}

@@ -615,8 +595,6 @@ static int __init microcode_init(void)
schedule_on_each_cpu(setup_online_cpu);

register_syscore_ops(&mc_syscore_ops);
- cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting",
- mc_cpu_starting, NULL);
cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
mc_cpu_online, mc_cpu_down_prep);

--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -156,7 +156,6 @@ enum cpuhp_state {
CPUHP_AP_IRQ_LOONGARCH_STARTING,
CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
CPUHP_AP_ARM_MVEBU_COHERENCY,
- CPUHP_AP_MICROCODE_LOADER,
CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
CPUHP_AP_PERF_X86_STARTING,
CPUHP_AP_PERF_X86_AMD_IBS_STARTING,


2023-10-06 13:26:48

by Borislav Petkov

[permalink] [raw]
Subject: Re: [patch V4 15/30] x86/microcode: Remove pointless apply() invocation

On Mon, Oct 02, 2023 at 01:59:52PM +0200, Thomas Gleixner wrote:
> Microcode is applied on the APs during early bringup. There is no point in
> trying to apply the microcode again during the hotplug operations and
> neither at the point where the microcode device is initialized.
>
> Collect CPU info and microcode revision in setup_online_cpu() for now. This
> will move to the CPU hotplug callback in the next step.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> V2: New patch
> ---
> arch/x86/kernel/cpu/microcode/core.c | 34 ++++++----------------------------
> include/linux/cpuhotplug.h | 1 -
> 2 files changed, 6 insertions(+), 29 deletions(-)

Partial revert, as discussed. I've left enough breadcrumbs in case we
wanna tackle this again in the future:

---
From: Thomas Gleixner <[email protected]>
Date: Mon, 2 Oct 2023 13:59:52 +0200
Subject: [PATCH] x86/microcode: Remove pointless apply() invocation

Microcode is applied on the APs during early bringup. There is no point
in trying to apply the microcode again during the hotplug operations and
neither at the point where the microcode device is initialized.

Collect CPU info and microcode revision in setup_online_cpu() for now.
This will move to the CPU hotplug callback later.

[ bp: Leave the starting notifier for the following scenario:

- boot, late load, suspend to disk, resume

without the starting notifier, only the last core manages to update the
microcode upon resume:

# rdmsr -a 0x8b
10000bf
10000bf
10000bf
10000bf
10000bf
10000dc <----

This is on an AMD F10h machine.

For the future, one should check whether potential unification of
the CPU init path could cover the resume path too so that this can
be simplified even more. ]

Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/microcode/core.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 0d99d5eab3fc..6f7632fa2e61 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -490,17 +490,6 @@ static void microcode_fini_cpu(int cpu)
microcode_ops->microcode_fini_cpu(cpu);
}

-static enum ucode_state microcode_init_cpu(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- memset(uci, 0, sizeof(*uci));
-
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
-
- return microcode_ops->apply_microcode(cpu);
-}
-
/**
* microcode_bsp_resume - Update boot CPU microcode during resume.
*/
@@ -555,14 +544,14 @@ static int mc_cpu_down_prep(unsigned int cpu)
static void setup_online_cpu(struct work_struct *work)
{
int cpu = smp_processor_id();
- enum ucode_state err;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- err = microcode_init_cpu(cpu);
- if (err == UCODE_ERROR) {
- pr_err("Error applying microcode on CPU%d\n", cpu);
- return;
- }
+ memset(uci, 0, sizeof(*uci));

+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ cpu_data(cpu).microcode = uci->cpu_sig.rev;
+ if (!cpu)
+ boot_cpu_data.microcode = uci->cpu_sig.rev;
mc_cpu_online(cpu);
}

--
2.42.0.rc0.25.ga82fb66fed25

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Subject: [tip: x86/microcode] x86/microcode: Remove pointless apply() invocation

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID: 034eaca8e03547ebff0b3974b88d2b772d858533
Gitweb: https://git.kernel.org/tip/034eaca8e03547ebff0b3974b88d2b772d858533
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 02 Oct 2023 13:59:52 +02:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Fri, 06 Oct 2023 15:05:18 +02:00

x86/microcode: Remove pointless apply() invocation

Microcode is applied on the APs during early bringup. There is no point
in trying to apply the microcode again during the hotplug operations and
neither at the point where the microcode device is initialized.

Collect CPU info and microcode revision in setup_online_cpu() for now.
This will move to the CPU hotplug callback later.

[ bp: Leave the starting notifier for the following scenario:

- boot, late load, suspend to disk, resume

without the starting notifier, only the last core manages to update the
microcode upon resume:

# rdmsr -a 0x8b
10000bf
10000bf
10000bf
10000bf
10000bf
10000dc <----

This is on an AMD F10h machine.

For the future, one should check whether potential unification of
the CPU init path could cover the resume path too so that this can
be simplified even more. ]

Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/microcode/core.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 0d99d5e..6f7632f 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -490,17 +490,6 @@ static void microcode_fini_cpu(int cpu)
microcode_ops->microcode_fini_cpu(cpu);
}

-static enum ucode_state microcode_init_cpu(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- memset(uci, 0, sizeof(*uci));
-
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
-
- return microcode_ops->apply_microcode(cpu);
-}
-
/**
* microcode_bsp_resume - Update boot CPU microcode during resume.
*/
@@ -555,14 +544,14 @@ static int mc_cpu_down_prep(unsigned int cpu)
static void setup_online_cpu(struct work_struct *work)
{
int cpu = smp_processor_id();
- enum ucode_state err;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- err = microcode_init_cpu(cpu);
- if (err == UCODE_ERROR) {
- pr_err("Error applying microcode on CPU%d\n", cpu);
- return;
- }
+ memset(uci, 0, sizeof(*uci));

+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ cpu_data(cpu).microcode = uci->cpu_sig.rev;
+ if (!cpu)
+ boot_cpu_data.microcode = uci->cpu_sig.rev;
mc_cpu_online(cpu);
}