Re: [PATCH v2 2/6] coresight: Fail to open with return stacks if they are unavailable

[James Clark <james.clark@arm.com>]

On 11/03/2022 15:53, Mike Leach wrote:
> Hi,
> 
> 
> On Fri, 11 Mar 2022 at 14:52, James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 28/01/2022 11:24, Suzuki K Poulose wrote:
>>> Hi James
>>>
>>> On 13/01/2022 09:10, James Clark wrote:
>>>> Maintain consistency with the other options by failing to open when they
>>>> aren't supported. For example ETM_OPT_TS, ETM_OPT_CTXTID2 and the newly
>>>> added ETM_OPT_BRANCH_BROADCAST all return with -EINVAL if they are
>>>> requested but not supported by hardware.
>>>
>>> Looking at this again (with similar comment to the Branch Broadcast),
>>> won't it disable using retstack on all CPUs, even when some of them
>>> support it ?
>>>
>>> i.e., CPU0 - supports retstack, CPU1 - doesn't
>>>
>>> A perf run with retstack will fail, as CPU1 doesn't support it (even
>>> though we advertise it, unconditionally).
>>>
>>> So, if we ignore the failure, this would still allow CPU0 to use
>>> the feature and as long as the OpenCSD is able to decode the trace
>>> we should ignore the failure ?
>>>
>>> I think we may also need to tune the etm4x_enable_hw() to skip
>>> updating the TRCCONFIGR with features not supported by the ETM
>>>
>>
>> Hi Suzuki,
>>
>> I'm picking up this branch broadcast change again after the haitus.
>>
>> For this point, do you think it would be worth distinguishing between "no
>> known CPUs that support the feature" vs "not currently running on a
>> CPU that supports it but there are others that do"?
>>
>> Also would we want to distinguish between per-CPU or per-process events?
>> For the former it actually is possible to fail to open because all of
>> the information is known.
>>
>> I'm just thinking of the case where someone asks for a load of flags
>> and thinks that they're getting them but get no feedback that they won't.
>> But I understand having some complicated solution like I'm suggesting
>> might be even more surprising to users.
>>
>> Maybe the cleanest solution is to ask users to supply a config that
>> can work on anywhere the event could possibly be scheduled. It doesn't
>> really make sense to have retstack on a per-process event on big-little
>> and then getting half of one type of data and half of another. It would
>> make more sense to fail to open in that case and they have the choice of
>> either doing per-CPU events or disabling retstacks altogether.
>>
> 
> return stack has no effect on the decoder output whatsoever. The only
> effect is to reduce the amount of traced addresses at the input
> (leaving more space for other trace),
> so it is irrelevant if CPU0 supports it but CPU1 doesn't.
> 
> sequence:
> 
> BL r0 (return stack is used only on link instructions)
> ...
> RET
> 
> will output trace:-
> ATOM E (BL r0)
> ...
> ADDR_ELEM <ret addr>
> ATOM E (RET)
> 
> for no return stack,
> 
> ATOM E (BL r0)
> ...
> ATOM E (RET)
> 
> fior return stack.
> 
> In both cases the decoder will push the address after BL r0 onto its
> return stack.
> 
> In the first case the decoder will use the supplied address, in the
> second will pop the top of its return stack.
> 
> The decode output in both cases will be "branched to r0, ran code,
> returned via link register"
> 
> The outcome is identical for the client. So the case for not tracing
> on a core that does not have return stack if specified is weak.
> 
> Perhaps a warning will be sufficient?
> 
> Mike
> 
> 
> 
>> This seems like a similar problem to the issue causing the Coresight self
>> test failure where a certain sink was picked that couldn't be reached and
>> the test failed.
>>
>> In that case the change we made doesn't quite match up to my suggestion here:
>>
>>  * Per-cpu but an unreachable sink -> fail
>>  * Per-process and potentially reachable sink in the future -> pass
>>
>> Maybe it would have been better to say that the sink always has to be
>> reachable otherwise is the outcome predicatable?

Hi Mike,

If it has no effect on the output then it makes sense to me to just drop this
patch. I think even a warning would not add much and as far as I know they are
discouraged.

James

>>
>> James
>>
>>> Suzuki
>>>
>>>
>>>>
>>>> The consequence of not doing this is that the user may not be
>>>> aware that they are not enabling the feature as it is silently disabled.
>>>>
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++----
>>>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> index 04669ecc0efa..a93c1a5fe045 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -674,10 +674,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>>>>       }
>>>>         /* return stack - enable if selected and supported */
>>>> -    if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
>>>> -        /* bit[12], Return stack enable bit */
>>>> -        config->cfg |= BIT(12);
>>>> -
>>>> +    if (attr->config & BIT(ETM_OPT_RETSTK)) {
>>>> +        if (!drvdata->retstack) {
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        } else {
>>>> +            /* bit[12], Return stack enable bit */
>>>> +            config->cfg |= BIT(12);
>>>> +        }
>>>> +    }
>>>>       /*
>>>>        * Set any selected configuration and preset.
>>>>        *
>>>
> 
> 
>