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

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] [day] [month] [year] [list]

Message-ID: <87d1xjnb7m.fsf@ashishki-desk.ger.corp.intel.com>
Date:	Tue, 15 Sep 2015 15:01:01 +0300
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	Takao Indoh <indou.takao@...fujitsu.com>, tglx@...utronix.de,
	mingo@...hat.com, hpa@...or.com, a.p.zijlstra@...llo.nl,
	acme@...nel.org, vgoyal@...hat.com, rostedt@...dmis.org
Cc:	linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2 3/4] perf/x86/intel/pt: Add Intel PT logger

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

> On 2015/09/08 18:48, Alexander Shishkin wrote:
>> Takao Indoh <indou.takao@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/