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

[Arnaldo Carvalho de Melo]

Em Tue, May 14, 2019 at 03:25:51PM -0400, Liang, Kan escreveu:
> 
> 
> On 5/14/2019 2:19 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, May 14, 2019 at 07:39:12AM -0700, kan.liang@xxxxxxxxxxxxxxx escreveu:
> > > From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > >   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".

Ok
 
> > 
> > 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!
 
> 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
> > 

-- 

- Arnaldo