[2/3] perf/x86/intel: Add more Icelake CPUIDs
diff mbox series

Message ID 20190603134122.13853-2-kan.liang@linux.intel.com
State New
Headers show
Series
  • [1/3] x86/CPU: Add more Icelake model number
Related show

Commit Message

Liang, Kan June 3, 2019, 1:41 p.m. UTC
From: Kan Liang <kan.liang@linux.intel.com>

Add new model number for Icelake desktop and server to perf.

The data source encoding for Icelake server is the same as Skylake
server.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/intel/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Peter Zijlstra June 3, 2019, 3:47 p.m. UTC | #1
On Mon, Jun 03, 2019 at 06:41:21AM -0700, kan.liang@linux.intel.com wrote:
> @@ -4962,7 +4965,9 @@ __init int intel_pmu_init(void)
>  		x86_pmu.cpu_events = get_icl_events_attrs();
>  		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
>  		x86_pmu.lbr_pt_coexist = true;
> -		intel_pmu_pebs_data_source_skl(false);
> +		intel_pmu_pebs_data_source_skl(
> +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_X) ||
> +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_XEON_D));

That's pretty sad, a model switch inside a model switch :/

>  		pr_cont("Icelake events, ");
>  		name = "icelake";
>  		break;

Would something like so not be nicer?

---
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4485,6 +4485,7 @@ __init int intel_pmu_init(void)
 	struct event_constraint *c;
 	unsigned int unused;
 	struct extra_reg *er;
+	bool pmem = false;
 	int version, i;
 	char *name;
 
@@ -4936,9 +4937,10 @@ __init int intel_pmu_init(void)
 		name = "knights-landing";
 		break;
 
+	case INTEL_FAM6_SKYLAKE_X:
+		pmem = true;
 	case INTEL_FAM6_SKYLAKE_MOBILE:
 	case INTEL_FAM6_SKYLAKE_DESKTOP:
-	case INTEL_FAM6_SKYLAKE_X:
 	case INTEL_FAM6_KABYLAKE_MOBILE:
 	case INTEL_FAM6_KABYLAKE_DESKTOP:
 		x86_add_quirk(intel_pebs_isolation_quirk);
@@ -4970,8 +4972,7 @@ __init int intel_pmu_init(void)
 		td_attr  = hsw_events_attrs;
 		mem_attr = hsw_mem_events_attrs;
 		tsx_attr = hsw_tsx_events_attrs;
-		intel_pmu_pebs_data_source_skl(
-			boot_cpu_data.x86_model == INTEL_FAM6_SKYLAKE_X);
+		intel_pmu_pebs_data_source_skl(pmem);
 
 		if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
 			x86_pmu.flags |= PMU_FL_TFA;
@@ -4985,7 +4986,11 @@ __init int intel_pmu_init(void)
 		name = "skylake";
 		break;
 
+	case INTEL_FAM6_ICELAKE_X:
+	case INTEL_FAM6_ICELAKE_XEON_D:
+		pmem = true;
 	case INTEL_FAM6_ICELAKE_MOBILE:
+	case INTEL_FAM6_ICELAKE_DESKTOP:
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -5009,7 +5014,7 @@ __init int intel_pmu_init(void)
 		tsx_attr = icl_tsx_events_attrs;
 		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
 		x86_pmu.lbr_pt_coexist = true;
-		intel_pmu_pebs_data_source_skl(false);
+		intel_pmu_pebs_data_source_skl(pmem);
 		pr_cont("Icelake events, ");
 		name = "icelake";
 		break;
Liang, Kan June 3, 2019, 4:14 p.m. UTC | #2
On 6/3/2019 11:47 AM, Peter Zijlstra wrote:
> On Mon, Jun 03, 2019 at 06:41:21AM -0700, kan.liang@linux.intel.com wrote:
>> @@ -4962,7 +4965,9 @@ __init int intel_pmu_init(void)
>>   		x86_pmu.cpu_events = get_icl_events_attrs();
>>   		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
>>   		x86_pmu.lbr_pt_coexist = true;
>> -		intel_pmu_pebs_data_source_skl(false);
>> +		intel_pmu_pebs_data_source_skl(
>> +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_X) ||
>> +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_XEON_D));
> 
> That's pretty sad, a model switch inside a model switch :/
> 
>>   		pr_cont("Icelake events, ");
>>   		name = "icelake";
>>   		break;
> 
> Would something like so not be nicer?

Yes, it looks better. Thanks.

Should I combine your patch with mine, and send out V2?
Or are you prefer to add your patch on top of this patch set?

Thanks,
Kan

