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

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

Takao Indoh <indou.takao@jp.fujitsu.com> writes:

> On 2015/09/08 18:48, Alexander Shishkin wrote:
>> Takao Indoh <indou.takao@jp.fujitsu.com> 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?
>
> That make sense, will do.
>
>> 
>>> +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.
>
> Yes, what I need is the last position where Intel PT hardware wrote
> data. Once kernel panic occurs, basically we should minimize the access
> to kernel data or functions because they may be broken. That is why I
> touch msr directly in this patch. But I agree to limit the access to msr
> except intel_pt driver. Using pmu.stop() or pt_event_stop() looks good
> to me.

Ok, thanks!

Regards,
--
Alex