linux-kernel - Re: [PATCH] perf/x86/intel: Clean up PEBS-via-PT on hybrid

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: <72dacfd6-4102-4efb-a699-875fa17acb28@linux.intel.com>
Date: Mon, 27 Jan 2025 09:34:18 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Adrian Hunter <adrian.hunter@...el.com>, peterz@...radead.org,
 mingo@...hat.com, acme@...nel.org, namhyung@...nel.org, irogers@...gle.com,
 alexander.shishkin@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf/x86/intel: Clean up PEBS-via-PT on hybrid



On 2025-01-27 3:19 a.m., Adrian Hunter wrote:
> On 24/01/25 20:34, kan.liang@...ux.intel.com wrote:
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> The PEBS-via-PT feature is exposed for the e-core of some hybrid
>> platforms, e.g., ADL and MTL. But it never works.
>>
>> $ dmesg | grep PEBS
>> [    1.793888] core: cpu_atom PMU driver: PEBS-via-PT
>>
>> $ perf record -c 1000 -e '{intel_pt/branch=0/,
>> cpu_atom/cpu-cycles,aux-output/pp}' -C8
>> Error:
>> The sys_perf_event_open() syscall returned with 22 (Invalid argument)
>> for event (cpu_atom/cpu-cycles,aux-output/pp).
>> "dmesg | grep -i perf" may provide additional information.
>>
>> The "PEBS-via-PT" is printed if the corresponding bit of per-PMU
>> capabilities is set. Since the feature is supported by the e-core HW,
>> perf sets the bit for e-core. However, for Intel PT, if a feature is not
>> supported on all CPUs, it is not supported at all. The PEBS-via-PT event
>> cannot be created successfully.
>>
>> The PEBS-via-PT is no longer enumerated on the latest hybrid platform. It
>> will be deprecated on future platforms with Arch PEBS. Let's remove it
>> from the existing hybrid platforms.
>>
>> Fixes: d9977c43bff8 ("perf/x86: Register hybrid PMUs")
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> Cc: Adrian Hunter <adrian.hunter@...el.com>
>> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>> ---
>>  arch/x86/events/intel/core.c | 10 ----------
>>  arch/x86/events/intel/ds.c   |  2 +-
>>  2 files changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 7f1b6b90a5fb..0a1030eb6db8 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4981,11 +4981,6 @@ static void intel_pmu_check_hybrid_pmus(struct x86_hybrid_pmu *pmu)
>>  	else
>>  		pmu->intel_ctrl &= ~(1ULL << GLOBAL_CTRL_EN_PERF_METRICS);
>>  
>> -	if (pmu->intel_cap.pebs_output_pt_available)
>> -		pmu->pmu.capabilities |= PERF_PMU_CAP_AUX_OUTPUT;
>> -	else
>> -		pmu->pmu.capabilities &= ~PERF_PMU_CAP_AUX_OUTPUT;
>> -
>>  	intel_pmu_check_event_constraints(pmu->event_constraints,
>>  					  pmu->cntr_mask64,
>>  					  pmu->fixed_cntr_mask64,
>> @@ -5063,9 +5058,6 @@ static bool init_hybrid_pmu(int cpu)
>>  
>>  	pr_info("%s PMU driver: ", pmu->name);
>>  
>> -	if (pmu->intel_cap.pebs_output_pt_available)
>> -		pr_cont("PEBS-via-PT ");
>> -
>>  	pr_cont("\n");
>>  
>>  	x86_pmu_show_pmu_cap(&pmu->pmu);
>> @@ -6420,11 +6412,9 @@ static __always_inline int intel_pmu_init_hybrid(enum hybrid_pmu_type pmus)
>>  		pmu->intel_cap.capabilities = x86_pmu.intel_cap.capabilities;
>>  		if (pmu->pmu_type & hybrid_small_tiny) {
>>  			pmu->intel_cap.perf_metrics = 0;
>> -			pmu->intel_cap.pebs_output_pt_available = 1;
>>  			pmu->mid_ack = true;
>>  		} else if (pmu->pmu_type & hybrid_big) {
>>  			pmu->intel_cap.perf_metrics = 1;
>> -			pmu->intel_cap.pebs_output_pt_available = 0;
>>  			pmu->late_ack = true;
>>  		}
>>  	}
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index 13a78a8a2780..123b0e4392b4 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -2742,7 +2742,7 @@ void __init intel_ds_init(void)
>>  			}
>>  			pr_cont("PEBS fmt%d%c%s, ", format, pebs_type, pebs_qual);
>>  
>> -			if (!is_hybrid() && x86_pmu.intel_cap.pebs_output_pt_available) {
>> +			if (x86_pmu.intel_cap.pebs_output_pt_available) {
> 
> I'd suggest keeping the !is_hybrid() check here
> since it documents and enforces the effective policy.

Sigh, the !is_hybrid() should never work here either. Because the
intel_ds_init() is invoked before intel_pmu_init_hybrid(), which enable
perf_is_hybrid.

I think we'd better remove it, but I will add some comments to explain
the status of PEBS-via-PT on hybrid instead. For example,

/*
 * The PEBS-via-PT is not supported on hybrid platforms, because not all
 * CPUs of a hybrid machine support the feature. The global
 * x86_pmu.intel_cap, which only contains the common capabilities, is
 * used to check the availability of the feature. The per-PMU
 * pebs_output_pt_available in a hybrid machine will be ignored.
 */

Thanks,
Kan
> 
>>  				pr_cont("PEBS-via-PT, ");
>>  				x86_get_pmu(smp_processor_id())->capabilities |= PERF_PMU_CAP_AUX_OUTPUT;
>>  			}
> 
>