| 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
| ||
|
Message-ID: <c6132eec-3c32-8471-76ab-6ec432b7b3f1@huawei.com>
Date: Wed, 18 Oct 2017 09:10:22 +0800
From: Xiongfeng Wang <wangxiongfeng2@...wei.com>
To: Jeremy Linton <jeremy.linton@....com>,
Tomasz Nowicki <tn@...ihalf.com>, <linux-acpi@...r.kernel.org>
CC: <mark.rutland@....com>, <Jonathan.Zhang@...ium.com>,
<Jayachandran.Nair@...ium.com>, <lorenzo.pieralisi@....com>,
<catalin.marinas@....com>, <gregkh@...uxfoundation.org>,
<jhugo@...eaurora.org>, <rjw@...ysocki.net>,
<linux-pm@...r.kernel.org>, <will.deacon@....com>,
<linux-kernel@...r.kernel.org>, <ahs3@...hat.com>,
<viresh.kumar@...aro.org>, <hanjun.guo@...aro.org>,
<sudeep.holla@....com>, <austinwc@...eaurora.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table
parsing
Hi Jeremy,
On 2017/10/17 23: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:
> I disagree, that routine is shared by the two code paths because its functionality is 99% duplicated between the two. The difference being whether it terminates the search at a given level, or continues searching until it runs out of nodes. The latter case is simply a degenerate version of the first.
>
>
>>
>> Also, seems like we can merge acpi_parse_pptt() & acpi_process_node().
>
> That is true, but I fail to see how any of this is actually fixes anything. There are a million ways to do this, including as pointed out by building another data-structure to simplify the parsing what is a table that is less than ideal for runtime parsing (starting with the direction of the relative pointers, and ending with having to "infer" information that isn't directly flagged). I actually built a couple other versions of this, including a nice cute version which is about 1/8 this size of this and really easy to understand but of course is recursive...
>
>
Maybe you can see my version below. It is similar to what you said above. It may give some help.
https://github.com/fenghusthu/acpi_pptt
Thanks
Xiongfeng Wang
>>
>> Here are my suggestions:
>>
>>
>> static struct acpi_pptt_cache *acpi_pptt_cache_type_level(
>> struct acpi_table_header *table_hdr,
>> struct acpi_subtable_header *res,
>> int *local_level,
>> int level, int type)
>> {
>> struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>>
>> if (res->type != ACPI_PPTT_TYPE_CACHE)
>> return NULL;
>>
>> while (cache) {
>> if ((*local_level == level) &&
>> (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
>> ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 == type)) {
>>
>> pr_debug("Found cache @ level %d\n", level);
>> return cache;
>> }
>> cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache);
>> (*local_level)++;
>> }
>> return NULL;
>> }
>>
>> static struct acpi_pptt_cache *_acpi_find_cache_node(
>> struct acpi_table_header *table_hdr,
>> struct acpi_pptt_processor *cpu_node,
>> int *local_level, int level, int type)
>> {
>> struct acpi_subtable_header *res;
>> struct acpi_pptt_cache *cache_tmp, *cache = NULL;
>> int resource = 0;
>>
>> /* walk down from the processor node */
>> while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>>
>> cache_tmp = acpi_pptt_cache_type_level(table_hdr, res,
>> local_level, level, type);
>> if (cache_tmp) {
>> if (cache)
>> pr_err("Found duplicate cache level/type unable to determine uniqueness\n");
>>
>> cache = cache_tmp;
>> }
>> resource++;
>> }
>> return cache;
>> }
>>
>> /* find the ACPI node describing the cache type/level for the given CPU */
>> static struct acpi_pptt_cache *acpi_find_cache_node(
>> struct acpi_table_header *table_hdr, u32 acpi_cpu_id,
>> enum cache_type type, unsigned int level,
>> struct acpi_pptt_processor **node)
>> {
>> int total_levels = 0;
>> struct acpi_pptt_cache *found = NULL;
>> struct acpi_pptt_processor *cpu_node;
>> u8 acpi_type = acpi_cache_type(type);
>>
>> pr_debug("Looking for CPU %d's level %d cache type %d\n",
>> acpi_cpu_id, level, acpi_type);
>>
>> cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
>> if (!cpu_node)
>> return NULL;
>>
>> do {
>> found = _acpi_find_cache_node(table_hdr, cpu_node,
>> &total_levels, level, acpi_type);
>> *node = cpu_node;
>> cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
>> } while ((cpu_node) && (!found));
>>
>> return found;
>> }
>>
>> static int acpi_pptt_cache_level(struct acpi_table_header *table_hdr,
>> struct acpi_subtable_header *res)
>> {
>> struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res;
>> int local_level = 1;
>>
>> if (res->type != ACPI_PPTT_TYPE_CACHE)
>> return 0;
>>
>> while ((cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache)))
>> local_level++;
>> return local_level;
>> }
>>
>> static int _acpi_count_cache_level(
>> struct acpi_table_header *table_hdr,
>> struct acpi_pptt_processor *cpu_node)
>> {
>> struct acpi_subtable_header *res;
>> int levels = 0, resource = 0, number_of_levels = 0;
>>
>> /* walk down from the processor node */
>> while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
>> levels = acpi_pptt_cache_level(table_hdr, res);
>>
>> /*
>> * we are looking for the max depth. Since its potentially
>> * possible for a given node to have resources with differing
>> * depths verify that the depth we have found is the largest.
>> */
>> if (levels > number_of_levels)
>> number_of_levels = levels;
>>
>> resource++;
>> }
>> return number_of_levels;
>> }
>>
>> static int acpi_count_cache_level(struct acpi_table_header *table_hdr,
>> u32 acpi_cpu_id)
>> {
>> int total_levels = 0;
>> struct acpi_pptt_processor *cpu_node;
>>
>> cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
>> while (cpu_node) {
>> total_levels += _acpi_count_cache_level(table_hdr, cpu_node);
>> cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
>> }
>>
>> return total_levels;
>> }
>>
>>
>> Did not compile the code so I may have missed somthing.
>>
>> Thanks,
>> Tomasz
>
>
> .
>
Powered by blists - more mailing lists