From mboxrd@z Thu Jan 1 00:00:00 1970 From: tnowicki@caviumnetworks.com (Tomasz Nowicki) Date: Wed, 18 Oct 2017 12:24:35 +0200 Subject: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing In-Reply-To: <4ee4af4d-d987-33f9-4d55-67bfede3ebfa@caviumnetworks.com> References: <20171012194856.13844-1-jeremy.linton@arm.com> <20171012194856.13844-2-jeremy.linton@arm.com> <38c63cfd-4c13-9461-0d4a-74b0943be0fd@semihalf.com> <2ce92b90-fc2c-3fff-a3e3-b9517057ab2c@arm.com> <4ee4af4d-d987-33f9-4d55-67bfede3ebfa@caviumnetworks.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 18.10.2017 07:39, Tomasz Nowicki wrote: > Hi, > > On 17.10.2017 17:22, Jeremy Linton wrote: >> Hi, >> >> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote: >>> Hi Jeremy, >>> >>> I did second round of review and have some more comments, please see >>> below: >>> >>> On 12.10.2017 21:48, Jeremy Linton wrote: >>>> ACPI 6.2 adds a new table, which describes how processing units >>>> are related to each other in tree like fashion. Caches are >>>> also sprinkled throughout the tree and describe the properties >>>> of the caches in relation to other caches and processing units. >>>> >>>> Add the code to parse the cache hierarchy and report the total >>>> number of levels of cache for a given core using >>>> acpi_find_last_cache_level() as well as fill out the individual >>>> cores cache information with cache_setup_acpi() once the >>>> cpu_cacheinfo structure has been populated by the arch specific >>>> code. >>>> >>>> Further, report peers in the topology using setup_acpi_cpu_topology() >>>> to report a unique ID for each processing unit at a given level >>>> in the tree. These unique id's can then be used to match related >>>> processing units which exist as threads, COD (clusters >>>> on die), within a given package, etc. >>>> >>>> Signed-off-by: Jeremy Linton >>>> --- >>>> ? drivers/acpi/pptt.c | 485 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> ? 1 file changed, 485 insertions(+) >>>> ? create mode 100644 drivers/acpi/pptt.c >>>> >>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >>>> new file mode 100644 >>>> index 000000000000..c86715fed4a7 >>>> --- /dev/null >>>> +++ b/drivers/acpi/pptt.c >>>> @@ -0,1 +1,485 @@ >>>> +/* >>>> + * Copyright (C) 2017, ARM >>>> + * >>>> + * This program is free software; you can redistribute it and/or >>>> modify it >>>> + * under the terms and conditions of the GNU General Public License, >>>> + * version 2, as published by the Free Software Foundation. >>>> + * >>>> + * This program is distributed in the hope it will be useful, but >>>> WITHOUT >>>> + * ANY WARRANTY; without even the implied warranty of >>>> MERCHANTABILITY or >>>> + * FITNESS FOR A PARTICULAR PURPOSE.? See the GNU General Public >>>> License for >>>> + * more details. >>>> + * >>>> + * This file implements parsing of Processor Properties Topology >>>> Table (PPTT) >>>> + * which is optionally used to describe the processor and cache >>>> topology. >>>> + * Due to the relative pointers used throughout the table, this >>>> doesn't >>>> + * leverage the existing subtable parsing in the kernel. >>>> + */ >>>> +#define pr_fmt(fmt) "ACPI PPTT: " fmt >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* >>>> + * Given the PPTT table, find and verify that the subtable entry >>>> + * is located within the table >>>> + */ >>>> +static struct acpi_subtable_header *fetch_pptt_subtable( >>>> +??? struct acpi_table_header *table_hdr, u32 pptt_ref) >>>> +{ >>>> +??? struct acpi_subtable_header *entry; >>>> + >>>> +??? /* there isn't a subtable at reference 0 */ >>>> +??? if (!pptt_ref) >>>> +??????? return NULL; >>>> + >>>> +??? if (pptt_ref + sizeof(struct acpi_subtable_header) > >>>> table_hdr->length) >>>> +??????? return NULL; >>>> + >>>> +??? entry = (struct acpi_subtable_header *)((u8 *)table_hdr + >>>> pptt_ref); >>>> + >>>> +??? if (pptt_ref + entry->length > table_hdr->length) >>>> +??????? return NULL; >>>> + >>>> +??? return entry; >>>> +} >>>> + >>>> +static struct acpi_pptt_processor *fetch_pptt_node( >>>> +??? struct acpi_table_header *table_hdr, u32 pptt_ref) >>>> +{ >>>> +??? return (struct acpi_pptt_processor >>>> *)fetch_pptt_subtable(table_hdr, pptt_ref); >>>> +} >>>> + >>>> +static struct acpi_pptt_cache *fetch_pptt_cache( >>>> +??? struct acpi_table_header *table_hdr, u32 pptt_ref) >>>> +{ >>>> +??? return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, >>>> pptt_ref); >>>> +} >>>> + >>>> +static struct acpi_subtable_header *acpi_get_pptt_resource( >>>> +??? struct acpi_table_header *table_hdr, >>>> +??? struct acpi_pptt_processor *node, int resource) >>>> +{ >>>> +??? u32 ref; >>>> + >>>> +??? if (resource >= node->number_of_priv_resources) >>>> +??????? return NULL; >>>> + >>>> +??? ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) + >>>> +????????????? sizeof(u32) * resource); >>>> + >>>> +??? return fetch_pptt_subtable(table_hdr, ref); >>>> +} >>>> + >>>> +/* >>>> + * given a pptt resource, verify that it is a cache node, then walk >>>> + * down each level of caches, counting how many levels are found >>>> + * as well as checking the cache type (icache, dcache, unified). If a >>>> + * level & type match, then we set found, and continue the search. >>>> + * Once the entire cache branch has been walked return its max >>>> + * depth. >>>> + */ >>>> +static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr, >>>> +??????????????? int local_level, >>>> +??????????????? struct acpi_subtable_header *res, >>>> +??????????????? struct acpi_pptt_cache **found, >>>> +??????????????? int level, int type) >>>> +{ >>>> +??? struct acpi_pptt_cache *cache; >>>> + >>>> +??? if (res->type != ACPI_PPTT_TYPE_CACHE) >>>> +??????? return 0; >>>> + >>>> +??? cache = (struct acpi_pptt_cache *) res; >>>> +??? while (cache) { >>>> +??????? local_level++; >>>> + >>>> +??????? if ((local_level == level) && >>>> +??????????? (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) && >>>> +??????????? ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == >>>> type)) { >>> >>> Attributes have to be shifted: >>> >>> (cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 >> >> Hmmm, I'm not sure that is true, the top level function in this >> routine convert the "linux" constant to the ACPI version of that >> constant. In that case the "type" field is pre-shifted, so that it >> matches the result of just anding against the field... That is unless >> I messed something up, which I don't see at the moment (and the code >> of course has been tested with PPTT's from multiple people at this >> point). > > For ThunderX2 I got lots of errors in dmesg: > Found duplicate cache level/type unable to determine uniqueness > > So I fixed "type" macros definitions (without shifting) and shift it > here which fixes the issue. As you said, it can be pre-shifted as well. > >> >> >>> >>>> +??????????? if (*found != NULL) >>>> +??????????????? pr_err("Found duplicate cache level/type unable to >>>> determine uniqueness\n"); Actually I still see this error messages in my dmesg. It is because the following ThunderX2 per-core L1 and L2 cache hierarchy: Core ------------------ | | | L1i ----- | | | | | ----L2 | | | | | L1d ----- | | | ------------------ In this case we have two paths which lead to L2 cache and hit above case. Is it really error case? Thanks, Tomasz