> 
> ---
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4485,6 +4485,7 @@ __init int intel_pmu_init(void)
>   	struct event_constraint *c;
>   	unsigned int unused;
>   	struct extra_reg *er;
> +	bool pmem = false;
>   	int version, i;
>   	char *name;
>   
> @@ -4936,9 +4937,10 @@ __init int intel_pmu_init(void)
>   		name = "knights-landing";
>   		break;
>   
> +	case INTEL_FAM6_SKYLAKE_X:
> +		pmem = true;
>   	case INTEL_FAM6_SKYLAKE_MOBILE:
>   	case INTEL_FAM6_SKYLAKE_DESKTOP:
> -	case INTEL_FAM6_SKYLAKE_X:
>   	case INTEL_FAM6_KABYLAKE_MOBILE:
>   	case INTEL_FAM6_KABYLAKE_DESKTOP:
>   		x86_add_quirk(intel_pebs_isolation_quirk);
> @@ -4970,8 +4972,7 @@ __init int intel_pmu_init(void)
>   		td_attr  = hsw_events_attrs;
>   		mem_attr = hsw_mem_events_attrs;
>   		tsx_attr = hsw_tsx_events_attrs;
> -		intel_pmu_pebs_data_source_skl(
> -			boot_cpu_data.x86_model == INTEL_FAM6_SKYLAKE_X);
> +		intel_pmu_pebs_data_source_skl(pmem);
>   
>   		if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
>   			x86_pmu.flags |= PMU_FL_TFA;
> @@ -4985,7 +4986,11 @@ __init int intel_pmu_init(void)
>   		name = "skylake";
>   		break;
>   
> +	case INTEL_FAM6_ICELAKE_X:
> +	case INTEL_FAM6_ICELAKE_XEON_D:
> +		pmem = true;
>   	case INTEL_FAM6_ICELAKE_MOBILE:
> +	case INTEL_FAM6_ICELAKE_DESKTOP:
>   		x86_pmu.late_ack = true;
>   		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
>   		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
> @@ -5009,7 +5014,7 @@ __init int intel_pmu_init(void)
>   		tsx_attr = icl_tsx_events_attrs;
>   		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
>   		x86_pmu.lbr_pt_coexist = true;
> -		intel_pmu_pebs_data_source_skl(false);
> +		intel_pmu_pebs_data_source_skl(pmem);
>   		pr_cont("Icelake events, ");
>   		name = "icelake";
>   		break;
>
Peter Zijlstra June 3, 2019, 4:30 p.m. UTC | #3
On Mon, Jun 03, 2019 at 12:14:49PM -0400, Liang, Kan wrote:
> 
> 
> On 6/3/2019 11:47 AM, Peter Zijlstra wrote:
> > On Mon, Jun 03, 2019 at 06:41:21AM -0700, kan.liang@linux.intel.com wrote:
> > > @@ -4962,7 +4965,9 @@ __init int intel_pmu_init(void)
> > >   		x86_pmu.cpu_events = get_icl_events_attrs();
> > >   		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
> > >   		x86_pmu.lbr_pt_coexist = true;
> > > -		intel_pmu_pebs_data_source_skl(false);
> > > +		intel_pmu_pebs_data_source_skl(
> > > +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_X) ||
> > > +			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_XEON_D));
> > 
> > That's pretty sad, a model switch inside a model switch :/
> > 
> > >   		pr_cont("Icelake events, ");
> > >   		name = "icelake";
> > >   		break;
> > 
> > Would something like so not be nicer?
> 
> Yes, it looks better. Thanks.
> 
> Should I combine your patch with mine, and send out V2?
> Or are you prefer to add your patch on top of this patch set?

I'll frob it. Thanks!

Patch
diff mbox series

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 546d13e436aa..c915afdaaba6 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4940,6 +4940,9 @@  __init int intel_pmu_init(void)
 		break;
 
 	case INTEL_FAM6_ICELAKE_MOBILE:
+	case INTEL_FAM6_ICELAKE_DESKTOP:
+	case INTEL_FAM6_ICELAKE_X:
+	case INTEL_FAM6_ICELAKE_XEON_D:
 		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
@@ -4962,7 +4965,9 @@  __init int intel_pmu_init(void)
 		x86_pmu.cpu_events = get_icl_events_attrs();
 		x86_pmu.rtm_abort_event = X86_CONFIG(.event=0xca, .umask=0x02);
 		x86_pmu.lbr_pt_coexist = true;
-		intel_pmu_pebs_data_source_skl(false);
+		intel_pmu_pebs_data_source_skl(
+			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_X) ||
+			(boot_cpu_data.x86_model == INTEL_FAM6_ICELAKE_XEON_D));
 		pr_cont("Icelake events, ");
 		name = "icelake";
 		break;