* [PATCH 1/3] coresight: Add config flag to enable branch broadcast @ 2021-12-08 16:09 James Clark 2021-12-08 16:09 ` [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable James Clark ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: James Clark @ 2021-12-08 16:09 UTC (permalink / raw) To: mathieu.poirier, coresight Cc: suzuki.poulose, James Clark, Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users When enabled, all taken branch addresses are output, even if the branch was because of a direct branch instruction. This enables reconstruction of the program flow without having access to the memory image of the code being executed. Use bit 8 for the config option which would be the correct bit for programming ETMv3. Although branch broadcast can't be enabled on ETMv3 because it's not in the define ETM3X_SUPPORTED_OPTIONS, using the correct bit might help prevent future collisions or allow it to be enabled if needed. Signed-off-by: James Clark <james.clark@arm.com> --- drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 +++++++++ include/linux/coresight-pmu.h | 2 ++ 3 files changed, 13 insertions(+) diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index c039b6ae206f..43bbd5dc3d3b 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -52,6 +52,7 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src); * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config'; * now take them as general formats and apply on all ETMs. */ +PMU_FORMAT_ATTR(branch_broadcast, "config:"__stringify(ETM_OPT_BRANCH_BROADCAST)); PMU_FORMAT_ATTR(cycacc, "config:" __stringify(ETM_OPT_CYCACC)); /* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */ PMU_FORMAT_ATTR(contextid1, "config:" __stringify(ETM_OPT_CTXTID)); @@ -97,6 +98,7 @@ static struct attribute *etm_config_formats_attr[] = { &format_attr_sinkid.attr, &format_attr_preset.attr, &format_attr_configid.attr, + &format_attr_branch_broadcast.attr, NULL, }; diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index bf18128cf5de..d2bafb50c66a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -692,6 +692,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev, ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); } + /* branch broadcast - enable if selected and supported */ + if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) { + if (!drvdata->trcbb) { + ret = -EINVAL; + goto out; + } else + config->cfg |= BIT(ETM4_CFG_BIT_BB); + } + out: return ret; } diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index 4ac5c081af93..6c2fd6cc5a98 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -18,6 +18,7 @@ * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and * directly use below macros as config bits. */ +#define ETM_OPT_BRANCH_BROADCAST 8 #define ETM_OPT_CYCACC 12 #define ETM_OPT_CTXTID 14 #define ETM_OPT_CTXTID2 15 @@ -25,6 +26,7 @@ #define ETM_OPT_RETSTK 29 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ +#define ETM4_CFG_BIT_BB 3 #define ETM4_CFG_BIT_CYCACC 4 #define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_VMID 7 -- 2.28.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-08 16:09 [PATCH 1/3] coresight: Add config flag to enable branch broadcast James Clark @ 2021-12-08 16:09 ` James Clark 2021-12-09 11:00 ` Suzuki K Poulose 2021-12-08 16:09 ` [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast James Clark 2021-12-09 9:21 ` [PATCH 1/3] coresight: Add config flag to enable " Suzuki K Poulose 2 siblings, 1 reply; 16+ messages in thread From: James Clark @ 2021-12-08 16:09 UTC (permalink / raw) To: mathieu.poirier, coresight Cc: suzuki.poulose, James Clark, Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users 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); + } + } /* * Set any selected configuration and preset. * -- 2.28.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-08 16:09 ` [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable James Clark @ 2021-12-09 11:00 ` Suzuki K Poulose 2021-12-09 11:13 ` James Clark 0 siblings, 1 reply; 16+ messages in thread From: Suzuki K Poulose @ 2021-12-09 11:00 UTC (permalink / raw) To: James Clark, mathieu.poirier, coresight Cc: Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users 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 ? Otherwise, looks good to me Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-09 11:00 ` Suzuki K Poulose @ 2021-12-09 11:13 ` James Clark 2021-12-10 17:22 ` Mathieu Poirier 2022-01-07 15:10 ` James Clark 0 siblings, 2 replies; 16+ messages in thread From: James Clark @ 2021-12-09 11:13 UTC (permalink / raw) To: Suzuki K Poulose, coresight Cc: Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier 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. > Otherwise, looks good to me > > Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-09 11:13 ` James Clark @ 2021-12-10 17:22 ` Mathieu Poirier 2021-12-13 9:48 ` Mike Leach 2021-12-13 14:23 ` James Clark 2022-01-07 15:10 ` James Clark 1 sibling, 2 replies; 16+ messages in thread From: Mathieu Poirier @ 2021-12-10 17:22 UTC (permalink / raw) To: James Clark Cc: Suzuki K Poulose, coresight, Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-10 17:22 ` Mathieu Poirier @ 2021-12-13 9:48 ` Mike Leach 2021-12-17 9:40 ` James Clark 2021-12-13 14:23 ` James Clark 1 sibling, 1 reply; 16+ messages in thread From: Mike Leach @ 2021-12-13 9:48 UTC (permalink / raw) To: Mathieu Poirier Cc: James Clark, Suzuki K Poulose, coresight, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users Hi James, A couple of points - relating mainly to docs: 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. 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. 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. 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-13 9:48 ` Mike Leach @ 2021-12-17 9:40 ` James Clark 2021-12-17 11:52 ` Mike Leach 0 siblings, 1 reply; 16+ messages in thread From: James Clark @ 2021-12-17 9:40 UTC (permalink / raw) To: Mike Leach Cc: Suzuki K Poulose, coresight, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier 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 > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-17 9:40 ` James Clark @ 2021-12-17 11:52 ` Mike Leach 2022-01-13 9:08 ` James Clark 0 siblings, 1 reply; 16+ messages in thread From: Mike Leach @ 2021-12-17 11:52 UTC (permalink / raw) To: James Clark Cc: Suzuki K Poulose, coresight, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-17 11:52 ` Mike Leach @ 2022-01-13 9:08 ` James Clark 0 siblings, 0 replies; 16+ messages in thread From: James Clark @ 2022-01-13 9:08 UTC (permalink / raw) To: Mike Leach Cc: Suzuki K Poulose, coresight, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier On 17/12/2021 11:52, Mike Leach wrote: > 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, > I've posted v2 of this change with some docs changes. I also did some testing around JITed code in java to see if there were any bad errors which I put in the cover letter. Although I think the main point of adding this format flag for perf is to open another avenue of testing or for people who just want to look at the raw trace manually for now. James > 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 >>> >>> >>> > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-10 17:22 ` Mathieu Poirier 2021-12-13 9:48 ` Mike Leach @ 2021-12-13 14:23 ` James Clark 1 sibling, 0 replies; 16+ messages in thread From: James Clark @ 2021-12-13 14:23 UTC (permalink / raw) To: Mathieu Poirier Cc: Suzuki K Poulose, coresight, Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users On 10/12/2021 17:22, Mathieu Poirier 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. I think I would like to hold off for a bit based on Mike's feedback. Seems like I may need to make a change about return stacks. Thanks James > > Thanks, > Mathieu > >> >>> Otherwise, looks good to me >>> >>> Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2021-12-09 11:13 ` James Clark 2021-12-10 17:22 ` Mathieu Poirier @ 2022-01-07 15:10 ` James Clark 2022-01-12 9:46 ` Suzuki K Poulose 1 sibling, 1 reply; 16+ messages in thread From: James Clark @ 2022-01-07 15:10 UTC (permalink / raw) To: Suzuki K Poulose, coresight Cc: Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier On 09/12/2021 11:13, 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. Hi Suzuki, I started on this and I think the only worthwhile change is to make them all consistent with sysreg.h. As in have xxx_SHIFT and xxx_MASK style definitions like: #define TRCCONFIGR_INSTP0_SHIFT 1 #define TRCCONFIGR_INSTPO_MASK GENMASK(1,0) This has been done for SPE and some of the new ETM stuff. If that sounds right to you I will go and do it as a followup patch to this one. It is quite a bit change so I can see maybe we don't want to do it? (Personally I would vote to do it) James > >> Otherwise, looks good to me >> >> Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable 2022-01-07 15:10 ` James Clark @ 2022-01-12 9:46 ` Suzuki K Poulose 0 siblings, 0 replies; 16+ messages in thread From: Suzuki K Poulose @ 2022-01-12 9:46 UTC (permalink / raw) To: James Clark, coresight Cc: Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users, Mathieu Poirier On 07/01/2022 15:10, James Clark wrote: > > > On 09/12/2021 11:13, 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. > > Hi Suzuki, > > I started on this and I think the only worthwhile change is to make them all consistent > with sysreg.h. As in have xxx_SHIFT and xxx_MASK style definitions like: > > #define TRCCONFIGR_INSTP0_SHIFT 1 > #define TRCCONFIGR_INSTPO_MASK GENMASK(1,0) > > This has been done for SPE and some of the new ETM stuff. If that sounds right to you > I will go and do it as a followup patch to this one. It is quite a bit change so I can > see maybe we don't want to do it? (Personally I would vote to do it) Yes, please go ahead with that. Thanks for taking it up ! Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast 2021-12-08 16:09 [PATCH 1/3] coresight: Add config flag to enable branch broadcast James Clark 2021-12-08 16:09 ` [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable James Clark @ 2021-12-08 16:09 ` James Clark 2021-12-10 2:41 ` Leo Yan 2021-12-09 9:21 ` [PATCH 1/3] coresight: Add config flag to enable " Suzuki K Poulose 2 siblings, 1 reply; 16+ messages in thread From: James Clark @ 2021-12-08 16:09 UTC (permalink / raw) To: mathieu.poirier, coresight Cc: suzuki.poulose, James Clark, Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users Now that a config flag for branch broadcast has been added, take it into account when trying to deduce what the driver would have programmed the TRCCONFIGR register to. Signed-off-by: James Clark <james.clark@arm.com> --- tools/include/linux/coresight-pmu.h | 2 ++ tools/perf/arch/arm/util/cs-etm.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h index 4ac5c081af93..6c2fd6cc5a98 100644 --- a/tools/include/linux/coresight-pmu.h +++ b/tools/include/linux/coresight-pmu.h @@ -18,6 +18,7 @@ * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and * directly use below macros as config bits. */ +#define ETM_OPT_BRANCH_BROADCAST 8 #define ETM_OPT_CYCACC 12 #define ETM_OPT_CTXTID 14 #define ETM_OPT_CTXTID2 15 @@ -25,6 +26,7 @@ #define ETM_OPT_RETSTK 29 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ +#define ETM4_CFG_BIT_BB 3 #define ETM4_CFG_BIT_CYCACC 4 #define ETM4_CFG_BIT_CTXTID 6 #define ETM4_CFG_BIT_VMID 7 diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index 293a23bf8be3..c7ef4e9b4a3a 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -527,6 +527,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) if (config_opts & BIT(ETM_OPT_CTXTID2)) config |= BIT(ETM4_CFG_BIT_VMID) | BIT(ETM4_CFG_BIT_VMID_OPT); + if (config_opts & BIT(ETM_OPT_BRANCH_BROADCAST)) + config |= BIT(ETM4_CFG_BIT_BB); + return config; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast 2021-12-08 16:09 ` [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast James Clark @ 2021-12-10 2:41 ` Leo Yan 2022-01-13 9:09 ` James Clark 0 siblings, 1 reply; 16+ messages in thread From: Leo Yan @ 2021-12-10 2:41 UTC (permalink / raw) To: James Clark Cc: mathieu.poirier, coresight, suzuki.poulose, Mike Leach, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users On Wed, Dec 08, 2021 at 04:09:07PM +0000, James Clark wrote: > Now that a config flag for branch broadcast has been added, take it into > account when trying to deduce what the driver would have programmed the > TRCCONFIGR register to. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > tools/include/linux/coresight-pmu.h | 2 ++ > tools/perf/arch/arm/util/cs-etm.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h > index 4ac5c081af93..6c2fd6cc5a98 100644 > --- a/tools/include/linux/coresight-pmu.h > +++ b/tools/include/linux/coresight-pmu.h > @@ -18,6 +18,7 @@ > * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and > * directly use below macros as config bits. > */ > +#define ETM_OPT_BRANCH_BROADCAST 8 I checked ETMv3 architecture spec (ARM IHI 0014Q), its bit 8 is "branch output" for all branch address outputting. I am not sure if it is the same thing between ETMv3's "branch output" and ETMv4's "branch broadcasting", but it makes sense for me to use bit 8 as an unified config bit to control these two options for ETMv3 and ETMv4 respectively. Just note, I understand this patch set is to enable branch broadcasting for entire memory region rather than using any comparators (see TRCBBCTLR) to limit branch broadcasting ranges. This is fine for me and we could enable ranges later. Reviewed-by: Leo Yan <leo.yan@linaro.org> > #define ETM_OPT_CYCACC 12 > #define ETM_OPT_CTXTID 14 > #define ETM_OPT_CTXTID2 15 > @@ -25,6 +26,7 @@ > #define ETM_OPT_RETSTK 29 > > /* ETMv4 CONFIGR programming bits for the ETM OPTs */ > +#define ETM4_CFG_BIT_BB 3 > #define ETM4_CFG_BIT_CYCACC 4 > #define ETM4_CFG_BIT_CTXTID 6 > #define ETM4_CFG_BIT_VMID 7 > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c > index 293a23bf8be3..c7ef4e9b4a3a 100644 > --- a/tools/perf/arch/arm/util/cs-etm.c > +++ b/tools/perf/arch/arm/util/cs-etm.c > @@ -527,6 +527,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) > if (config_opts & BIT(ETM_OPT_CTXTID2)) > config |= BIT(ETM4_CFG_BIT_VMID) | > BIT(ETM4_CFG_BIT_VMID_OPT); > + if (config_opts & BIT(ETM_OPT_BRANCH_BROADCAST)) > + config |= BIT(ETM4_CFG_BIT_BB); > + > return config; > } > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast 2021-12-10 2:41 ` Leo Yan @ 2022-01-13 9:09 ` James Clark 0 siblings, 0 replies; 16+ messages in thread From: James Clark @ 2022-01-13 9:09 UTC (permalink / raw) To: Leo Yan Cc: mathieu.poirier, coresight, suzuki.poulose, Mike Leach, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users On 10/12/2021 02:41, Leo Yan wrote: > On Wed, Dec 08, 2021 at 04:09:07PM +0000, James Clark wrote: >> Now that a config flag for branch broadcast has been added, take it into >> account when trying to deduce what the driver would have programmed the >> TRCCONFIGR register to. >> >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> tools/include/linux/coresight-pmu.h | 2 ++ >> tools/perf/arch/arm/util/cs-etm.c | 3 +++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h >> index 4ac5c081af93..6c2fd6cc5a98 100644 >> --- a/tools/include/linux/coresight-pmu.h >> +++ b/tools/include/linux/coresight-pmu.h >> @@ -18,6 +18,7 @@ >> * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and >> * directly use below macros as config bits. >> */ >> +#define ETM_OPT_BRANCH_BROADCAST 8 > > I checked ETMv3 architecture spec (ARM IHI 0014Q), its bit 8 is "branch > output" for all branch address outputting. I am not sure if it is the > same thing between ETMv3's "branch output" and ETMv4's "branch > broadcasting", but it makes sense for me to use bit 8 as an unified > config bit to control these two options for ETMv3 and ETMv4 > respectively. > > Just note, I understand this patch set is to enable branch broadcasting > for entire memory region rather than using any comparators (see > TRCBBCTLR) to limit branch broadcasting ranges. This is fine for me and > we could enable ranges later. > > Reviewed-by: Leo Yan <leo.yan@linaro.org> Hi Leo, Thanks for the review, I've posted v2, but I only put your reviewed by tag on this commit because I wasn't sure if you meant for the full set. Please take another look Thanks James > >> #define ETM_OPT_CYCACC 12 >> #define ETM_OPT_CTXTID 14 >> #define ETM_OPT_CTXTID2 15 >> @@ -25,6 +26,7 @@ >> #define ETM_OPT_RETSTK 29 >> >> /* ETMv4 CONFIGR programming bits for the ETM OPTs */ >> +#define ETM4_CFG_BIT_BB 3 >> #define ETM4_CFG_BIT_CYCACC 4 >> #define ETM4_CFG_BIT_CTXTID 6 >> #define ETM4_CFG_BIT_VMID 7 >> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c >> index 293a23bf8be3..c7ef4e9b4a3a 100644 >> --- a/tools/perf/arch/arm/util/cs-etm.c >> +++ b/tools/perf/arch/arm/util/cs-etm.c >> @@ -527,6 +527,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) >> if (config_opts & BIT(ETM_OPT_CTXTID2)) >> config |= BIT(ETM4_CFG_BIT_VMID) | >> BIT(ETM4_CFG_BIT_VMID_OPT); >> + if (config_opts & BIT(ETM_OPT_BRANCH_BROADCAST)) >> + config |= BIT(ETM4_CFG_BIT_BB); >> + >> return config; >> } >> >> -- >> 2.28.0 >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] coresight: Add config flag to enable branch broadcast 2021-12-08 16:09 [PATCH 1/3] coresight: Add config flag to enable branch broadcast James Clark 2021-12-08 16:09 ` [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable James Clark 2021-12-08 16:09 ` [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast James Clark @ 2021-12-09 9:21 ` Suzuki K Poulose 2 siblings, 0 replies; 16+ messages in thread From: Suzuki K Poulose @ 2021-12-09 9:21 UTC (permalink / raw) To: James Clark, mathieu.poirier, coresight Cc: Mike Leach, Leo Yan, John Garry, Will Deacon, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel, linux-kernel, linux-perf-users On 08/12/2021 16:09, James Clark wrote: > When enabled, all taken branch addresses are output, even if the branch > was because of a direct branch instruction. This enables reconstruction > of the program flow without having access to the memory image of the > code being executed. > > Use bit 8 for the config option which would be the correct bit for > programming ETMv3. Although branch broadcast can't be enabled on ETMv3 > because it's not in the define ETM3X_SUPPORTED_OPTIONS, using the > correct bit might help prevent future collisions or allow it to be > enabled if needed. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ > drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 +++++++++ > include/linux/coresight-pmu.h | 2 ++ > 3 files changed, 13 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index c039b6ae206f..43bbd5dc3d3b 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -52,6 +52,7 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src); > * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config'; > * now take them as general formats and apply on all ETMs. > */ > +PMU_FORMAT_ATTR(branch_broadcast, "config:"__stringify(ETM_OPT_BRANCH_BROADCAST)); > PMU_FORMAT_ATTR(cycacc, "config:" __stringify(ETM_OPT_CYCACC)); > /* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */ > PMU_FORMAT_ATTR(contextid1, "config:" __stringify(ETM_OPT_CTXTID)); > @@ -97,6 +98,7 @@ static struct attribute *etm_config_formats_attr[] = { > &format_attr_sinkid.attr, > &format_attr_preset.attr, > &format_attr_configid.attr, > + &format_attr_branch_broadcast.attr, > NULL, > }; > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index bf18128cf5de..d2bafb50c66a 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -692,6 +692,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); > } > > + /* branch broadcast - enable if selected and supported */ > + if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) { > + if (!drvdata->trcbb) { > + ret = -EINVAL; > + goto out; > + } else > + config->cfg |= BIT(ETM4_CFG_BIT_BB); nit: For consistent styling, I would recommend to wrap the else case also in { }. Otherwise looks good to me. Suzuki ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-01-13 9:09 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-08 16:09 [PATCH 1/3] coresight: Add config flag to enable branch broadcast James Clark 2021-12-08 16:09 ` [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable James Clark 2021-12-09 11:00 ` Suzuki K Poulose 2021-12-09 11:13 ` James Clark 2021-12-10 17:22 ` Mathieu Poirier 2021-12-13 9:48 ` Mike Leach 2021-12-17 9:40 ` James Clark 2021-12-17 11:52 ` Mike Leach 2022-01-13 9:08 ` James Clark 2021-12-13 14:23 ` James Clark 2022-01-07 15:10 ` James Clark 2022-01-12 9:46 ` Suzuki K Poulose 2021-12-08 16:09 ` [PATCH 3/3] perf cs-etm: Update deduction of TRCCONFIGR register for branch broadcast James Clark 2021-12-10 2:41 ` Leo Yan 2022-01-13 9:09 ` James Clark 2021-12-09 9:21 ` [PATCH 1/3] coresight: Add config flag to enable " Suzuki K Poulose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).