linux-kernel - Re: [patch 9/9] x86, bts, ftrace: a BTS ftrace plug-in prototype

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: <20081125164038.GE22504@elte.hu>
Date:	Tue, 25 Nov 2008 17:40:38 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Markus Metzger <markus.t.metzger@...el.com>
Cc:	hpa@...or.com, linux-kernel@...r.kernel.org, tglx@...utronix.de,
	markus.t.metzger@...il.com, roland@...hat.com,
	akpm@...ux-foundation.org, mtk.manpages@...il.com,
	eranian@...glemail.com, juan.villacis@...el.com
Subject: Re: [patch 9/9] x86, bts, ftrace: a BTS ftrace plug-in prototype


* Markus Metzger <markus.t.metzger@...el.com> wrote:

> Index: ftrace/kernel/trace/trace_bts.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ ftrace/kernel/trace/trace_bts.c	2008-11-25 08:18:07.000000000 +0100

> +#define this_tracer per_cpu(tracer, smp_processor_id())
> +#define this_buffer per_cpu(buffer, smp_processor_id())

please change these to inline functions.

> +/*
> + * Information to interpret a BTS record.
> + * This will go into an in-kernel BTS interface.
> + */
> +static unsigned char sizeof_field;
> +static unsigned long debugctl_mask;

__read_mostly.

> +static void bts_trace_cpuinit(struct cpuinfo_x86 *c)
> +{
> +	switch (c->x86) {
> +	case 0x6:
> +		switch (c->x86_model) {
> +		case 0x0 ... 0xC:
> +			break;
> +		case 0xD:
> +		case 0xE: /* Pentium M */
> +			sizeof_field = sizeof(long);
> +			debugctl_mask = (1<<6)|(1<<7);
> +			break;
> +		default:
> +			sizeof_field = 8;
> +			debugctl_mask = (1<<6)|(1<<7);
> +			break;
> +		}
> +		break;
> +	case 0xF:
> +		switch (c->x86_model) {
> +		case 0x0:
> +		case 0x1:
> +		case 0x2: /* Netburst */
> +			sizeof_field = sizeof(long);
> +			debugctl_mask = (1<<2)|(1<<3);
> +			break;
> +		default:
> +			/* sorry, don't know about them */
> +			break;
> +		}
> +		break;
> +	default:
> +		/* sorry, don't know about them */
> +		break;
> +	}
> +}
> +
> +static inline void bts_enable(void)
> +{
> +	unsigned long debugctl;
> +
> +	rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> +	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl | debugctl_mask);
> +}
> +
> +static inline void bts_disable(void)
> +{
> +	unsigned long debugctl;
> +
> +	rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> +	wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl & ~debugctl_mask);
> +}

these x86 architecture bits should move into arch/x86/kernel/ftrace.c. 
(or ds.c)

> +static void bts_trace_print_header(struct seq_file *m)
> +{
> +#ifdef __i386__
> +	seq_puts(m, "# CPU#    FROM           TO     FUNCTION\n");
> +	seq_puts(m, "#  |       |             |         |\n");
> +#else
> +	seq_puts(m,
> +		 "# CPU#        FROM                   TO         FUNCTION\n");
> +	seq_puts(m,
> +		 "#  |           |                     |             |\n");
> +#endif

lets just standardize on the 64-bit width, ok?

> +#ifdef CONFIG_KALLSYMS
> +		char function[KSYM_SYMBOL_LEN];
> +		sprint_symbol(function, it->from);
> +#else
> +		char *function = "<unknown>";
> +#endif

just do this:

		char function[KSYM_SYMBOL_LEN];
		sprint_symbol(function, it->from);

that should do the right thing in the !KALLSYMS case too.

> +struct tracer bts_tracer __read_mostly =
> +{
> +	.name		= "bts",

please rename it to: "hw-branch-tracer". "BTS" is an x86 concept and 
we want to keep the name generic.

	Ingo
--
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/