[PATCH 0/1] arm64: defconfig: Add Coresight as module

[PATCH 0/1] arm64: defconfig: Add Coresight as module

[James Clark]

As suggested by Catalin here's the change to add Coresight to defconfig.

Unfortunately I don't think we should add CONFIG_CORESIGHT_SOURCE_ETM4X
which builds a few files until [1] is merged because of the overhead
of CONFIG_PID_IN_CONTEXTIDR.

[1]: https://lore.kernel.org/lkml/20211021134530.206216-1-leo.yan@linaro.org/T/

James Clark (1):
  arm64: defconfig: Add Coresight as module

 arch/arm64/configs/defconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.28.0

[PATCH 1/1] arm64: defconfig: Add Coresight as module

[James Clark]

Add Coresight to defconfig so that build errors are caught.
CONFIG_CORESIGHT_SOURCE_ETM4X is excluded because it depends on
CONFIG_PID_IN_CONTEXTIDR which has a performance cost.

Signed-off-by: James Clark <james.clark@arm.com>
---
 arch/arm64/configs/defconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ef3467092ded..c183914ab999 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1340,4 +1340,13 @@ CONFIG_DEBUG_FS=y
 # CONFIG_SCHED_DEBUG is not set
 # CONFIG_DEBUG_PREEMPT is not set
 # CONFIG_FTRACE is not set
+CONFIG_CORESIGHT=m
+CONFIG_CORESIGHT_LINK_AND_SINK_TMC=m
+CONFIG_CORESIGHT_CATU=m
+CONFIG_CORESIGHT_SINK_TPIU=m
+CONFIG_CORESIGHT_SINK_ETBV10=m
+CONFIG_CORESIGHT_STM=m
+CONFIG_CORESIGHT_CPU_DEBUG=m
+CONFIG_CORESIGHT_CTI=m
+CONFIG_CORESIGHT_CTI_INTEGRATION_REGS=y
 CONFIG_MEMTEST=y
-- 
2.28.0

Re: [PATCH 0/1] arm64: defconfig: Add Coresight as module

[Catalin Marinas]

On Wed, Sep 21, 2022 at 03:05:34PM +0100, James Clark wrote:
> As suggested by Catalin here's the change to add Coresight to defconfig.
> 
> Unfortunately I don't think we should add CONFIG_CORESIGHT_SOURCE_ETM4X
> which builds a few files until [1] is merged because of the overhead
> of CONFIG_PID_IN_CONTEXTIDR.
> 
> [1]: https://lore.kernel.org/lkml/20211021134530.206216-1-leo.yan@linaro.org/T/

I thought the overhead wasn't the problem, it's mostly negligible. We
can probably save a few more cycles on the __switch_to() path by
replacing several isb()s in those functions with a single one just
before cpu_switch_to().

IIRC the issue is that unless a process runs in the root pid namespace,
the actual pid written to contextidr is meaningless.

Now that you reminded me of that thread, I see three options (sorry, not
entirely related to the defconfig updates):

1. Remove CONFIG_PID_IN_CONTEXTIDR and corresponding code completely,
   find other events to correlate the task with the trace.

2. Always on CONFIG_PID_IN_CONTEXTIDR (we might as well remove the
   Kconfig entry). This would write the root pid namespace value
   (task_pid_nr()).

3. Similar to (2) but instead write task_pid_nr_ns(). An alternative
   here is to write -1 if the task is not in the root pid namespace.

Strong preference for (1).

-- 
Catalin

Re: [PATCH 1/1] arm64: defconfig: Add Coresight as module

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On Wed, Sep 21, 2022 at 03:05:35PM +0100, James Clark wrote:

> +CONFIG_CORESIGHT_CTI=m
> +CONFIG_CORESIGHT_CTI_INTEGRATION_REGS=y

Do we want this turned on by default?  According to the
description it's a bit dangerous and it's exposed via sysfs
rather than debugfs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] arm64: defconfig: Add Coresight as module

