linux-kernel - Re: [PATCH] x86/perf: Add hardware performance events support for Zhaoxin CPU.

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]

Message-ID: <20200331101834.GP20730@hirez.programming.kicks-ass.net>
Date:   Tue, 31 Mar 2020 12:18:34 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     CodyYao-oc <CodyYao-oc@...oxin.com>
Cc:     mingo@...hat.com, acme@...nel.org, mark.rutland@....com,
        alexander.shishkin@...ux.intel.com, jolsa@...hat.com,
        namhyung@...nel.org, tglx@...utronix.de, bp@...en8.de,
        hpa@...or.com, x86@...nel.org, linux-kernel@...r.kernel.org,
        cooperyan@...oxin.com
Subject: Re: [PATCH] x86/perf: Add hardware performance events support for
 Zhaoxin CPU.

On Tue, Mar 31, 2020 at 05:39:59PM +0800, CodyYao-oc wrote:
> Zhaoxin CPU has provided facilities for monitoring performance
> via PMU(Performance Monitor Unit), but the functionality is unused so far.
> Therefore, add support for zhaoxin pmu to make performance related
> hardware events available.

This looks like an Intel Architectural PMU v2 or so, is that correct? Do
you have a link to documentation for your CPU?

> +static void zhaoxin_pmu_disable_all(void)
> +{
> +	wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> +}
> +
> +static void zhaoxin_pmu_enable_all(int added)
> +{
> +	wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, x86_pmu.intel_ctrl);
> +}
> +
> +static inline u64 zhaoxin_pmu_get_status(void)
> +{
> +	u64 status;
> +
> +	rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, status);
> +
> +	return status;
> +}
> +
> +static inline void zhaoxin_pmu_ack_status(u64 ack)
> +{
> +	wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, ack);
> +}

> +static int zhaoxin_pmu_handle_irq(struct pt_regs *regs)
> +{
> +	struct perf_sample_data data;
> +	struct cpu_hw_events *cpuc;
> +	int bit;
> +	u64 status;
> +	bool is_zxc;
> +	int handled = 0;
> +
> +	cpuc = this_cpu_ptr(&cpu_hw_events);
> +	apic_write(APIC_LVTPC, APIC_DM_NMI);
> +	zhaoxin_pmu_disable_all();
> +	status = zhaoxin_pmu_get_status();
> +	if (!status)
> +		goto done;
> +
> +	if (boot_cpu_data.x86 == 0x06 &&
> +		(boot_cpu_data.x86_model == 0x0f ||
> +			boot_cpu_data.x86_model == 0x19))
> +		is_zxc = true;
> +again:
> +
> +	/*Clearing status works only if the global control is enable on zxc.*/
> +	if (is_zxc)
> +		wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, x86_pmu.intel_ctrl);
> +
> +	zhaoxin_pmu_ack_status(status);
> +
> +	if (is_zxc)
> +		wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);

That's an unfortunate errata; perhaps write it like so:

static inline void zxc_pmu_ack_status(u64 status)
{
	/*
	 * ZXC needs global control enabled in order to clear status bits.
	 */
	zhaoxin_pmu_enable_all(0);
	zhaoxin_pmu_ack_status(status);
	zhaoxin_pmu_disable_all();
}

	if (is_zxc)
		zxc_pmu_ack_status(status);
	else
		zhaoxin_pmu_ack_status(status);

Alternatively; you can do a whole zxc specific handle_irq() and move the
get/ack status before disable_all(). If you do that, then factor this:

> +	/*
> +	 * CondChgd bit 63 doesn't mean any overflow status. Ignore
> +	 * and clear the bit.
> +	 */
> +	if (__test_and_clear_bit(63, (unsigned long *)&status)) {
> +		if (!status)
> +			goto done;
> +	}
> +
> +	for_each_set_bit(bit, (unsigned long *)&status, X86_PMC_IDX_MAX) {
> +		struct perf_event *event = cpuc->events[bit];
> +
> +		handled++;
> +
> +		if (!test_bit(bit, cpuc->active_mask))
> +			continue;
> +
> +		x86_perf_event_update(event);
> +		perf_sample_data_init(&data, 0, event->hw.last_period);
> +
> +		if (!x86_perf_event_set_period(event))
> +			continue;
> +
> +		if (perf_event_overflow(event, &data, regs))
> +			x86_pmu_stop(event, 0);
> +	}

bit into it's own function so you don't have to duplicate it. Then the
two handle_irq() functions only differ in the status handling.

> +
> +	/*
> +	 * Repeat if there is more work to be done:
> +	 */
> +	status = zhaoxin_pmu_get_status();
> +	if (status)
> +		goto again;
> +
> +done:
> +	zhaoxin_pmu_enable_all(0);
> +	return handled;
> +}