[PATCH] psci: add debugfs for runtime instrumentation

On 19 June 2017 at 13:44, Mark Rutland [off-list ref] wrote:

On Fri, Jun 16, 2017 at 04:26:20PM +0200, Vincent Guittot wrote:

quoted

On 16 June 2017 at 13:47, Mark Rutland [off-list ref] wrote:

quoted

On Fri, Jun 16, 2017 at 10:23:40AM +0200, Vincent Guittot wrote:

quoted

Add debugfs to display runtime instrumentation results that can be collected
by PSCI firmware.

Could you elaborate on this please? e.g.

yes sorry. I should have given more details about it

quoted

* What is instrumented?

The ARM Trusted Firwware that implements PSCI, also has a Performance
Measurement Framework (PMF) that can be used to save some timetamps
when running psci firmware. On top of PMF, serveral services have been
implemented like the PSCI residency stats and the runtime
instrumentation service. The latter saves timestamp when a PSCI
service is entered and then leaved or when cache maintenance is
executed durign psci call. With these timestamps, we can measure the
duration of some part of code of the PSCI firmware.

Ok. So PMF is an ATF concept, and not a PSCI concept (as it's not
defined by the PSCI spec).

Yes

[...]

quoted

quoted

* On which platforms is this available, and when?

The code that save the timestamp is generic and as result available
for all platforms that implement PSCI. Nevertheless, each platfrom has
to enable the SMC service. At now, juno and hikey has enable the
service and hikey960 should some soon as well

Ok. So this is available on a subset of platforms using ATF.

The service is generic but not enable by default on platform

[...]

quoted

quoted

quoted

+config ARM_PSCI_INSTRUMENTATION
+     bool "ARM PSCI Run Time Instrumentation"
+     depends on ARM_PSCI_FW && CPU_IDLE && DEBUG_FS
+     help
+       Enable debugfs interface for PSCI FW in which you can retrieve
+       runtime instrumentation values of the PSCI firmware like time to
+       enter or leave a cpu suspend state or time to flush cache.

Looking at the latest published spec (ARM DEN 0022D), PSCI doesn't
provide such information, as there's only PSCI_STAT_{COUNT,RESIDENCY}.

This use the same Performance Measurement Framework as
PSCI_STAT_{COUNT,RESIDENCY}

Sure, but that's an ATF implementation detail, and not something defined
by the PSCI spec.

Yes

FWIW, I'm more than happy to add support for PSCI_STAT_{COUNT,RESIDENCY}
since these are in the spec.

quoted

quoted

What mechanisms are you using here?

More details can be founded here
https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/firmware-design.md#13--performance-measurement-framework

Thanks. This all sounds IMP DEF, with platforms able to provide their
own monitors.

quoted

quoted

As far as I can tell, these are not part of the PSCI standard, and I'm
not aware of any documentation.

I'm very much not keen on using IMPLEMENTATION DEFINED functionality in
the PSCI driver, as IMP DEF stuff always creates long-term maintenance
problems. You'll need to convince me that this is worth supporting.

I will try.

I have put it in psci.c as it is really about measurement latenccy of
psci firmware but it can probably be put somewhere else if it is more
relevant. The only link between current psci driver and this feature
is the call to psci_update_all_rt_instr() in psci_cpu_suspend_enter()
to make sure that we have coherent timestamp
Thinking a bit more about this, we can probably use the cpu_pm_exit

To be honest, the more I think about this, the less keen I am on having
it in the kernel.

It's all IMP DEF, and will need additional info in DT or ACPI to be safe
to invoke (even ignoring usable), and has subtle interactions with the
PSCI driver. It's of limited utility to a very small number of
developers.

[...]

quoted

quoted

quoted

+     /* Test if runtime instrumentation is enable */
+     if (!psci_rt_instr_enable()) {

This is an unconditional initcall, so this simply is not safe.

you're right, it should be called only if/when psci driver has been probed

Even where PSCI is present, I don't believe it's safe to invoke this.
It's a SiP call, which a vendor may have used for any other purpose.

In fact, these functions which are part of SiP, are defined in the ATF
https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/arm-sip-service.md

So at minimum, we'd need a description in the DT in order to know that
we can invoke this. Since it's all IMP DEF, we'd need all the monitors
described too, and at that point it's complex enough, and so tailored to
ATF's internals that I really don't want this in the kernel.

So I agree that linking this to PSCI firmware driver is not the best
choice as it is only related to ATF firmware and not  PSCI spec.
and this will fit best with a ATF SiP dedicated driver

Thanks

So sorry, but given all of the above, I have to NAK support for this ATF
feature.

If we can get some more useful performance monitors into the PSCI spec,
I am more than happy to support those.

Thanks,
Mark.