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

[Peter Zijlstra <peterz@infradead.org>]

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;
> +}