Messages in this thread | | | Subject | Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable | From | James Clark <> | Date | Fri, 17 Dec 2021 09:40:55 +0000 |
| |
On 13/12/2021 09:48, Mike Leach wrote: > Hi James, > > A couple of points - relating mainly to docs: >
Hi Mike,
Thanks for the feedback. I was in the process of adding some docs and ran into this https://lkml.org/lkml/2021/12/16/1087 so I went to fix that first. Now I will add some more details and resubmit.
> 1. Activating branch broadcast overrides any setting of return stack. > As a minimum there needs to be a documentation update to reflect this > -Setting both options is not prohibited in hardware - and in the case > where we can use branch broadcast over a range both are then relevant. >
Do you mean that if branch broadcast and return stacks are both requested, but branch broadcast is limited to a range, return stacks will only be available outside that branch broadcast range? But if branch broadcast is enabled for all ranges there will be no return stacks at all?
> 2. A documented note to reflect that choosing this option will result > in a significant increase in the amount of trace generated - possible > danger of overflows, or less actual instructions covered. In addition > perhaps documents could reflect the intended use-case for this option, > given the disadvantages. >
Will do.
> 3. Has this been tested in a situation where it will be of use? > Testing against static code images will show the same decoded trace > output as not using branch broadcast. (although the packet dumps will > show additional output)> > Given a primary use is for situations where code is patched or > dynamically altered at runtime - then this can affect the full decode > output. If the code is being patched to only alter the branch > addresses then decode should work against static images. > If, however, we are tracing code that adds in new branches, on top of > NOPs for example, then the decoding against the original static image > will be wrong, as the image will have the NOPs, rather than the branch > instructions so the apparent location of E atoms will be in a > different position to the actual code. Is there anything in perf that > will ensure that the patched code is presented to the decoder? > > If there are potential decode issues - these too need documenting. >
I'm not sure this should be a blocking issue for this set. Branch broadcast could already be enabled by setting the mode via sysfs. And the perf decode part isn't necessarily a step in the workflow, maybe someone wants to gather data for another tool.
I will do some testing after this change though, but I imagine we would have had issues reported it it wasn't working already which lowers the priority.
James
> Other than the documents and testing, I cannot see any issues with > this patch set in terms of setting and enabling the option. > > Regards > > Mike > > > On Fri, 10 Dec 2021 at 17:22, Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: >> >> Hi James, >> >> On Thu, Dec 09, 2021 at 11:13:55AM +0000, James Clark wrote: >>> >>> >>> On 09/12/2021 11:00, Suzuki K Poulose wrote: >>>> On 08/12/2021 16:09, 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. >>>>> >>>>> 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 d2bafb50c66a..0a9bb943a5e5 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); >>>>> + } >>>> >>>> nit: While at this, please could you change the hard coded value >>>> to ETM4_CFG_BIT_RETSTK ? >>>> >>> I started changing them all because I had trouble searching for bits by name but then >>> I thought it would snowball into a bigger change so I undid it. >>> >>> I think I'll just go and do it now if it's an issue here. >> >> I can apply this set right away and you send another patch to fix all hard coded >> bitfields or you can send another revision with all 4 patches included in it >> (bitfields fix plus these 3). Just let me know what you want to do. And next >> time please add a cover letter. >> >> Thanks, >> Mathieu >> >>> >>>> Otherwise, looks good to me >>>> >>>> Suzuki > > >
| |