linux-kernel - Re: [PATCH v2 1/4] perf tools: Support Arm arch timer counter

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] [day] [month] [year] [list]

Message-ID: <20200821093632.GA22365@leoy-ThinkPad-X240s>
Date:   Fri, 21 Aug 2020 17:36:32 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     "liwei (GF)" <liwei391@...wei.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ian Rogers <irogers@...gle.com>, Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Kemeng Shi <shikemeng@...wei.com>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Igor Lubashev <ilubashe@...mai.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Stephane Eranian <eranian@...gle.com>,
        James Clark <james.clark@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] perf tools: Support Arm arch timer counter

Hi Wei,

On Thu, Aug 20, 2020 at 10:56:46AM +0800, liwei (GF) wrote:

[...]

> > +int perf_read_arch_timer_conversion(const struct perf_event_mmap_page *pc,
> > +				    struct perf_arch_timer_conversion *tc)
> > +{
> > +	bool cap_user_time_zero, cap_user_time_short;
> > +	u32 seq;
> > +	int i = 0;
> > +
> > +	while (1) {
> > +		seq = pc->lock;
> > +		/* Add barrier between the sequence lock and data accessing */
> > +		rmb();
> > +
> > +		tc->time_mult = pc->time_mult;
> > +		tc->time_shift = pc->time_shift;
> > +		tc->time_zero = pc->time_zero;
> > +		tc->time_cycles = pc->time_cycles;
> > +		tc->time_mask = pc->time_mask;
> > +		cap_user_time_zero = pc->cap_user_time_zero;
> > +		cap_user_time_short = pc->cap_user_time_short;
> > +
> > +		/* Add barrier between the data accessing and sequence lock */
> > +		rmb();
> > +		if (pc->lock == seq && !(seq & 1))
> > +			break;
> > +		if (++i > 10000) {
> > +			pr_debug("%s: failed to get perf_event_mmap_page lock\n",
> > +				 __func__);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (!cap_user_time_zero || !cap_user_time_short)
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> 
> Should we implement the perf_event__synth_time_conv() method? As it can be supported
> on arm64 arch now.

Thanks a lot for pointing out, agree that we should implement
perf_event__synth_time_conv() for Arm64.  Will do it in next spin.

> These is also a tsc get method called rdtsc(), weak implementation in 'tools/perf/util/tsc.c',
> maybe we should rename it to an arch independent name, and implement the arm64 version
> (read_cntvct_el0 in patch 3) here.

If connect with your comment in patch 02, I think you are asking to
consolidate for TSC common APIs and operations.  I took time to review
the codes for conversion between timer counter and time stamp, your
suggestion is reasonable and feasible for me.

But the consolidation will impact the existed x86 implementation, before
proceed to this direction for the next patch set, I'd like to get some
review from x86's developers.

@PeterZ, could you take a look for this and let me know if you have any
concern for the consolidation?

> Thus we can also make the testcase generic,
> instead of adding a new one for arm64 :).

Exactly.  Will do this if we get agreement for consolidation.

Thanks for the reviewing,
Leo