Re: [PATCH] perf/x86/intel: Restrict period on Haswell

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2024-08-16 3:27 p.m., Liang, Kan wrote:
> The HSW11 is also BDM11. It sounds like we need the trick from both bdw
> and nhm.
> 
> How about this?
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index e8bd45556c30..42f557a128b9 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4664,6 +4664,12 @@ static void nhm_limit_period(struct perf_event
> *event, s64 *left)
>  	*left = max(*left, 32LL);
>  }
> 
> +static void hsw_limit_period(struct perf_event *event, s64 *left)
> +{
> +	nhm_limit_period(event, left);


Sigh, apparently, I used an old specification update (Rev 003) for HSW.
It claims that the BDM55 is also applied to HSW (HSW75).
https://www.mouser.com/pdfdocs/xeone31200v3specupdate.PDF
So I thought the nhm_limit_period() should be used for HSW as well.

However, a newer version (Rev 016) deleted the HSW75 for HSW.
https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e3-1200v3-spec-update-oct2016.pdf

Yes, as you suggested, something as below is required.


diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index e8bd45556c30..b22a4289553b 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4634,6 +4634,17 @@ static enum hybrid_cpu_type
adl_get_hybrid_cpu_type(void)
 	return HYBRID_INTEL_CORE;
 }

+static inline bool erratum_hsw11(struct perf_event *event)
+{
+	return (event->hw.config & INTEL_ARCH_EVENT_MASK) ==
+		X86_CONFIG(.event=0xc0, .umask=0x01);
+}
+
+static void hsw_limit_period(struct perf_event *event, s64 *left)
+{
+	*left = max(*left, erratum_hsw11(event) ? 128 : 32);
+}
+
 /*
  * Broadwell:
  *
@@ -4651,8 +4662,7 @@ static enum hybrid_cpu_type
adl_get_hybrid_cpu_type(void)
  */
 static void bdw_limit_period(struct perf_event *event, s64 *left)
 {
-	if ((event->hw.config & INTEL_ARCH_EVENT_MASK) ==
-			X86_CONFIG(.event=0xc0, .umask=0x01)) {
+	if (erratum_hsw11(event)) {
 		if (*left < 128)
 			*left = 128;
 		*left &= ~0x3fULL;
@@ -6821,6 +6831,7 @@ __init int intel_pmu_init(void)

 		x86_pmu.hw_config = hsw_hw_config;
 		x86_pmu.get_event_constraints = hsw_get_event_constraints;
+		x86_pmu.limit_period = hsw_limit_period;
 		x86_pmu.lbr_double_abort = true;
 		extra_attr = boot_cpu_has(X86_FEATURE_RTM) ?
 			hsw_format_attr : nhm_format_attr;
Thanks,
Kan

> +	bdw_limit_period(event, left);
> +}
>  static void glc_limit_period(struct perf_event *event, s64 *left)
>  {
>  	if (event->attr.precise_ip == 3)
> 
> Do you plan to post the "limit" patch for HSW?
> Or should I send the patch?