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

[Takao Indoh <indou.takao@jp.fujitsu.com>]

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.

Thanks,
Takao Indoh


> 
> Thanks,
> --
> Alex
>