Re: [PATCH 2/3] perf/x86/intel: Add more Icelake CPUIDs

[Liang, Kan]

On 6/3/2019 11:47 AM, Peter Zijlstra wrote:

On Mon, Jun 03, 2019 at 06:41:21AM -0700, kan.li...@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;