[Mathieu Poirier]

On Wed, Sep 21, 2022 at 04:26:59PM +0100, Mark Brown wrote:
> On Wed, Sep 21, 2022 at 03:05:35PM +0100, James Clark wrote:
> 
> > +CONFIG_CORESIGHT_CTI=m
> > +CONFIG_CORESIGHT_CTI_INTEGRATION_REGS=y
>

I agree - integration registers should not be enabled by default.

> Do we want this turned on by default?  According to the
> description it's a bit dangerous and it's exposed via sysfs
> rather than debugfs.

Re: [PATCH 1/1] arm64: defconfig: Add Coresight as module

[James Clark]

On 21/09/2022 17:46, Mathieu Poirier wrote:
> On Wed, Sep 21, 2022 at 04:26:59PM +0100, Mark Brown wrote:
>> On Wed, Sep 21, 2022 at 03:05:35PM +0100, James Clark wrote:
>>
>>> +CONFIG_CORESIGHT_CTI=m
>>> +CONFIG_CORESIGHT_CTI_INTEGRATION_REGS=y
>>
> 
> I agree - integration registers should not be enabled by default.
> 
>> Do we want this turned on by default?  According to the
>> description it's a bit dangerous and it's exposed via sysfs
>> rather than debugfs.
> 
> 

Should I disable just CONFIG_CORESIGHT_CTI_INTEGRATION_REGS or
CONFIG_CORESIGHT_CTI as well? There are other writable registers exposed
via sysfs outside of these two options, so I wanted to check if it's
just the integration registers that are the issue.

Re: [PATCH 1/1] arm64: defconfig: Add Coresight as module

[Suzuki K Poulose]

On 22/09/2022 10:04, James Clark wrote:
> 
> 
> On 21/09/2022 17:46, Mathieu Poirier wrote:
>> On Wed, Sep 21, 2022 at 04:26:59PM +0100, Mark Brown wrote:
>>> On Wed, Sep 21, 2022 at 03:05:35PM +0100, James Clark wrote:
>>>
>>>> +CONFIG_CORESIGHT_CTI=m
>>>> +CONFIG_CORESIGHT_CTI_INTEGRATION_REGS=y
>>>
>>
>> I agree - integration registers should not be enabled by default.
>>
>>> Do we want this turned on by default?  According to the
>>> description it's a bit dangerous and it's exposed via sysfs
>>> rather than debugfs.
>>
>>
> 
> Should I disable just CONFIG_CORESIGHT_CTI_INTEGRATION_REGS or
> CONFIG_CORESIGHT_CTI as well? There are other writable registers exposed
> via sysfs outside of these two options, so I wanted to check if it's
> just the integration registers that are the issue.

It is good/fine to keep CORESIGHT_CTI. But you may remove the 
INTEGRATION_REGS. They are there for "verification" of the CTI
integration on the SoC. We added them only for the platform
bring up purposes.

Suzuki

Re: [PATCH 0/1] arm64: defconfig: Add Coresight as module

[James Clark]

On 21/09/2022 16:08, Catalin Marinas wrote:
> On Wed, Sep 21, 2022 at 03:05:34PM +0100, James Clark wrote:
>> As suggested by Catalin here's the change to add Coresight to defconfig.
>>
>> Unfortunately I don't think we should add CONFIG_CORESIGHT_SOURCE_ETM4X
>> which builds a few files until [1] is merged because of the overhead
>> of CONFIG_PID_IN_CONTEXTIDR.
>>
>> [1]: https://lore.kernel.org/lkml/20211021134530.206216-1-leo.yan@linaro.org/T/
> 
> I thought the overhead wasn't the problem, it's mostly negligible. We
> can probably save a few more cycles on the __switch_to() path by
> replacing several isb()s in those functions with a single one just
> before cpu_switch_to().
> 
> IIRC the issue is that unless a process runs in the root pid namespace,
> the actual pid written to contextidr is meaningless.

