linux-kernel - Re: [PATCH v2] arm64: topology: Setup AMU FIE for online CPUs only

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] [day] [month] [year] [list]
Message-ID: <0efd122b-221a-4928-8b8d-76821cfc4548@huawei.com>
Date: Sat, 2 Aug 2025 18:23:58 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Beata Michalska <beata.michalska@....com>
CC: <catalin.marinas@....com>, <will@...nel.org>, <sudeep.holla@....com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<linuxarm@...wei.com>, <jonathan.cameron@...wei.com>,
	<viresh.kumar@...aro.org>, <vincent.guittot@...aro.org>,
	<yangyicong@...ilicon.com>, <zhanjie9@...ilicon.com>, <lihuisong@...wei.com>,
	<yubowen8@...wei.com>, <linhongye@...artners.com>
Subject: Re: [PATCH v2] arm64: topology: Setup AMU FIE for online CPUs only

On 2025/7/31 16:44, Beata Michalska wrote:

> Hi Lifeng,
> 
> Apologies for late reply.
> 
> On Fri, Jul 25, 2025 at 06:28:13PM +0800, Lifeng Zheng wrote:
>> When boot with maxcpu=1 restrict, and LPI(Low Power Idle States) is on,
>> only CPU0 will go online. The support AMU flag of CPU0 will be set but the
>> flags of other CPUs will not. This will cause AMU FIE set up fail for CPU0
>> when it shares a cpufreq policy with other CPU(s). After that, when other
>> CPUs are finally online and the support AMU flags of them are set, they'll
>> never have a chance to set up AMU FIE, even though they're eligible.
>>
>> To solve this problem, the process of setting up AMU FIE needs to be
>> modified as follows:
>>
>> 1. Set up AMU FIE only for the online CPUs.
>>
>> 2. Try to set up AMU FIE each time a CPU goes online and do the
>> freq_counters_valid() check for all the online CPUs share the same policy.
>> If this check fails, clear scale freq source of these CPUs, in case they
>> use different source of the freq scale.
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
>> ---
>>  arch/arm64/kernel/topology.c | 49 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 5d07ee85bdae..d578c496d457 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -357,12 +357,15 @@ static void amu_fie_setup(const struct cpumask *cpus)
>>  
>>  	/* We are already set since the last insmod of cpufreq driver */
>>  	if (cpumask_available(amu_fie_cpus) &&
>> -	    unlikely(cpumask_subset(cpus, amu_fie_cpus)))
>> +	    cpumask_subset(cpus, amu_fie_cpus))
>>  		return;
>>  
>> -	for_each_cpu(cpu, cpus)
>> -		if (!freq_counters_valid(cpu))
>> +	for_each_cpu(cpu, cpus) {
>> +		if (!freq_counters_valid(cpu)) {
>> +			topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_ARCH, cpus);
>>  			return;
>> +		}
>> +	}
>>  
>>  	if (!cpumask_available(amu_fie_cpus) &&
>>  	    !zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL)) {
>> @@ -385,7 +388,7 @@ static int init_amu_fie_callback(struct notifier_block *nb, unsigned long val,
>>  	struct cpufreq_policy *policy = data;
>>  
>>  	if (val == CPUFREQ_CREATE_POLICY)
>> -		amu_fie_setup(policy->related_cpus);
>> +		amu_fie_setup(policy->cpus);
> Right, so this will only work for the AMU side of things. The cpufreq core still
> considers `related_cpus` which should be aligned (if possible).

'cpus' means the online CPUs relate to this policy, and 'related_cpus'
means all the CPUs relate to this policy. Use 'cpus' here because AMU FIE
only needs to be set for the online CPUs. As for the offline CPUs, they
don't have to set up AMU FIE, and shouldn't affect other CPUs setting up
AMU FIE.

>>  
>>  	/*
>>  	 * We don't need to handle CPUFREQ_REMOVE_POLICY event as the AMU
>> @@ -404,10 +407,46 @@ static struct notifier_block init_amu_fie_notifier = {
>>  	.notifier_call = init_amu_fie_callback,
>>  };
>>  
>> +static int cpuhp_topology_online(unsigned int cpu)
>> +{
>> +	struct cpufreq_policy *policy __free(put_cpufreq_policy);
>> +	cpumask_var_t cpus_to_set;
>> +
>> +	if (!zalloc_cpumask_var(&cpus_to_set, GFP_KERNEL))
>> +		return -ENOMEM;
>> +
>> +	cpumask_copy(cpus_to_set, cpumask_of(cpu));
>> +
>> +	policy = cpufreq_cpu_get(cpu);
>> +	if (policy) {
>> +		cpumask_or(cpus_to_set, cpus_to_set, policy->cpus);
> This should be available via `amu_fie_cpus` mask (as of subset).
> So it should be enough to test only the CPU at hand?

Yeah, makes sense. Thanks.

> Additionally there is no tracking whether that CPU hasn't been verified already.

I don't think it's necessary. If a CPU has been verified already and pass,
then the 'amu_fie_cpus' mask should contain it, otherwise it deserves to be
checked again.

>> +		amu_fie_setup(cpus_to_set);
>> +	}
>> +
>> +	free_cpumask_var(cpus_to_set);
>> +	return 0;
>> +}
>> +
>>  static int __init init_amu_fie(void)
>>  {
>> -	return cpufreq_register_notifier(&init_amu_fie_notifier,
>> +	int ret;
>> +
>> +	ret = cpufreq_register_notifier(&init_amu_fie_notifier,
>>  					CPUFREQ_POLICY_NOTIFIER);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +					"arm64/topology:online",
>> +					cpuhp_topology_online,
>> +					NULL);
> We should check whether we really have to set it up, as of all CPUs are already
> online).

All CPUs are likely to be online at this point. However, we cannot
guarantee that this is still the case when the cpufreq policies are
created.

>> +	if (ret < 0) {
>> +		cpufreq_unregister_notifier(&init_amu_fie_notifier,
>> +					    CPUFREQ_POLICY_NOTIFIER);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  core_initcall(init_amu_fie);
>>  
>> -- 
>> 2.33.0
>>
>