Workaround the following oops:
topology_hotplug_apic
topo_set_cpuids
set_cpu_possible(cpu, true);
// write to __ro_after_init section after init
adobriyan: I'm not sure what's going on, can it set unset bit here?
If not, then why does it repeat the job and set already set bits.
Anyhow, let's not oops peoples' machines for now.
Reported-by: Jonathan Cameron <[email protected]>
Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Alexey Dobriyan <[email protected]>
---
arch/x86/kernel/cpu/topology.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index aaca8d235dc2..42f4a17f8019 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -117,13 +117,18 @@ static __init int topo_get_cpunr(u32 apic_id)
return topo_info.nr_assigned_cpus++;
}
-static void topo_set_cpuids(unsigned int cpu, u32 apic_id, u32 acpi_id)
+static void topo_set_cpuids(unsigned int cpu, u32 apic_id, u32 acpi_id, bool from_init)
{
#if defined(CONFIG_SMP) || defined(CONFIG_X86_64)
early_per_cpu(x86_cpu_to_apicid, cpu) = apic_id;
early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
#endif
- set_cpu_possible(cpu, true);
+ if (from_init) {
+ set_cpu_possible(cpu, true);
+ } else {
+ /* cpu_possible_mask is supposed to be fixed by now. */
+ WARN_ON(!cpumask_test_cpu(cpu, cpu_possible_mask));
+ }
set_cpu_present(cpu, true);
}
@@ -191,7 +196,7 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
cpu = topo_get_cpunr(apic_id);
cpuid_to_apicid[cpu] = apic_id;
- topo_set_cpuids(cpu, apic_id, acpi_id);
+ topo_set_cpuids(cpu, apic_id, acpi_id, true);
} else {
u32 pkgid = topo_apicid(apic_id, TOPO_PKG_DOMAIN);
@@ -343,7 +348,7 @@ int topology_hotplug_apic(u32 apic_id, u32 acpi_id)
return -ENOSPC;
set_bit(apic_id, phys_cpu_present_map);
- topo_set_cpuids(cpu, apic_id, acpi_id);
+ topo_set_cpuids(cpu, apic_id, acpi_id, false);
cpu_mark_primary_thread(cpu, apic_id);
return cpu;
}
--
2.43.2
On Sun, Apr 07 2024 at 18:26, Alexey Dobriyan wrote:
> Workaround the following oops:
>
> topology_hotplug_apic
> topo_set_cpuids
> set_cpu_possible(cpu, true);
> // write to __ro_after_init section after init
Duh, yes.
> adobriyan: I'm not sure what's going on, can it set unset bit here?
> If not, then why does it repeat the job and set already set bits.
>
> Anyhow, let's not oops peoples' machines for now.
Adding a bandaid to paper over the non-understood real problem is
definitely not a good plan. I take this patch as a bug report.
Proper fix below.
Thanks,
tglx
---
Subject: x86/topology: Don't update cpu_possible_map in topo_set_cpuids()
From: Thomas Gleixner <[email protected]>
Date: Mon, 08 Apr 2024 15:22:01 +0200
topo_set_cpuids() updates cpu_present_map and cpu_possible map. It is
invoked during enumeration and "physical hotplug" operations. In the
latter case this results in a kernel crash because cpu_possible_map is
marked read only after init completes.
There is no reason to update cpu_possible_map in that function. During
enumeration cpu_possible_map is not relevant and gets fully initialized
after enumeration completed. On "physical hotplug" the bit is already set
because the kernel allows only CPUs to be plugged which have been
enumerated and associated to a CPU number during early boot.
Remove the bogus update of cpu_possible_map.
Fixes: 0e53e7b656cf ("x86/cpu/topology: Sanitize the APIC admission logic")
Reported-by: Jonathan Cameron <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/cpu/topology.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -123,7 +123,6 @@ static void topo_set_cpuids(unsigned int
early_per_cpu(x86_cpu_to_apicid, cpu) = apic_id;
early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
#endif
- set_cpu_possible(cpu, true);
set_cpu_present(cpu, true);
}
@@ -210,7 +209,11 @@ static __init void topo_register_apic(u3
topo_info.nr_disabled_cpus++;
}
- /* Register present and possible CPUs in the domain maps */
+ /*
+ * Register present and possible CPUs in the domain
+ * maps. cpu_possible_map will be updated in
+ * topology_init_possible_cpus() after enumeration is done.
+ */
for (dom = TOPO_SMT_DOMAIN; dom < TOPO_MAX_DOMAIN; dom++)
set_bit(topo_apicid(apic_id, dom), apic_maps[dom].map);
}
* Thomas Gleixner <[email protected]> wrote:
> On Sun, Apr 07 2024 at 18:26, Alexey Dobriyan wrote:
> > Workaround the following oops:
> >
> > topology_hotplug_apic
> > topo_set_cpuids
> > set_cpu_possible(cpu, true);
> > // write to __ro_after_init section after init
>
> Duh, yes.
>
> > adobriyan: I'm not sure what's going on, can it set unset bit here?
> > If not, then why does it repeat the job and set already set bits.
> >
> > Anyhow, let's not oops peoples' machines for now.
>
> Adding a bandaid to paper over the non-understood real problem is
> definitely not a good plan. I take this patch as a bug report.
>
> Proper fix below.
BTW., independently of the fix, warning about a too late set_cpu_possible()
might still make sense - clearly it *can* be called too late by
architecture init code :-)
It's not a performance-sensitive function in any case, so the extra
debugging code might not hurt.
Thanks,
Ingo
On Tue, Apr 09, 2024 at 10:42:13AM +0200, Ingo Molnar wrote:
>
> * Thomas Gleixner <[email protected]> wrote:
>
> > On Sun, Apr 07 2024 at 18:26, Alexey Dobriyan wrote:
> > > Workaround the following oops:
> > >
> > > topology_hotplug_apic
> > > topo_set_cpuids
> > > set_cpu_possible(cpu, true);
> > > // write to __ro_after_init section after init
> >
> > Duh, yes.
> >
> > > adobriyan: I'm not sure what's going on, can it set unset bit here?
> > > If not, then why does it repeat the job and set already set bits.
> > >
> > > Anyhow, let's not oops peoples' machines for now.
> >
> > Adding a bandaid to paper over the non-understood real problem is
> > definitely not a good plan. I take this patch as a bug report.
> >
> > Proper fix below.
>
> BTW., independently of the fix, warning about a too late set_cpu_possible()
> might still make sense - clearly it *can* be called too late by
> architecture init code :-)
Make the function __init ? Then it goes away right when the data becomes
RO.
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: a9025cd1c673a8d6eefc79d911075b8b452eba8f
Gitweb: https://git.kernel.org/tip/a9025cd1c673a8d6eefc79d911075b8b452eba8f
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 08 Apr 2024 15:22:01 +02:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 10 Apr 2024 15:31:38 +02:00
x86/topology: Don't update cpu_possible_map in topo_set_cpuids()
topo_set_cpuids() updates cpu_present_map and cpu_possible map. It is
invoked during enumeration and "physical hotplug" operations. In the
latter case this results in a kernel crash because cpu_possible_map is
marked read only after init completes.
There is no reason to update cpu_possible_map in that function. During
enumeration cpu_possible_map is not relevant and gets fully initialized
after enumeration completed. On "physical hotplug" the bit is already set
because the kernel allows only CPUs to be plugged which have been
enumerated and associated to a CPU number during early boot.
Remove the bogus update of cpu_possible_map.
Fixes: 0e53e7b656cf ("x86/cpu/topology: Sanitize the APIC admission logic")
Reported-by: Jonathan Cameron <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/r/87ttkc6kwx.ffs@tglx
---
arch/x86/kernel/cpu/topology.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index aaca8d2..d17c9b7 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -123,7 +123,6 @@ static void topo_set_cpuids(unsigned int cpu, u32 apic_id, u32 acpi_id)
early_per_cpu(x86_cpu_to_apicid, cpu) = apic_id;
early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
#endif
- set_cpu_possible(cpu, true);
set_cpu_present(cpu, true);
}
@@ -210,7 +209,11 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
topo_info.nr_disabled_cpus++;
}
- /* Register present and possible CPUs in the domain maps */
+ /*
+ * Register present and possible CPUs in the domain
+ * maps. cpu_possible_map will be updated in
+ * topology_init_possible_cpus() after enumeration is done.
+ */
for (dom = TOPO_SMT_DOMAIN; dom < TOPO_MAX_DOMAIN; dom++)
set_bit(topo_apicid(apic_id, dom), apic_maps[dom].map);
}
The following commit has been merged into the x86/alternatives branch of tip:
Commit-ID: 675ae1aac0572595999e6f78a16e51351291f7aa
Gitweb: https://git.kernel.org/tip/675ae1aac0572595999e6f78a16e51351291f7aa
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 08 Apr 2024 15:22:01 +02:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 10 Apr 2024 15:24:27 +02:00
x86/topology: Don't update cpu_possible_map in topo_set_cpuids()
topo_set_cpuids() updates cpu_present_map and cpu_possible map. It is
invoked during enumeration and "physical hotplug" operations. In the
latter case this results in a kernel crash because cpu_possible_map is
marked read only after init completes.
There is no reason to update cpu_possible_map in that function. During
enumeration cpu_possible_map is not relevant and gets fully initialized
after enumeration completed. On "physical hotplug" the bit is already set
because the kernel allows only CPUs to be plugged which have been
enumerated and associated to a CPU number during early boot.
Remove the bogus update of cpu_possible_map.
Fixes: 0e53e7b656cf ("x86/cpu/topology: Sanitize the APIC admission logic")
Reported-by: Jonathan Cameron <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/r/87ttkc6kwx.ffs@tglx
---
arch/x86/kernel/cpu/topology.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index aaca8d2..d17c9b7 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -123,7 +123,6 @@ static void topo_set_cpuids(unsigned int cpu, u32 apic_id, u32 acpi_id)
early_per_cpu(x86_cpu_to_apicid, cpu) = apic_id;
early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
#endif
- set_cpu_possible(cpu, true);
set_cpu_present(cpu, true);
}
@@ -210,7 +209,11 @@ static __init void topo_register_apic(u32 apic_id, u32 acpi_id, bool present)
topo_info.nr_disabled_cpus++;
}
- /* Register present and possible CPUs in the domain maps */
+ /*
+ * Register present and possible CPUs in the domain
+ * maps. cpu_possible_map will be updated in
+ * topology_init_possible_cpus() after enumeration is done.
+ */
for (dom = TOPO_SMT_DOMAIN; dom < TOPO_MAX_DOMAIN; dom++)
set_bit(topo_apicid(apic_id, dom), apic_maps[dom].map);
}