Re: [PATCH v2 1/2] devfreq: qcom: Add L2 Krait Cache devfreq scaling driver

[Sudeep Holla]

On Wed, Sep 30, 2020 at 01:56:01PM +0200, ansuelsmth@xxxxxxxxx wrote:
> > Subject: Re: [PATCH v2 1/2] devfreq: qcom: Add L2 Krait Cache devfreq
> > scaling driver
> >
> > On Tue, Sep 29, 2020 at 06:29:24PM +0200, Ansuel Smith wrote:
> > > Qcom L2 Krait CPUs use the generic cpufreq-dt driver and doesn't
> actually
> > > scale the Cache frequency when the CPU frequency is changed. This
> > > devfreq driver register with the cpu notifier and scale the Cache
> > > based on the max Freq across all core as the CPU cache is shared across
> > > all of them. If provided this also scale the voltage of the regulator
> > > attached to the CPU cache. The scaling logic is based on the CPU freq
> > > and the 3 scaling interval are set by the device dts.
> > >
> >
> > I have raised this concern before. I am worried this kind of independent
> > CPU and cache frequency controls make way for clkscrew kind of attacks.
> > Why can't the clocks be made parent/child or secondary and automatically
> > updated when CPU clocks are changed.
> >
>
> I don't think I understand this fully. Anyway about the clkscrew attack, the
> range are set on the dts so unless someone actually wants to have a
> vulnerable system, the range can't be changes at runtime. The devfreq
> governor is set to immutable and can't be changes AFAIK.
>

I don't think that matters, we are talking about Secure/Non-secure boundary
here. DT can be modified or simple a rogue devfreq module can do all the
bad things.

> About 'automatically updated when CPU changes', the cache is shared across 2
> core and they scale independently. We can be in situation where one cpu is
> at max and one at idle freq and the cache is set to idle. To fix this at
> every change the clk should find the max value and I think this would make
> all the clk scaling very slow.

This sounds like we are going back to coupled idle states kind of setup.
Sorry we don't want those anymore.

> If you have any suggestion on how I can implement this better, I'm
> more than happy to address them. For now, the lack of this kind of cache
> scale, make the system really slow since by default the init of the cpu and
> cache clks put them at the lowest frequency and nobody changes that.
> (we have cpufreq scaling support but the cache is never actually scaled)

As I mentioned, if this needs to be done in OSPM, then hide it in the clock
building right dependencies. Clk will even have refcount so that one idle
CPU won't bring the cache down to idle frequency.

--
Regards,
Sudeep