Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d74ee654-287a-a065-71ec-577957d86bf7@arm.com>
Date:   Thu, 13 Apr 2023 12:17:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v3 4/4] cacheinfo: Add use_arch[|_cache]_info
 field/function
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     linux-kernel@vger.kernel.org, Radu Rendec <rrendec@redhat.com>,
        Alexandre Ghiti <alexghiti@rivosinc.com>,
        Conor Dooley <conor.dooley@microchip.com>,
        Will Deacon <will@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Gavin Shan <gshan@redhat.com>
References: <20230413091436.230134-1-pierre.gondois@arm.com>
 <20230413091436.230134-5-pierre.gondois@arm.com>
 <20230413094952.ncdldvv2ysjqwbsv@bogus>
Content-Language: en-US
From:   Pierre Gondois <pierre.gondois@arm.com>
In-Reply-To: <20230413094952.ncdldvv2ysjqwbsv@bogus>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/13/23 11:49, Sudeep Holla wrote:
> On Thu, Apr 13, 2023 at 11:14:34AM +0200, Pierre Gondois wrote:
>> The cache information can be extracted from either a Device
>> Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
>> for arm64).
>>
>> The clidr_el1 register is used only if DT/ACPI information is not
>> available. It does not states how caches are shared among CPUs.
>>
>> Add a use_arch_cache_info field/function to identify when the
>> DT/ACPI doesn't provide cache information. Use this information
>> to assume L1 caches are privates and L2 and higher are shared among
>> all CPUs.
>>
> 
> I have tentatively merged first 3 patches along with Radu's series(waiting
> for build tests still before confirming). I am not yet sure on this.
> 
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>   drivers/base/cacheinfo.c  | 13 ++++++++++++-
>>   include/linux/cacheinfo.h | 10 ++++++++++
>>   2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index 06de9a468958..49dbb4357911 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -40,7 +40,8 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>   	 * For non DT/ACPI systems, assume unique level 1 caches,
>>   	 * system-wide shared caches for all other levels.
>>   	 */
>> -	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
>> +	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
>> +	    this_leaf->use_arch_info)
> 
> Can't we just use use_arch_cache_info() here ?

I think that if we use use_arch_cache_info() here, then arm64  platforms
will always return here and never check fw_token/this_leaf->id values.
Indeed, we also need to know that no cache information is available in
DT/ACPI, cf. [1]

> 
>>   		return (this_leaf->level != 1) && (sib_leaf->level != 1);
>>   
>>   	if ((sib_leaf->attributes & CACHE_ID) &&
>> @@ -349,6 +350,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>>   	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>   	struct cacheinfo *this_leaf, *sib_leaf;
>>   	unsigned int index, sib_index;
>> +	bool use_arch_info = false;
>>   	int ret = 0;
>>   
>>   	if (this_cpu_ci->cpu_map_populated)
>> @@ -361,6 +363,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>>   	 */
>>   	if (!last_level_cache_is_valid(cpu)) {
>>   		ret = cache_setup_properties(cpu);
>> +		if (ret && use_arch_cache_info()) {
>> +			// Possibility to rely on arch specific information.

[1]

>> +			use_arch_info = true;
>> +			ret = 0;
>> +		}
>> +
>>   		if (ret)
>>   			return ret;
>>   	}
>> @@ -370,6 +378,9 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>>   
>>   		this_leaf = per_cpu_cacheinfo_idx(cpu, index);
>>   
>> +		if (use_arch_info)
>> +			this_leaf->use_arch_info = true;
>> +
>>   		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
>>   		for_each_online_cpu(i) {
>>   			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);
>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>> index 908e19d17f49..fed675b251a2 100644
>> --- a/include/linux/cacheinfo.h
>> +++ b/include/linux/cacheinfo.h
>> @@ -66,6 +66,7 @@ struct cacheinfo {
>>   #define CACHE_ALLOCATE_POLICY_MASK	\
>>   	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>   #define CACHE_ID		BIT(4)
>> +	bool use_arch_info;
> 
> Do you see the need to stash this value as it is either globally true or
> false based on the arch ?

A static variable could be used instead and set to true if we fail to fetch the
cache information from DT/ACPI, cf. [1]. The only possible transition for this
variable would be from false->true. I'll check if this works like this.

> 
>>   	void *fw_token;
>>   	bool disable_sysfs;
>>   	void *priv;
>> @@ -129,4 +130,13 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
>>   	return -1;
>>   }
>>   
>> +static inline bool use_arch_cache_info(void)
>> +{
>> +#if defined(CONFIG_ARM64)
>> +	return true;
>> +#else
>> +	return false;
>> +#endif
>> +}
>> +
> 
> Can we just have it as:
> #ifdef CONFIG_ARM64
> #define use_arch_cache_info()	(true)
> #else
> #define use_arch_cache_info()	(false)
> #endif

Yes sure, I'll post a v4 with this along Conor's requested change.

> 
>>   #endif /* _LINUX_CACHEINFO_H */
>> -- 
>> 2.25.1
>>
>