On 27/04/2023 16:12, Leo Yan wrote:
Hi James,
On Mon, Apr 24, 2023 at 02:47:44PM +0100, James Clark wrote:
Currently the cs_etm_set_option() function both validates and applies the config options. Because it's only called when they are added automatically, there are some paths where the user can apply the option on the command line and skip the validation. By moving it to the end it covers both cases.
Also, options don't need to be re-applied anyway, Perf handles parsing and applying the config terms automatically.
Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/cs-etm.c | 152 +++++++++++++----------------- 1 file changed, 68 insertions(+), 84 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index f9b9ebf7fffc..af0a2400c655 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -69,21 +69,29 @@ static const char * const metadata_ete_ro[] = { static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu); -static int cs_etm_set_context_id(struct auxtrace_record *itr,
struct evsel *evsel, int cpu)+static int cs_etm_validate_context_id(struct auxtrace_record *itr,
struct evsel *evsel, int cpu){
- struct cs_etm_recording *ptr;
- struct perf_pmu *cs_etm_pmu;
- struct cs_etm_recording *ptr =
container_of(itr, struct cs_etm_recording, itr);- struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; char path[PATH_MAX];
- int err = -EINVAL;
- int err; u32 val;
- u64 contextid;
- u64 contextid =
evsel->core.attr.config &(perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") |perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2"));Seems to me, this would break backward compability.
The old kernel (before 5.11) doesn't provide 'contextid1' and 'contextid2', so we always check the entry 'contextid' rather than 'contextid1' and 'contextid2'.
With this change, if a kernel doesn't contain 'contextid1' and 'contextid2' formats, will perf tool never trace for contexid?
No because I changed to to be purely validation, so the format flags would still be applied. But yes I think you are right there is a small issue.
Now validation of 'contextid' isn't done on pre 5.11 kernels. But that only checks for ETMv3 anyway. Validation of 'contextid1' and 'contextid2' isn't a problem, because if the kernel doesn't support them they can't be applied on the command line anyway.
I can fix it by checking for 'contextid' and ETMv3 first and then doing 'contextid1' and 'contextid2' after.
Thanks, Leo