lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 18 Oct 2017 12:24:35 +0200 From: Tomasz Nowicki <tnowicki@...iumnetworks.com> To: Tomasz Nowicki <tnowicki@...iumnetworks.com>, Jeremy Linton <jeremy.linton@....com>, linux-acpi@...r.kernel.org Cc: mark.rutland@....com, Jonathan.Zhang@...ium.com, Jayachandran.Nair@...ium.com, lorenzo.pieralisi@....com, austinwc@...eaurora.org, linux-pm@...r.kernel.org, jhugo@...eaurora.org, gregkh@...uxfoundation.org, sudeep.holla@....com, rjw@...ysocki.net, linux-kernel@...r.kernel.org, will.deacon@....com, wangxiongfeng2@...wei.com, viresh.kumar@...aro.org, hanjun.guo@...aro.org, catalin.marinas@....com, ahs3@...hat.com, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing 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@....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. > >> >> >>> >>>> + 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
Powered by blists - more mailing lists