| Message ID | 20240624212008.663832-2-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rework mce_setup() | expand |
On Mon, Jun 24 2024 at 16:20, Yazen Ghannam wrote: > The need to look up a CPU number from an APIC ID is done in at least one > other place outside of APIC/topology code: > apei_smca_report_x86_error(). The need .... is done? > #ifdef CONFIG_X86_LOCAL_APIC > int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level); > +int topology_get_cpunr(u32 apic_id); > #else > static inline int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level) > { > return 0; > } > + > +static inline int topology_get_cpunr(u32 apic_id) > +{ > + return -ENODEV; Why ENODEV and not 0? > +} > #endif Thanks, tglx
On Tue, Jun 25, 2024 at 08:50:13AM +0200, Thomas Gleixner wrote: > On Mon, Jun 24 2024 at 16:20, Yazen Ghannam wrote: > > > The need to look up a CPU number from an APIC ID is done in at least one > > other place outside of APIC/topology code: > > apei_smca_report_x86_error(). > > The need .... is done? Hmm, yeah the wording isn't clear. I'll rephrase it. > > > #ifdef CONFIG_X86_LOCAL_APIC > > int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level); > > +int topology_get_cpunr(u32 apic_id); > > #else > > static inline int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level) > > { > > return 0; > > } > > + > > +static inline int topology_get_cpunr(u32 apic_id) > > +{ > > + return -ENODEV; > > Why ENODEV and not 0? > Since '0' is a valid CPU number, my first thought was that we should return an error code if we can't explicitly find the correct CPU number. But would/could we have SMP support without X86_LOCAL_APIC? If not, then we'd only have CPU 0 in any case. Is this why the function above returns 0? I can make the change to return 0 here also. But... The MCE code depends on X86_LOCAL_APIC, and it is the only external user. So should I drop the config check and export? Thanks, Yazen
On Tue, Jun 25, 2024 at 09:42:12PM -0400, Yazen Ghannam wrote:
> But would/could we have SMP support without X86_LOCAL_APIC?
Look at how X86_LOCAL_APIC is defined in Kconfig.
On Fri, Jun 28, 2024 at 10:37:22AM +0200, Borislav Petkov wrote: > On Tue, Jun 25, 2024 at 09:42:12PM -0400, Yazen Ghannam wrote: > > But would/could we have SMP support without X86_LOCAL_APIC? > > Look at how X86_LOCAL_APIC is defined in Kconfig. > Thanks for the tip. It looks to me that SMP and X86_LOCAL_APIC are generally independent. Thanks, Yazen
On Fri, Jun 28, 2024 at 10:15:42AM -0400, Yazen Ghannam wrote: > Thanks for the tip. It looks to me that SMP and X86_LOCAL_APIC are > generally independent. They are? config X86_LOCAL_APIC def_bool y depends on X86_64 || SMP ^^^^^^^ ^^^
On Fri, Jun 28, 2024 at 04:45:35PM +0200, Borislav Petkov wrote: > On Fri, Jun 28, 2024 at 10:15:42AM -0400, Yazen Ghannam wrote: > > Thanks for the tip. It looks to me that SMP and X86_LOCAL_APIC are > > generally independent. > > They are? > > config X86_LOCAL_APIC > def_bool y > depends on X86_64 || SMP > ^^^^^^^ ^^^ > Right, SMP does not depend on X86_LOCAL_APIC. Otherwise, there would be a circular dependency here. X86_LOCAL_APIC depends on the logical OR of a bunch of options. So it depends on "any one" of the options to be enabled. But it doesn't need all of them. config UP_LATE_INIT def_bool y depends on !SMP && X86_LOCAL_APIC ^^^^^^^^^^^^ This shows that X86_LOCAL_APIC doesn't have a hard dependency on SMP. Otherwise, this option would never work. Thanks, Yazen
On Mon, Jul 01, 2024 at 01:51:42PM -0400, Yazen Ghannam wrote: > X86_LOCAL_APIC depends on the logical OR of a bunch of options. So it > depends on "any one" of the options to be enabled. But it doesn't need > all of them. Yes, I can see that. How does any of that info answer your initial question? IOW, if you don't have LAPIC support in the kernel, what CPU number should we return for any APIC ID serving as input, and why?
On Mon, Jul 01, 2024 at 09:07:04PM +0200, Borislav Petkov wrote: > On Mon, Jul 01, 2024 at 01:51:42PM -0400, Yazen Ghannam wrote: > > X86_LOCAL_APIC depends on the logical OR of a bunch of options. So it > > depends on "any one" of the options to be enabled. But it doesn't need > > all of them. > > Yes, I can see that. > > How does any of that info answer your initial question? > > IOW, if you don't have LAPIC support in the kernel, what CPU number should we > return for any APIC ID serving as input, and why? > I still think it should return an error code, because theoretically LAPIC can be disabled and SMP can be enabled. But I spent some time trying to see if this would work in practice, and it looks like you can't disable X86_LOCAL_APIC without hitting a bunch of build errors on x86_64. It seems like a lot of common APIC and SMP code implicitly depends on X86_LOCAL_APIC. This was true even for a tiny config. However, this worked for an i386 build (with SMP=n). I think the most practical option is to keep the local search routine in mce/apei. I don't think all the additional complexity is worth it for a simple for-loop. Regarding the X86_LOCAL_APIC=n build issues, should these be investigated? Thanks, Yazen
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index abe3a8f22cbd..d8c3c3c818bc 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -171,11 +171,17 @@ static inline unsigned int topology_num_threads_per_package(void) #ifdef CONFIG_X86_LOCAL_APIC int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level); +int topology_get_cpunr(u32 apic_id); #else static inline int topology_get_logical_id(u32 apicid, enum x86_topology_domains at_level) { return 0; } + +static inline int topology_get_cpunr(u32 apic_id) +{ + return -ENODEV; +} #endif #ifdef CONFIG_SMP diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c index 621a151ccf7d..fc74578ee3bd 100644 --- a/arch/x86/kernel/cpu/topology.c +++ b/arch/x86/kernel/cpu/topology.c @@ -95,7 +95,15 @@ static inline u32 topo_apicid(u32 apicid, enum x86_topology_domains dom) return apicid & (UINT_MAX << x86_topo_system.dom_shifts[dom - 1]); } -static int topo_lookup_cpuid(u32 apic_id) +/** + * topology_get_cpunr - Retrieve the CPU number for the given APIC ID + * @apic_id: The APIC ID for which to lookup the CPU number + * + * Returns: + * - >= 0: The CPU number for the given APIC ID + * - -ENODEV: @apic_id does not match any known CPU + */ +int topology_get_cpunr(u32 apic_id) { int i; @@ -106,10 +114,11 @@ static int topo_lookup_cpuid(u32 apic_id) } return -ENODEV; } +EXPORT_SYMBOL_GPL(topology_get_cpunr); static __init int topo_get_cpunr(u32 apic_id) { - int cpu = topo_lookup_cpuid(apic_id); + int cpu = topology_get_cpunr(apic_id); if (cpu >= 0) return cpu; @@ -388,7 +397,7 @@ int topology_hotplug_apic(u32 apic_id, u32 acpi_id) if (!test_bit(apic_id, apic_maps[TOPO_SMT_DOMAIN].map)) return -ENODEV; - cpu = topo_lookup_cpuid(apic_id); + cpu = topology_get_cpunr(apic_id); if (cpu < 0) return -ENOSPC;
The need to look up a CPU number from an APIC ID is done in at least one other place outside of APIC/topology code: apei_smca_report_x86_error(). With the recent topology rework, there is now a helper function to do just this task. Export the helper so other code can use it. Also, update the name to match other exported topology_* helpers. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Link: https://lkml.kernel.org/r/20240618172447.GA1387@yaz-khff2.amd.com v1->v2: * New in v2. arch/x86/include/asm/topology.h | 6 ++++++ arch/x86/kernel/cpu/topology.c | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-)