linux-kernel - Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b5dc310d-ca95-fd08-5b2e-8bbaeba19533@arm.com>
Date:   Thu, 19 Oct 2017 09:24:12 -0500
From:   Jeremy Linton <jeremy.linton@....com>
To:     Tomasz Nowicki <tnowicki@...iumnetworks.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

Hi,


On 10/19/2017 12:18 AM, Tomasz Nowicki wrote:
> 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:
>>>> On 17.10.2017 17:22, Jeremy Linton wrote:
>>>>> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:
>>>>>> On 12.10.2017 21:48, Jeremy Linton wrote:
(trimming)
>>>>>>> +            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.

Great.

Also, I think this is a better change:

      if ((*found != NULL) && (*found != cache))
          pr_err("Found duplicate cache level/type unable to determine 
uniqueness\n");

Which if its a duplicate node/type at the given level the message is 
just suppressed. It will of course still trigger in cases like:

L1d->L2
l1i->L2

or other odd cases.