linux-kernel - Re: [PATCH v4 3/3] arm64: implement CPPC FFH support using AMUs

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: <20201113163712.GA5058@arm.com>
Date:   Fri, 13 Nov 2020 16:37:12 +0000
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     catalin.marinas@....com, mark.rutland@....com, will@...nel.org,
        morten.rasmussen@....com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/3] arm64: implement CPPC FFH support using AMUs

Hi Sudeep,

On Friday 13 Nov 2020 at 14:16:58 (+0000), Sudeep Holla wrote:
[..]
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index b8cb16e3a2cc..7c9b6a0ecd6a 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -147,6 +147,9 @@ void update_freq_counters_refs(void)
> >
> >  static inline bool freq_counters_valid(int cpu)
> >  {
> > +	if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask))
> > +		return false;
> > +
> >  	if (!cpu_has_amu_feat(cpu)) {
> >  		pr_debug("CPU%d: counters are not supported.\n", cpu);
> >  		return false;
> > @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void)
> >  	this_cpu_write(arch_core_cycles_prev, core_cnt);
> >  	this_cpu_write(arch_const_cycles_prev, const_cnt);
> >  }
> > +
> > +#ifdef CONFIG_ACPI_CPPC_LIB
> > +#include <acpi/cppc_acpi.h>
> 
> Not sure what arm64 maintainers prefer, but this code has nothing to do
> with topolopy strictly speaking. I wonder if we can put it in separate

Yes, you are correct. I am/was wondering the same for all the
counters/AMU related functions, but given they were only used for
topology_scale_freq_tick() *until now*, it was okay to keep them in
topology.c.

But I might soon have at least one additional (to FIE and FFH) small
usecase for them in the implementation of arch_freq_get_on_cpu(), so all
these functions might be better off in a separate file as well.

Side note: I don't think frequency invariance is strictly speaking
related to topology either. Nether are other functions in the
arch_topology driver. It's likely we got used to placing all
arch function implementation in either the arch_topology driver or the
<arch>/kernel/topology.c.

> file conditionally compiled if CONFIG_ACPI_CPPC_LIB is enabled there
> by eliminating #ifdef(main reason for raising this point).
> 

I'm happy to split either one(FFH) or both(FFH and counters) in separate
files. Given the above, let me know if/how you guys prefer this done.

> Either way:
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@....com>
> 

Thank you for the reviews,
Ionela.

> --
> Regards,
> Sudeep