linux-kernel - Re: [Patch v2 12/24] perf/x86/intel: Allocate arch-PEBS buffer and initialize PEBS

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

Message-ID: <2e1b48d7-1528-4de0-affa-e6c13e0ce1b1@linux.intel.com>
Date: Wed, 26 Feb 2025 13:48:52 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo
 <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
 Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Kan Liang <kan.liang@...ux.intel.com>, Andi Kleen <ak@...ux.intel.com>,
 Eranian Stephane <eranian@...gle.com>, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v2 12/24] perf/x86/intel: Allocate arch-PEBS buffer and
 initialize PEBS_BASE MSR


On 2/25/2025 7:18 PM, Peter Zijlstra wrote:
> On Tue, Feb 18, 2025 at 03:28:06PM +0000, Dapeng Mi wrote:
>> Arch-PEBS introduces a new MSR IA32_PEBS_BASE to store the arch-PEBS
>> buffer physical address. This patch allocates arch-PEBS buffer and then
>> initialize IA32_PEBS_BASE MSR with the buffer physical address.
> Not loving how this patch obscures the whole DS area thing and naming.

arch-PEBS uses a totally independent buffer to save the PEBS records and
don't use the debug store area anymore. To reuse the original function as
much as possible and don't mislead users to think arch-PEBS has some
relationship with debug store, the original key word "ds" in the function
names are changed to "BTS_PEBS". I know the name maybe not perfect, do you
have any suggestion? Thanks.


>
>
>> @@ -624,13 +604,18 @@ static int alloc_pebs_buffer(int cpu)
>>  	int max, node = cpu_to_node(cpu);
>>  	void *buffer, *insn_buff, *cea;
>>  
>> -	if (!x86_pmu.ds_pebs)
>> +	if (!intel_pmu_has_pebs())
>>  		return 0;
>>  
>> -	buffer = dsalloc_pages(bsiz, GFP_KERNEL, cpu);
>> +	buffer = dsalloc_pages(bsiz, preemptible() ? GFP_KERNEL : GFP_ATOMIC, cpu);
> But this plain smells bad, what is this about?

In initial implementation, alloc_pebs_buffer() could be called in 
init_debug_store_on_cpu() which may be in a irq context. But it was dropped
in latest implementation. So this change is not needed any more. Would drop
it in next version.


>
>>  	if (unlikely(!buffer))
>>  		return -ENOMEM;
>>  
>> +	if (x86_pmu.arch_pebs) {
>> +		hwev->pebs_vaddr = buffer;
>> +		return 0;
>> +	}
>> +
>>  	/*
>>  	 * HSW+ already provides us the eventing ip; no need to allocate this
>>  	 * buffer then.