linux-kernel - Re: [PATCH 2/3] perf parse-regs: Add generic support for non-gprs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 May 2019 15:25:51 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>,
        linux-perf-users@...r.kernel.org,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 2/3] perf parse-regs: Add generic support for non-gprs



On 5/14/2019 2:19 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 14, 2019 at 07:39:12AM -0700, kan.liang@...ux.intel.com escreveu:
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> Some non general purpose registers, e.g. XMM, can be collected on some
>> platforms, e.g. X86 Icelake.
>>
>> Add a weak function has_non_gprs_support() to check if the
>> kernel/hardware support non-gprs collection.
>> Add a weak function non_gprs_mask() to return non-gprs mask.
>>
>> By default, the non-gprs collection is not support. Specific platforms
>> should implement their own non-gprs functions if they can collect
>> non-gprs.
>>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>>   tools/perf/util/parse-regs-options.c | 10 +++++++++-
>>   tools/perf/util/perf_regs.c          | 10 ++++++++++
>>   tools/perf/util/perf_regs.h          |  2 ++
>>   3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c
>> index b21617f..2f90f31 100644
>> --- a/tools/perf/util/parse-regs-options.c
>> +++ b/tools/perf/util/parse-regs-options.c
>> @@ -12,6 +12,7 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
>>   	const struct sample_reg *r;
>>   	char *s, *os = NULL, *p;
>>   	int ret = -1;
>> +	bool has_non_gprs = has_non_gprs_support(intr);
> 
> This is generic code, what does "non gprs" means for !Intel? Can we come
> up with a better, not architecture specific jargon? Or you mean "general
> purpose registers"?

I mean "general purpose registers".

> 
> Perhaps we can ask for a register mask for use with intr? I.e.:
> 
> For the -I/--intr-regs
> 
>          uint64_t mask = arch__intr_reg_mask();
> 
> And for --user-regs
> 
>          uint64_t mask = arch__user_reg_mask();
> 
> And then on that loop do:
> 
>    	for (r = sample_reg_masks; r->name; r++) {
> 		if (r->mask & mask))
>    			fprintf(stderr, "%s ", r->name);
>    	}
> 
> ?

Yes, it looks like a little bit simpler than my implementation.
I will send out V2.

Thanks,
Kan

>    
>>   	if (unset)
>>   		return 0;
>> @@ -37,6 +38,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
>>   			if (!strcmp(s, "?")) {
>>   				fprintf(stderr, "available registers: ");
>>   				for (r = sample_reg_masks; r->name; r++) {
>> +					if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK))
>> +						break;
>>   					fprintf(stderr, "%s ", r->name);
>>   				}
>>   				fputc('\n', stderr);
>> @@ -44,6 +47,8 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
>>   				return -1;
>>   			}
>>   			for (r = sample_reg_masks; r->name; r++) {
>> +				if (!has_non_gprs && (r->mask & ~PERF_REGS_MASK))
>> +					continue;
>>   				if (!strcasecmp(s, r->name))
>>   					break;
>>   			}
>> @@ -64,8 +69,11 @@ __parse_regs(const struct option *opt, const char *str, int unset, bool intr)
>>   	ret = 0;
>>   
>>   	/* default to all possible regs */
>> -	if (*mode == 0)
>> +	if (*mode == 0) {
>>   		*mode = PERF_REGS_MASK;
>> +		if (has_non_gprs)
>> +			*mode |= non_gprs_mask(intr);
>> +	}
>>   error:
>>   	free(os);
>>   	return ret;
>> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
>> index 2acfcc5..0d278b6 100644
>> --- a/tools/perf/util/perf_regs.c
>> +++ b/tools/perf/util/perf_regs.c
>> @@ -13,6 +13,16 @@ int __weak arch_sdt_arg_parse_op(char *old_op __maybe_unused,
>>   	return SDT_ARG_SKIP;
>>   }
>>   
>> +bool __weak has_non_gprs_support(bool intr __maybe_unused)
>> +{
>> +	return false;
>> +}
>> +
>> +u64 __weak non_gprs_mask(bool intr __maybe_unused)
>> +{
>> +	return 0;
>> +}
>> +
>>   #ifdef HAVE_PERF_REGS_SUPPORT
>>   int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
>>   {
>> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
>> index 1a15a4b..983b4e6 100644
>> --- a/tools/perf/util/perf_regs.h
>> +++ b/tools/perf/util/perf_regs.h
>> @@ -23,6 +23,8 @@ enum {
>>   };
>>   
>>   int arch_sdt_arg_parse_op(char *old_op, char **new_op);
>> +bool has_non_gprs_support(bool intr);
>> +u64 non_gprs_mask(bool intr);
>>   
>>   #ifdef HAVE_PERF_REGS_SUPPORT
>>   #include <perf_regs.h>
>> -- 
>> 2.7.4
>