Subject: Re: [PATCH RFC 1/3] x86: Add Intel PT common files
To: <tglx@linutronix.de>
References: <1438145496-5932-1-git-send-email-indou.takao@jp.fujitsu.com>
 <1438145496-5932-2-git-send-email-indou.takao@jp.fujitsu.com>
 <alpine.DEB.2.11.1508021157520.3825@nanos>
From: Takao Indoh <indou.takao@jp.fujitsu.com>
CC: <mingo@redhat.com>, <hpa@zytor.com>, <a.p.zijlstra@chello.nl>,
        <acme@kernel.org>, <alexander.shishkin@linux.intel.com>,
        <vgoyal@redhat.com>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>
Message-ID: <55BEDC7E.9090808@jp.fujitsu.com>
Date: Mon, 3 Aug 2015 12:14:06 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101
 Thunderbird/38.1.0
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.11.1508021157520.3825@nanos>
Content-Type: text/plain; charset="iso-2022-jp"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2957
Lines: 133

On 2015/08/02 19:02, Thomas Gleixner wrote:
> On Wed, 29 Jul 2015, Takao Indoh wrote:
>> +/*
>> + * Table of Physical Addresses bits
>> + */
>> +enum topa_sz {
>> +	TOPA_4K	= 0,
>> +	TOPA_8K,
>> +	TOPA_16K,
>> +	TOPA_32K,
>> +	TOPA_64K,
>> +	TOPA_128K,
>> +	TOPA_256K,
>> +	TOPA_512K,
>> +	TOPA_1MB,
>> +	TOPA_2MB,
>> +	TOPA_4MB,
>> +	TOPA_8MB,
>> +	TOPA_16MB,
>> +	TOPA_32MB,
>> +	TOPA_64MB,
>> +	TOPA_128MB,
>> +	TOPA_SZ_END,
>> +};
> 
> While moving this around, can we pretty please clean that up? That
> enum just pointless. None of the values is ever used and they hardly
> have any value as they are just computable.

Ok, I'll update my patches based on Alex's comments, but before that
I'll clean up intel_pt.h and perf_event_intel_pt.c.

Thanks,
Takao Indoh

> 
>> +static inline unsigned int sizes(enum topa_sz tsz)
>> +{
>> +	return 1 << (tsz + 12);
> 
> 12?? PAGE_SHIFT perhaps?
> 
>> +#define TOPA_SHIFT 12
> 
> Sigh.
> 
>> diff --git a/arch/x86/kernel/cpu/intel_pt_cap.c b/arch/x86/kernel/cpu/intel_pt_cap.c
>> new file mode 100644
>> index 0000000..a2cfbfc
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/intel_pt_cap.c
>> @@ -0,0 +1,69 @@
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/mm.h>
>> +#include <asm/intel_pt.h>
>> +
>> +enum cpuid_regs {
>> +	CR_EAX = 0,
>> +	CR_ECX,
>> +	CR_EDX,
>> +	CR_EBX
>> +};
>> +
>> +static u32 cpuid_cache[4 * PT_CPUID_LEAVES];
> 
> 4 ? Magic constant pulled from thin air?
> 
>> +static int pt_cap_initialized;
>> +
>> +#define PT_CAP(_n, _l, _r, _m)						\
>> +	[PT_CAP_ ## _n] = { .name = __stringify(_n), .leaf = _l,	\
>> +			    .reg = _r, .mask = _m }
>> +
>> +static struct pt_cap_desc {
>> +	const char	*name;
>> +	u32		leaf;
>> +	u8		reg;
>> +	u32		mask;
>> +} pt_caps[] = {
>> +	PT_CAP(max_subleaf,		0, CR_EAX, 0xffffffff),
>> +	PT_CAP(cr3_filtering,		0, CR_EBX, BIT(0)),
>> +	PT_CAP(topa_output,		0, CR_ECX, BIT(0)),
>> +	PT_CAP(topa_multiple_entries,	0, CR_ECX, BIT(1)),
>> +	PT_CAP(payloads_lip,		0, CR_ECX, BIT(31)),
>> +};
>> +
>> +u32 pt_cap_get(enum pt_capabilities cap)
>> +{
>> +	struct pt_cap_desc *cd = &pt_caps[cap];
>> +	u32 c = cpuid_cache[cd->leaf * 4 + cd->reg];
> 
> Ditto
> 
>> +	unsigned int shift = __ffs(cd->mask);
>> +
>> +	return (c & cd->mask) >> shift;
>> +}
>> +
>> +const char *pt_cap_name(enum pt_capabilities cap)
>> +{
>> +	return pt_caps[cap].name;
>> +}
>> +
>> +int pt_cap_num(void)
>> +{
>> +	return ARRAY_SIZE(pt_caps);
>> +}
>> +
>> +void __init pt_cap_init(void)
>> +{
>> +	int i;
>> +
>> +	if (pt_cap_initialized)
>> +		return;
>> +
>> +	for (i = 0; i < PT_CPUID_LEAVES; i++) {
>> +		cpuid_count(20, i,
>> +			    &cpuid_cache[CR_EAX + i*4],
> 
> Once more.
> 
> Thanks,
> 
> 	tglx
> 


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