Re: [PATCH v2 3/4] perf/x86/intel/pt: Add Intel PT logger

[Alexander Shishkin]

Takao Indoh <indou.takao@xxxxxxxxxxxxxx> writes:

> +/* intel_pt */
> +static struct perf_event_attr pt_attr_pt = {
> +	.config		= 0x400, /* bit10: TSCEn */

Doesn't it make sense to make these things configurable via sysfs or
whatnot?

> +static int pt_log_buf_nr_pages = 128; /* number of pages for log buffer */

Same here.

> +static struct cpumask pt_log_cpu_mask;
> +
> +static DEFINE_PER_CPU(struct perf_event *, pt_perf_event_pt);
> +static DEFINE_PER_CPU(struct perf_event *, pt_perf_event_sched);
> +static DEFINE_PER_CPU(struct perf_event *, pt_perf_event_dummy);
> +
> +/* Saved registers on panic */
> +static DEFINE_PER_CPU(u64, saved_msr_ctl);
> +static DEFINE_PER_CPU(u64, saved_msr_status);
> +static DEFINE_PER_CPU(u64, saved_msr_output_base);
> +static DEFINE_PER_CPU(u64, saved_msr_output_mask);
> +
> +void save_intel_pt_registers(void)
> +{
> +	int cpu = smp_processor_id();
> +	u64 ctl;
> +
> +	if (!cpumask_test_cpu(cpu, &pt_log_cpu_mask))
> +		return;
> +
> +	/* Save RTIT_CTL register */
> +	rdmsrl(MSR_IA32_RTIT_CTL, ctl);
> +	per_cpu(saved_msr_ctl, cpu) = ctl;
> +
> +	/* Stop tracing */
> +	ctl &= ~RTIT_CTL_TRACEEN;
> +	wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> +
> +	/* Save other registers */
> +	rdmsrl(MSR_IA32_RTIT_STATUS, per_cpu(saved_msr_status, cpu));
> +	rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, per_cpu(saved_msr_output_base, cpu));
> +	rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, per_cpu(saved_msr_output_mask, cpu));

I'd really like to keep the PT msr accesses confined to the intel_pt
driver. Maybe have a similar function there? That way you could also use
pt_config_start() instead of clearing TraceEn by hand.

Do you need these saved msr values for the crash tool? I'm guessing
you'd need the write pointer to figure out where the most recent data
is. But then again, if you go the perf_event_disable() path, it'll all
happen automatically in the driver. Or rather __perf_event_disable()
type of thing since this is strictly cpu-local. Or even
event::pmu::stop() would do the trick. The buffer's write head would
then be in this_cpu_ptr(&pt_ctx)->handle.head.

Thanks,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/