linux-kernel - Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs

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: <dd475eb6-7a0b-fbb4-316b-1af94c238699@huawei.com>
Date:   Mon, 27 Mar 2023 14:57:07 +0800
From:   Yicong Yang <yangyicong@...wei.com>
To:     Sudeep Holla <sudeep.holla@....com>,
        Pierre Gondois <pierre.gondois@....com>
CC:     <yangyicong@...ilicon.com>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <palmer@...osinc.com>,
        <linux-kernel@...r.kernel.org>, <prime.zeng@...ilicon.com>,
        <linuxarm@...wei.com>
Subject: Re: [PATCH] cacheinfo: Fix LLC is not exported through sysfs

Hi Pierre and Sudeep,

On 2023/3/24 19:35, Sudeep Holla wrote:
> On Thu, Mar 23, 2023 at 06:58:53PM +0100, Pierre Gondois wrote:
>> Hello Yicong,
>>
>> FWIW, I think the patch is correct and I could reproduce the issue.
>>
>> On 3/23/23 13:25, Yicong Yang wrote:
>>> From: Yicong Yang <yangyicong@...ilicon.com>
>>>
>>> After entering 6.3-rc1 the LLC cacheinfo is not exported on our ACPI
>>> based arm64 server. This is because the LLC cacheinfo is partly reset
>>> when secondary CPUs boot up. On arm64 the primary cpu will allocate
>>> and setup cacheinfo:
>>> init_cpu_topology()
>>>    for_each_possible_cpu()
>>>      fetch_cache_info() // Allocate cacheinfo and init levels
>>> detect_cache_attributes()
>>>    cache_shared_cpu_map_setup()
>>>      if (!last_level_cache_is_valid()) // not valid, setup LLC
>>>        cache_setup_properties() // setup LLC
>>>
>>> On secondary CPU boot up:
>>> detect_cache_attributes()
>>>    populate_cache_leaves()
>>>      get_cache_type() // Get cache type from clidr_el1,
>>>                       // for LLC type=CACHE_TYPE_NOCACHE
>>>    cache_shared_cpu_map_setup()
>>>      if (!last_level_cache_is_valid()) // Valid and won't go to this branch,
>>>                                        // leave LLC's type=CACHE_TYPE_NOCACHE
>>>
>>> The last_level_cache_is_valid() use cacheinfo->{attributes, fw_token} to
>>> test it's valid or not, but populate_cache_leaves() will only reset
>>> LLC's type, so we won't try to re-setup LLC's type and leave it
>>> CACHE_TYPE_NOCACHE and won't export it through sysfs.
>>>
> 
> IIUC this is for the case where arch register doesn't report the system level
> cache. I wonder if it makes sense to fix the arch callback to deal with that
> instead of here. I am fine either way, just checking as ideally it is
> something populate_cache_leaves() is messing up.
> 

yes it's right, the LLC information is not provided by the CPU register and can
only be retrieved from PPTT on my machine. Maybe fix the issue first, I don't
know how to make arch callback handle this since arch_topology is also used
other than arm64 which I'm not familiar with.

> [...]
> 
>>> @@ -481,6 +488,7 @@ int detect_cache_attributes(unsigned int cpu)
>>>   	if (ret)
>>>   		goto free_ci;
>>> +update_cpu_map:
>>
>> Maybe just a suggestion about the code itself,
>> it should be possible to replace the 'goto' by an 'if' condition.
>> (Similarly, the 'populate_leaves:' label could have been avoided.)
>>
> 
> Agreed, I prefer that as well.
> 

ok, will modify as below with a little refactor to get rid of the
'populate_leaves:' label as suggested.

Thanks,
Yicong

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index f6573c335f4c..e34e6b77e81a 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -462,24 +462,28 @@ int detect_cache_attributes(unsigned int cpu)
         * as it will happen only once (the cacheinfo memory is never freed).
         * Just populate the cacheinfo.
         */
-       if (per_cpu_cacheinfo(cpu))
-               goto populate_leaves;
-
-       if (init_cache_level(cpu) || !cache_leaves(cpu))
-               return -ENOENT;
+       if (!per_cpu_cacheinfo(cpu)) {
+               if (init_cache_level(cpu) || !cache_leaves(cpu))
+                       return -ENOENT;

-       ret = allocate_cache_info(cpu);
-       if (ret)
-               return ret;
+               ret = allocate_cache_info(cpu);
+               if (ret)
+                       return ret;
+       }

-populate_leaves:
        /*
-        * populate_cache_leaves() may completely setup the cache leaves and
-        * shared_cpu_map or it may leave it partially setup.
+        * If LLC is valid the cache leaves were already populated so just go to
+        * update the cpu map.
         */
-       ret = populate_cache_leaves(cpu);
-       if (ret)
-               goto free_ci;
+       if (!last_level_cache_is_valid(cpu)) {
+               /*
+                * populate_cache_leaves() may completely setup the cache leaves and
+                * shared_cpu_map or it may leave it partially setup.
+                */
+               ret = populate_cache_leaves(cpu);
+               if (ret)
+                       goto free_ci;
+       }

        /*
         * For systems using DT for cache hierarchy, fw_token