This is true, and Leo has recently disabled it in that scenario in
aab473867fed.

> 
> Now that you reminded me of that thread, I see three options (sorry, not
> entirely related to the defconfig updates):
> 
> 1. Remove CONFIG_PID_IN_CONTEXTIDR and corresponding code completely,
>    find other events to correlate the task with the trace.

Unfortunately when tracing per core we would need kernel timestamps in
the trace to correlate to the switch records. At the moment Coresight is
using a different clock source so it's not possible and we're still
using the context ID to correlate samples.

With FEAT_TRF in v8.4 it will be possible to do this and we've started
working towards that here: 0f00b223ea22. But we'd still have to support
older hardware too, so CONFIG_PID_IN_CONTEXTIDR can't be removed completely.

For SPE it's not required because we already have the right timestamps
in the samples and we've added support for no context IDs in the decoder
here: 27d113cfe892

> 
> 2. Always on CONFIG_PID_IN_CONTEXTIDR (we might as well remove the
>    Kconfig entry). This would write the root pid namespace value
>    (task_pid_nr()).

If we're not worried about the overhead after all, this would be the
easiest solution. And then SPE or Coresight already decide whether they
want to use the value or not, so no further changes are needed.

From Leo's patch there is a table that shows a 1% overhead with it
enabled permanently, and I've heard a figure like that mentioned before.
So I could also resurrect that patch to use static keys? Although it's a
bit more complicated, that would be my preference. And then we can have
that mode always on.

> 
> 3. Similar to (2) but instead write task_pid_nr_ns(). An alternative
>    here is to write -1 if the task is not in the root pid namespace.
> 
> Strong preference for (1).
> 

Re: [PATCH 0/1] arm64: defconfig: Add Coresight as module

[Catalin Marinas]

On Thu, Sep 22, 2022 at 10:34:45AM +0100, James Clark wrote:
> On 21/09/2022 16:08, Catalin Marinas wrote:
> > 2. Always on CONFIG_PID_IN_CONTEXTIDR (we might as well remove the
> >    Kconfig entry). This would write the root pid namespace value
> >    (task_pid_nr()).
> 
> If we're not worried about the overhead after all, this would be the
> easiest solution. And then SPE or Coresight already decide whether they
> want to use the value or not, so no further changes are needed.
> 
> From Leo's patch there is a table that shows a 1% overhead with it
> enabled permanently, and I've heard a figure like that mentioned before.
> So I could also resurrect that patch to use static keys? Although it's a
> bit more complicated, that would be my preference. And then we can have
> that mode always on.

I don't think we should bother with static keys, just always enable it
but try to reduce/group the ISBs from all the functions called on the
__switch_to() path. We may actually get a speed-up.

-- 
Catalin

Re: [PATCH 0/1] arm64: defconfig: Add Coresight as module

[James Clark]

On 22/09/2022 11:52, Catalin Marinas wrote:
> On Thu, Sep 22, 2022 at 10:34:45AM +0100, James Clark wrote:
>> On 21/09/2022 16:08, Catalin Marinas wrote:
>>> 2. Always on CONFIG_PID_IN_CONTEXTIDR (we might as well remove the
>>>    Kconfig entry). This would write the root pid namespace value
>>>    (task_pid_nr()).
>>
>> If we're not worried about the overhead after all, this would be the
>> easiest solution. And then SPE or Coresight already decide whether they
>> want to use the value or not, so no further changes are needed.
>>
>> From Leo's patch there is a table that shows a 1% overhead with it
>> enabled permanently, and I've heard a figure like that mentioned before.
>> So I could also resurrect that patch to use static keys? Although it's a
>> bit more complicated, that would be my preference. And then we can have
>> that mode always on.
> 
> I don't think we should bother with static keys, just always enable it
> but try to reduce/group the ISBs from all the functions called on the
> __switch_to() path. We may actually get a speed-up.
> 

Ok thanks I will take a look at this