Re: [PATCH v3 4/4] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois <pierre.gondois@arm.com>]

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
>>
>