[PATCH] x86/apic: Fix CPU devicetree-node lookups

[PATCH] x86/apic: Fix CPU devicetree-node lookups

[Johan Hovold]

Fix CPU devicetree-node lookups by implementing
arch_match_cpu_phys_id().

This allows using of_get_cpu_node() and of_cpu_device_node_get() to look
up CPU devicetree nodes and specifically makes sure that CPU struct
devices are linked to the correct devicetree nodes.

Note that CPUs are described in devicetree using their APIC ids and
that those do not generally coincide with the logical ids (e.g. used by
the default arch_match_cpu_phys_id() implementation).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 arch/x86/kernel/apic/apic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b3eef1d5c903..19c0119892dd 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2311,6 +2311,11 @@ static int cpuid_to_apicid[] = {
 	[0 ... NR_CPUS - 1] = -1,
 };
 
+bool arch_match_cpu_phys_id(int cpu, u64 phys_id)
+{
+	return phys_id == cpuid_to_apicid[cpu];
+}
+
 #ifdef CONFIG_SMP
 /**
  * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread
-- 
2.26.2

Re: [PATCH] x86/apic: Fix CPU devicetree-node lookups

[Johan Hovold]

On Tue, Nov 24, 2020 at 02:42:47PM +0100, Johan Hovold wrote:
> Fix CPU devicetree-node lookups by implementing
> arch_match_cpu_phys_id().
> 
> This allows using of_get_cpu_node() and of_cpu_device_node_get() to look
> up CPU devicetree nodes and specifically makes sure that CPU struct
> devices are linked to the correct devicetree nodes.
> 
> Note that CPUs are described in devicetree using their APIC ids and
> that those do not generally coincide with the logical ids (e.g. used by
> the default arch_match_cpu_phys_id() implementation).
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  arch/x86/kernel/apic/apic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index b3eef1d5c903..19c0119892dd 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2311,6 +2311,11 @@ static int cpuid_to_apicid[] = {
>  	[0 ... NR_CPUS - 1] = -1,
>  };
>  
> +bool arch_match_cpu_phys_id(int cpu, u64 phys_id)
> +{
> +	return phys_id == cpuid_to_apicid[cpu];
> +}
> +
>  #ifdef CONFIG_SMP
>  /**
>   * apic_id_is_primary_thread - Check whether APIC ID belongs to a primary thread

Any comments to this one? Can it be picked up for 5.11?

Johan

Re: [PATCH] x86/apic: Fix CPU devicetree-node lookups

[Thomas Gleixner]

On Tue, Dec 08 2020 at 11:05, Johan Hovold wrote:

> On Tue, Nov 24, 2020 at 02:42:47PM +0100, Johan Hovold wrote:
>> Fix CPU devicetree-node lookups by implementing
>> arch_match_cpu_phys_id().
>> 
>> This allows using of_get_cpu_node() and of_cpu_device_node_get() to look
>> up CPU devicetree nodes and specifically makes sure that CPU struct
>> devices are linked to the correct devicetree nodes.
>> 
>> Note that CPUs are described in devicetree using their APIC ids and
>> that those do not generally coincide with the logical ids (e.g. used by
>> the default arch_match_cpu_phys_id() implementation).

Can you please explain why this is relevant for any of the existing few
x86 DT users?

The above is incoherent word salad TBH:

    1) Fix .....

       without any information what is broken

    2) This allows ...

       It allows. Cool. Is it mandatory, desired or whatever?

    3) Note ...

       That might be an actual problem decsription, but I can't say for
       sure.

Changelog structure should be:

   Set context if necessary

   Describe the problem and the possible impact

   Describe the solution

and not:

    Claim to fix something unspecified

    Give some explanation how cool it is

    Add random notes what might or might not be solved by that.

I still have no idea what this 'Fix' is fixing if it is fixing anything
at all. Is is a fix which needs to be backported or is it a 'fix' which
fixes something which got changed elsewhere and forgotten.

Sorry, my crystalball got lost.

Thanks,

        tglx

Re: [PATCH] x86/apic: Fix CPU devicetree-node lookups

[Johan Hovold]

On Wed, Dec 09, 2020 at 03:19:26AM +0100, Thomas Gleixner wrote:
> On Tue, Dec 08 2020 at 11:05, Johan Hovold wrote:
> 
> > On Tue, Nov 24, 2020 at 02:42:47PM +0100, Johan Hovold wrote:
> >> Fix CPU devicetree-node lookups by implementing
> >> arch_match_cpu_phys_id().
> >> 
> >> This allows using of_get_cpu_node() and of_cpu_device_node_get() to look
> >> up CPU devicetree nodes and specifically makes sure that CPU struct
> >> devices are linked to the correct devicetree nodes.
> >> 
> >> Note that CPUs are described in devicetree using their APIC ids and
> >> that those do not generally coincide with the logical ids (e.g. used by
> >> the default arch_match_cpu_phys_id() implementation).
> 
> Can you please explain why this is relevant for any of the existing few
> x86 DT users?

Let me try to break it down.

> The above is incoherent word salad TBH:
> 
>     1) Fix .....
> 
>        without any information what is broken

CPU device-tree node lookups are broken as the subject and introductory
paragraph indicates.

>     2) This allows ...
> 
>        It allows. Cool. Is it mandatory, desired or whatever?

It is mandatory in that any arch with devicetree support which does not
have an identity mapping between physical and logical CPU ids need to
override the default implementation of arch_match_cpu_phys_id().

Failing to do so breaks the lookups done using the above mentioned
helpers and also results in in the CPU struct devices pointing to the
wrong devicetree node (e.g. exported through sysfs).

>     3) Note ...
> 
>        That might be an actual problem decsription, but I can't say for
>        sure.

Yeah, that is why this is needed on x86 and also explains why this isn't
an issue on a UP system where the physical and logical id of CPU0
coincides.

I agree that this is unnecessarily terse.

> I still have no idea what this 'Fix' is fixing if it is fixing anything
> at all. Is is a fix which needs to be backported or is it a 'fix' which
> fixes something which got changed elsewhere and forgotten.

The only in-tree devicetree source for x86 is for a UP system where CPU0
has APIC id 0 and the initial code didn't even fetch the APIC id from
devicetree (even if the CPU was described in DT).

Support for SMP was later added by added by commit 4e07db9c8db8
("x86/devicetree: Use CPU description from Device Tree") and since then
we need to provide an arch_match_cpu_phys_id() implementation.

I'll resend with a better explanation.

Johan