Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[James Clark <james.clark@linaro.org>]

On 04/12/2025 1:45 pm, Mike Leach wrote:
> Hi,
> 
> On Thu, 4 Dec 2025 at 10:55, James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 02/12/2025 11:53 am, James Clark wrote:
>>>
>>>
>>> On 02/12/2025 11:43 am, Leo Yan wrote:
>>>> On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:
>>>>
>>>> [...]
>>>>
>>>>> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu
>>>>> cpu, u32 *offset,
>>>>>        case CS_ETMV3:
>>>>>            magic = __perf_cs_etmv3_magic;
>>>>>            /* Get configuration register */
>>>>> -        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
>>>>> +        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
>>>>
>>>> I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().
>>>>
>>>> For ETMv3, we directly pass CONFIG to the kernel, and after validation
>>>> in the dirver, then the value will be set to ETMCR.  If we already know
>>>> the config value is consistent between user space and kernel, why
>>
>> One other note is that since moving the timestamp field, this is no
>> longer true either. The value in attr.config isn't directly put into ETMCR.
>>
>>>> introduce a redundant "guess" operation here?
>>>>
>>>> Thanks,
>>>> Leo
>>>
>>> Because userspace doesn't always come up with the same value as the
>>> driver. For example right now in ETM3, ETMCR_RETURN_STACK isn't set
>>> depending on certain conditions that userspace doesn't know about.  ETM4
>>> has the same for TRCCONFIGR_RS and maybe some others. In the future,
>>> other versions of the driver could do different things as long as we
>>> don't break decoding.
>>>
>>> I didn't want the function name to imply it was doing something it
>>> wasn't as that confused me a little bit. It's definitely not "getting"
>>> the value. Maybe "guess" isn't the best it could be, but it's not far off.
>>>
>>
> 
> Perhaps cs_etm_synth_etmcr()? We cannot read it directly as it has not

synth is a good name, I can use that.

> been set at the time of creating these headers. (unlike the sets of
> static read only IDR regs that we do read).
> 
> When in perf mode the only configuration bits set in the ConfigR for
> either ETM3 or 4 are those generated or implied by parameters on the
> perf command line.
> This info has to pass from perf to the driver somehow. Evidently many
> years ago, when only ETMv3/PTM existed the easy way was perf.config ==
> etm.configr, now that is no longer feasible.
> As long as perf and the drivers interpret the command line attributes
> in the same way - all is well.
> 
> As James says, the actual configr can differ from the synth one - the
> key is the bits that control the trace format - e.g. cyclecounts,
> rather than trace filtering e.g. userspace/kernel that affects the
> drivers configr but not the synthesized value in perf.
> Decode cares about format, not about filtering. Additionally some
> things - like return-stack are implementation dependent - optional on
> PTM, not at all on ETMv3. If the trace unit does not support it then
> the drivers ignore this. the only effect on the trace output is less
> compression if retstack cannot be used.
> 
> Generally decode needs to know about things that affect format and
> function, rather than filtering.
> 
> Mike