Messages in this thread | | | From | Mike Leach <> | Date | Fri, 17 Dec 2021 11:52:59 +0000 | Subject | Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable |
| |
Hi James
On Fri, 17 Dec 2021 at 09:41, James Clark <james.clark@arm.com> wrote: > > > > 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? >
Correct - you may want to branch broadcast over a small ranges, but otherwise use return stack to decrease the amount of trace in other areas. Once the complex config sets are accepted upstream then this set-up will be possible by writing a config to do this.
> > 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. >
Someone could set this in sysfs - when collecting data via sysfs. In this case they would not be using perf for decode anyway as you say.
What you have added here is a new method for perf to set this feature and perf always starts off with a clean configuration - then adds according to command line options. Therefore this will be the first time anyone will have been able to set this for a perf session.
It there are potential limitations, then we need to be clear about them - then users of perf, and other hypothetical tools can make a good choice about if this option is appropriate - and we avoid mailing list complaints (or at least can point to the docs) if users find issues that they were warned about!
> 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. >
really depends on what was tested! If it is being used over static code then the subsequent decode will be fine,
Regards
Mike
> 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 > > > > > >
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
| |