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;
Subject: Re: [PATCH v6 11/12] arm64: topology: enable ACPI/PPTT based CPU
 topology
To:     Jeremy Linton <jeremy.linton@arm.com>, <linux-acpi@vger.kernel.org>
References: <20180113005920.28658-1-jeremy.linton@arm.com>
 <20180113005920.28658-12-jeremy.linton@arm.com>
CC:     <linux-arm-kernel@lists.infradead.org>, <sudeep.holla@arm.com>,
        <hanjun.guo@linaro.org>, <lorenzo.pieralisi@arm.com>,
        <rjw@rjwysocki.net>, <will.deacon@arm.com>,
        <catalin.marinas@arm.com>, <gregkh@linuxfoundation.org>,
        <viresh.kumar@linaro.org>, <mark.rutland@arm.com>,
        <linux-kernel@vger.kernel.org>, <linux-pm@vger.kernel.org>,
        <jhugo@codeaurora.org>, <Jonathan.Zhang@cavium.com>,
        <ahs3@redhat.com>, <Jayachandran.Nair@cavium.com>,
        <austinwc@codeaurora.org>, <lenb@kernel.org>,
        <vkilari@codeaurora.org>, <morten.rasmussen@arm.com>,
        Juri Lelli <juri.lelli@arm.com>
From:   Xiongfeng Wang <wangxiongfeng2@huawei.com>
Message-ID: <928cb0c9-1d7a-3d0c-0538-a8bb0e2a86b1@huawei.com>
Date:   Thu, 25 Jan 2018 20:15:21 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <20180113005920.28658-12-jeremy.linton@arm.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jeremy,

I have tested the patch with the newest UEFI. It prints the below error:

[    4.017371] BUG: arch topology borken
[    4.021069] BUG: arch topology borken
[    4.024764] BUG: arch topology borken
[    4.028460] BUG: arch topology borken
[    4.032153] BUG: arch topology borken
[    4.035849] BUG: arch topology borken
[    4.039543] BUG: arch topology borken
[    4.043239] BUG: arch topology borken
[    4.046932] BUG: arch topology borken
[    4.050629] BUG: arch topology borken
[    4.054322] BUG: arch topology borken

I checked the code and found that the newest UEFI set PPTT physical_package_flag on a physical package node and
the NUMA domain (SRAT domains) starts from the layer of DIE. (The topology of our board is core->cluster->die->package).

When the kernel starts to build sched_domain, the multi-core sched_domain contains all the cores within a package,
and the lowest NUMA sched_domain contains all the cores within a die. But the kernel requires that the multi-core
sched_domain should be a subset of the lowest NUMA sched_domain, so the BUG info is printed.

If we modify the UEFI to make NUMA sched_domain start from the layer of package, then all the topology information
within the package will be discarded. I think we need to build the multi-core sched_domain using the cores within
the cluster instead of the cores within the package. I think that's what 'multi-core' means. Multi cores form a cluster. I guess.
If we build the multi-core sched_domain using the cores within a cluster, I think we need to add fields in struct cpu_topology
to record which cores are in each cluster.


Thanks,
Xiongfeng

On 2018/1/13 8:59, Jeremy Linton wrote:
> Propagate the topology information from the PPTT tree to the
> cpu_topology array. We can get the thread id, core_id and
> cluster_id by assuming certain levels of the PPTT tree correspond
> to those concepts. The package_id is flagged in the tree and can be
> found by calling find_acpi_cpu_topology_package() which terminates
> its search when it finds an ACPI node flagged as the physical
> package. If the tree doesn't contain enough levels to represent
> all of the requested levels then the root node will be returned
> for all subsequent levels.
> 
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/kernel/topology.c | 46 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 7b06e263fdd1..ce8ec7fd6b32 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -11,6 +11,7 @@
>   * for more details.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/arch_topology.h>
>  #include <linux/cpu.h>
>  #include <linux/cpumask.h>
> @@ -22,6 +23,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/topology.h>
>  #include <linux/slab.h>
> +#include <linux/smp.h>
>  #include <linux/string.h>
>  
>  #include <asm/cpu.h>
> @@ -300,6 +302,46 @@ static void __init reset_cpu_topology(void)
>  	}
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Propagate the topology information of the processor_topology_node tree to the
> + * cpu_topology array.
> + */
> +static int __init parse_acpi_topology(void)
> +{
> +	bool is_threaded;
> +	int cpu, topology_id;
> +
> +	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
> +
> +	for_each_possible_cpu(cpu) {
> +		topology_id = find_acpi_cpu_topology(cpu, 0);
> +		if (topology_id < 0)
> +			return topology_id;
> +
> +		if (is_threaded) {
> +			cpu_topology[cpu].thread_id = topology_id;
> +			topology_id = find_acpi_cpu_topology(cpu, 1);
> +			cpu_topology[cpu].core_id   = topology_id;
> +			topology_id = find_acpi_cpu_topology_package(cpu);
> +			cpu_topology[cpu].package_id = topology_id;
> +		} else {
> +			cpu_topology[cpu].thread_id  = -1;
> +			cpu_topology[cpu].core_id    = topology_id;
> +			topology_id = find_acpi_cpu_topology_package(cpu);
> +			cpu_topology[cpu].package_id = topology_id;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +#else
> +static inline int __init parse_acpi_topology(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
>  
>  void __init init_cpu_topology(void)
>  {
> @@ -309,6 +351,8 @@ void __init init_cpu_topology(void)
>  	 * Discard anything that was parsed if we hit an error so we
>  	 * don't use partial information.
>  	 */
> -	if (of_have_populated_dt() && parse_dt_topology())
> +	if ((!acpi_disabled) && parse_acpi_topology())
> +		reset_cpu_topology();
> +	else if (of_have_populated_dt() && parse_dt_topology())
>  		reset_cpu_topology();
>  }
>