Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 22 Mar 2019 16:54:50 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Pu Wen <puwen@hygon.cn>
Cc:     tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
        x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] x86/cpu/hygon: Fix phys_proc_id calculation logic
 for multi-die processor
Message-ID: <20190322155449.GE12472@zn.tnic>
References: <1553251380-7015-1-git-send-email-puwen@hygon.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1553251380-7015-1-git-send-email-puwen@hygon.cn>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 22, 2019 at 06:43:00PM +0800, Pu Wen wrote:
> For Hygon family 18h multi-die processor platform, which support 1-Die/
> 2-Die/4-Die per socket, the system view is shown in the following system
> topology.
> 
> System View (with 1-Die 2-Socket):
>            |------------|
>          ------       -----
>  SOCKET0 | D0 |       | D1 |  SOCKET1
>          ------       -----
> 
> System View (with 2-Die 2-socket):
>            --------------------
>            |     -------------|------
>            |     |            |     |
>          ------------       ------------
>  SOCKET0 | D1 -- D0 |       | D3 -- D2 | SOCKET1
>          ------------       ------------
> 
> System View (with 4-Die 2-Socket) :
>            --------------------
>            |     -------------|------
>            |     |            |     |
>          ------------       ------------
>          | D1 -- D0 |       | D7 -- D6 |
>          | |  \/ |  |       | |  \/ |  |
>  SOCKET0 | |  /\ |  |       | |  /\ |  | SOCKET1
>          | D2 -- D3 |       | D4 -- D5 |
>          ------------       ------------
>            |     |            |     |
>            ------|------------|     |
>                  --------------------
> 
> Current codes direct use "phys_proc_id = initial_apicid >> bits", which

Use proper english please - there's no "codes"

> calc phys_proc_id from initial_apicid by shifting *bits*, will get

... or "calc"

> wrong phys_proc_id for Hygon family 18h 1-Die/2-Die 2-socket system.
> 
> According to document [1] section 2.1.11.1, the *bits* is the value of
> CPUID_Fn80000008_ECX[12:15]. The value may be 4, 5 or 6. The definitions
> are: 4 - 1 die, 5 - 2 die, 6 - 3/4 die.
> 
> ApicId definition is as below (see section 2.1.10.2.1.3 of [1]):
>              -------------------------------------------------
>          Bit |     6     |   5  4  |    3   |    2   1   0   |
>              |-----------|---------|--------|----------------|
>          IDs | Socket ID | Node ID | CCX ID | Core/Thread ID |
>              -------------------------------------------------
> 
> So for 3/4-Die CPU the *bits* is 6, which is same to ApicID definition
> field, which still can get correct socket ID for 2-socket system. But
> for 1-Die or 2-Die CPU the *bits* is 4 or 5, which will cause the right
> shifted result is not exactly the value of socket ID.
> 
> For Hygon family 18h CPU the socket ID should be obtained from ApicId[6].
> To fix the problem and match ApicID field definition, adjust the shift
> bits to 6 for all Hygon family 18h multi-die CPUs.

Sounds to me like you're programming the initial APIC ID not
the same way as AMD do...

> Reference:
> [1] https://www.amd.com/system/files/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf
> 
> Signed-off-by: Pu Wen <puwen@hygon.cn>
> ---
>  arch/x86/kernel/cpu/hygon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
> index cf25405..2df6dd9 100644
> --- a/arch/x86/kernel/cpu/hygon.c
> +++ b/arch/x86/kernel/cpu/hygon.c
> @@ -87,6 +87,9 @@ static void hygon_get_topology(struct cpuinfo_x86 *c)
>  		if (!err)
>  			c->x86_coreid_bits = get_count_order(c->x86_max_cores);
>  
> +		/* Socket ID is ApicId[6] for these processors. */
> +		c->phys_proc_id = c->apicid >> 6;

That 6 is a magic number, I assume?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.