Messages in this thread | | | Subject | Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing | From | Tomasz Nowicki <> | Date | Thu, 19 Oct 2017 07:18:43 +0200 |
| |
On 18.10.2017 19:30, Jeremy Linton wrote: > On 10/18/2017 05:24 AM, Tomasz Nowicki wrote: >> 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 <jeremy.linton@arm.com> >>>>>> --- >>>>>> 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 <linux/acpi.h> >>>>>> +#include <linux/cacheinfo.h> >>>>>> +#include <acpi/processor.h> >>>>>> + >>>>>> +/* >>>>>> + * 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. > > Ah, yah right... If you removed the shift per your original comment then > it breaks this. Yes, and the type definitions for cache type aren't > wrong in this version because the unified state has the 3rd bit set for > both the 0x3 and 0x2 values and its only used to covert from the linux > type to the ACPI type (and not back because we don't mess with whatever > the original "detection" was). I'm not really planning on changing that > because I don't think it helps "readability" (and it converts a compile > time constant to a runtime shift). > >>> >>>> >>>> >>>>> >>>>>> + 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? > > No, but its not deterministic unless we mark the node, which doesn't > solve the problem of a table constructed like > > L1i->L2 (unified) > L1d->L2 (unified) > > or various other structures which aren't disallowed by the spec and have > non-deterministic real world meanings, anymore than constructing the > table like: > > L1i > Lid->L2(unified) > > which I tend to prefer because with a structuring like that it can be > deterministic (and in a way actually represents the non-coherent > behavior of (most?) ARM64 core's i-caches, as could be argued the first > example if the allocation policies are varied between the L2 nodes). > > The really ugly bits here happen if you add another layer: > > L1i->L2i-L3 > L1d------^ > > which is why I made that an error message, not including the fact that > since the levels aren't tagged the numbering and meaning isn't clear. > > (the L1i in the above example might be better called an L0i to avoid > throwing off the reset of the hierarchy numbering, also so it could be > ignored). > > Summary: > > I'm not at all happy with this specification's attempt to leave out > pieces of information which make parsing things more deterministic. In > this case I'm happy to demote the message level, but not remove it > entirely but I do think the obvious case you list shouldn't be the > default one. > > Lastly: > > I'm assuming the final result is that the table is actually being parsed > correctly despite the ugly message?
Indeed, the ThunderX2 PPTT table is being parsed so that topology shown in lstopo and lscpu is correct.
Thanks, Tomasz
| |