Date: Tue, 10 Mar 2026 22:20:26 +0000 From: Warner Losh <imp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Cc: Ali Mashtizadeh <ali@mashtizadeh.com> Subject: git: bfb2fd5f6618 - main - libpmc: Explicitly whitelist json fields Message-ID: <69b0992a.3f8e0.623c0715@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=bfb2fd5f66183454cfe8771595df09c0f23c7efb commit bfb2fd5f66183454cfe8771595df09c0f23c7efb Author: Ali Mashtizadeh <ali@mashtizadeh.com> AuthorDate: 2026-02-28 20:45:27 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2026-03-10 22:20:17 +0000 libpmc: Explicitly whitelist json fields Adds all missing Intel fields and turns jevents.c into an explicit white list mechanism so that we no longer ignore important fields that often invalidate the counter. The json event parser must now parse every field on each architecture that we support. This has been tested by running tinderbox and manually running jevent against our current json repository. As a bonus I fixed spelling errors in the AMD JSON definitions. Sponsored by: Netflix Reviewed by: imp Pull Request: https://github.com/freebsd/freebsd-src/pull/2055 --- lib/libpmc/libpmc.c | 5 +- lib/libpmc/libpmc_pmu_util.c | 19 +++ lib/libpmc/pmu-events/arch/x86/amdzen4/cache.json | 2 +- .../pmu-events/arch/x86/amdzen5/load-store.json | 2 +- .../pmu-events/arch/x86/amdzen6/load-store.json | 2 +- lib/libpmc/pmu-events/jevents.c | 129 ++++++++++++++++----- 6 files changed, 126 insertions(+), 33 deletions(-) diff --git a/lib/libpmc/libpmc.c b/lib/libpmc/libpmc.c index 155da7cf6a7b..6be91a7501fe 100644 --- a/lib/libpmc/libpmc.c +++ b/lib/libpmc/libpmc.c @@ -1121,8 +1121,11 @@ pmc_allocate(const char *ctrspec, enum pmc_mode mode, r = spec_copy = strdup(ctrspec); ctrname = strsep(&r, ","); if (pmc_pmu_enabled()) { - if (pmc_pmu_pmcallocate(ctrname, &pmc_config) == 0) + errno = pmc_pmu_pmcallocate(ctrname, &pmc_config); + if (errno == 0) goto found; + if (errno == EOPNOTSUPP) + goto out; } free(spec_copy); spec_copy = NULL; diff --git a/lib/libpmc/libpmc_pmu_util.c b/lib/libpmc/libpmc_pmu_util.c index 0832ab32e2f1..5db60fe6dafe 100644 --- a/lib/libpmc/libpmc_pmu_util.c +++ b/lib/libpmc/libpmc_pmu_util.c @@ -241,6 +241,7 @@ struct pmu_event_desc { uint8_t ped_edge; uint8_t ped_fc_mask; uint8_t ped_ch_mask; + uint8_t ped_pebs; }; static const struct pmu_events_map * @@ -377,6 +378,8 @@ pmu_parse_event(struct pmu_event_desc *ped, const char *eventin) ped->ped_allcores = strtol(value, NULL, 10); else if (strcmp(key, "allsources") == 0) ped->ped_allsources = strtol(value, NULL, 10); + else if (strcmp(key, "pebs") == 0) + ped->ped_pebs = strtol(value, NULL, 10); else { debug = getenv("PMUDEBUG"); if (debug != NULL && strcmp(debug, "true") == 0 && value != NULL) @@ -487,6 +490,8 @@ pmc_pmu_print_counter_full(const char *ev) continue; if (strcasestr(pe->name, ev) == NULL) continue; + if (pe != pme->table) + printf("\n"); printf("name: %s\n", pe->name); if (pe->long_desc != NULL) printf("desc: %s\n", pe->long_desc); @@ -603,6 +608,7 @@ pmc_pmu_intel_pmcallocate(const char *event_name, struct pmc_op_pmcallocate *pm, strcasestr(event_name, "uncore") != NULL) { pm->pm_class = PMC_CLASS_UCP; pm->pm_caps |= PMC_CAP_QUALIFIER; + /* XXX Check and block unsupported uncore counters */ } else if (ped->ped_event == 0x0) { pm->pm_class = PMC_CLASS_IAF; } else { @@ -631,6 +637,19 @@ pmc_pmu_intel_pmcallocate(const char *event_name, struct pmc_op_pmcallocate *pm, iap->pm_iap_config |= IAP_INV; if (pm->pm_caps & PMC_CAP_INTERRUPT) iap->pm_iap_config |= IAP_INT; + + /* XXX Implement alone flag in the kernel module */ + + /* + * PEBS counters are not implemented on FreeBSD yet. Counters labeled + * with PEBS = 2 in the JSON definitions require PEBS. It seems that + * some of them return bogus counts if you attempt to use them. + */ + if (ped->ped_pebs == 2) { + printf("PEBS only counters are not supported!\n"); + return (EOPNOTSUPP); + } + return (0); } diff --git a/lib/libpmc/pmu-events/arch/x86/amdzen4/cache.json b/lib/libpmc/pmu-events/arch/x86/amdzen4/cache.json index e6d710cf3ce2..01c848a9dbb3 100644 --- a/lib/libpmc/pmu-events/arch/x86/amdzen4/cache.json +++ b/lib/libpmc/pmu-events/arch/x86/amdzen4/cache.json @@ -182,7 +182,7 @@ { "EventName": "ls_inef_sw_pref.all", "EventCode": "0x52", - "BriefDescript6ion": "Software prefetches that did not fetch data outside of the processor core for any reason.", + "BriefDescription": "Software prefetches that did not fetch data outside of the processor core for any reason.", "UMask": "0x03" }, { diff --git a/lib/libpmc/pmu-events/arch/x86/amdzen5/load-store.json b/lib/libpmc/pmu-events/arch/x86/amdzen5/load-store.json index 06bbaea15925..f36b58031d4b 100644 --- a/lib/libpmc/pmu-events/arch/x86/amdzen5/load-store.json +++ b/lib/libpmc/pmu-events/arch/x86/amdzen5/load-store.json @@ -345,7 +345,7 @@ { "EventName": "ls_inef_sw_pref.all", "EventCode": "0x52", - "BriefDescript6ion": "Software prefetches that did not fetch data outside of the processor core for any reason.", + "BriefDescription": "Software prefetches that did not fetch data outside of the processor core for any reason.", "UMask": "0x03" }, { diff --git a/lib/libpmc/pmu-events/arch/x86/amdzen6/load-store.json b/lib/libpmc/pmu-events/arch/x86/amdzen6/load-store.json index 4291eb59426f..4adba70bffb1 100644 --- a/lib/libpmc/pmu-events/arch/x86/amdzen6/load-store.json +++ b/lib/libpmc/pmu-events/arch/x86/amdzen6/load-store.json @@ -351,7 +351,7 @@ { "EventName": "ls_inef_sw_pref.all", "EventCode": "0x52", - "BriefDescript6ion": "Software prefetches that did not fetch data outside of the processor core for any reason.", + "BriefDescription": "Software prefetches that did not fetch data outside of the processor core for any reason.", "UMask": "0x03" }, { diff --git a/lib/libpmc/pmu-events/jevents.c b/lib/libpmc/pmu-events/jevents.c index 4e4f53a0c0d0..25119cd3c7b8 100644 --- a/lib/libpmc/pmu-events/jevents.c +++ b/lib/libpmc/pmu-events/jevents.c @@ -242,27 +242,33 @@ static struct msrmap *lookup_msr(char *map, jsmntok_t *val) return NULL; } +/* + * Converts the unit names to add a prefix used to identify the counter class + * in other parts of libpmc. The fallback path for unknown units prefixes the + * unit with "uncore_". + */ static struct map { const char *json; const char *perf; } unit_to_pmu[] = { + /* Intel */ + { "cpu_core", "cpu_core" }, + { "cpu_atom", "cpu_atom" }, { "CBO", "uncore_cbox" }, { "QPI LL", "uncore_qpi" }, { "SBO", "uncore_sbox" }, { "iMPH-U", "uncore_arb" }, - { "CPU-M-CF", "cpum_cf" }, - { "CPU-M-SF", "cpum_sf" }, { "UPI LL", "uncore_upi" }, + /* AMD */ + { "L3PMC", "amd_l3" }, + { "DFPMC", "amd_df" }, + /* ARM HiSilicon */ { "hisi_sicl,cpa", "hisi_sicl,cpa"}, { "hisi_sccl,ddrc", "hisi_sccl,ddrc" }, { "hisi_sccl,hha", "hisi_sccl,hha" }, { "hisi_sccl,l3c", "hisi_sccl,l3c" }, - /* it's not realistic to keep adding these, we need something more scalable ... */ + /* ARM FreeScale */ { "imx8_ddr", "imx8_ddr" }, - { "L3PMC", "amd_l3" }, - { "DFPMC", "amd_df" }, - { "cpu_core", "cpu_core" }, - { "cpu_atom", "cpu_atom" }, {} }; @@ -586,14 +592,24 @@ static int json_events(const char *fn, "Expected string value"); nz = !json_streq(map, val, "0"); - /* match_field */ - if (json_streq(map, field, "UMask") && nz) { - addfield(map, &umask, "", "umask=", val); - } else if (json_streq(map, field, "EnAllCores") && nz) { + /* + * Match the field against known fields. This list is + * an explicit whitelist so that the build will break + * if we add new json definitions with unimplemented + * fields. If a field may contain a zero value that + * results in ignoring the field, do not check nz in + * the top level conditional statement as it will + * result in executing the else clause that reports an + * error. + */ + if (json_streq(map, field, "UMask")) { + if (nz) + addfield(map, &umask, "", "umask=", val); + } else if (json_streq(map, field, "EnAllCores")) { addfield(map, &allcores, "", "allcores=", val); - } else if (json_streq(map, field, "EnAllSlices") && nz) { + } else if (json_streq(map, field, "EnAllSlices")) { addfield(map, &allslices, "", "allslices=", val); - } else if (json_streq(map, field, "SliceId") && nz) { + } else if (json_streq(map, field, "SliceId")) { /* * We use sourceid because there's a * descripency where the JSON from linux calls @@ -603,18 +619,26 @@ static int json_events(const char *fn, * the references in hwpmc_amd.h. */ addfield(map, &sliceid, "", "sourceid=", val); - } else if (json_streq(map, field, "ThreadMask") && nz) { - addfield(map, &threadmask, "", "l3_thread_mask=", val); - } else if (json_streq(map, field, "CounterMask") && nz) { - addfield(map, &cmask, "", "cmask=", val); - } else if (json_streq(map, field, "Invert") && nz) { - addfield(map, &inv, "", "inv=", val); - } else if (json_streq(map, field, "AnyThread") && nz) { - addfield(map, &any, "", "any=", val); - } else if (json_streq(map, field, "EdgeDetect") && nz) { - addfield(map, &edge, "", "edge=", val); - } else if (json_streq(map, field, "SampleAfterValue") && nz) { - addfield(map, &period, "", "period=", val); + } else if (json_streq(map, field, "ThreadMask")) { + if (nz) + addfield(map, &threadmask, "", "l3_thread_mask=", val); + } else if (json_streq(map, field, "CounterMask")) { + if (nz) + addfield(map, &cmask, "", "cmask=", val); + } else if (json_streq(map, field, "RdWrMask")) { + /* AMD UMC */ + } else if (json_streq(map, field, "Invert")) { + if (nz) + addfield(map, &inv, "", "inv=", val); + } else if (json_streq(map, field, "AnyThread")) { + if (nz) + addfield(map, &any, "", "any=", val); + } else if (json_streq(map, field, "EdgeDetect")) { + if (nz) + addfield(map, &edge, "", "edge=", val); + } else if (json_streq(map, field, "SampleAfterValue")) { + if (nz) + addfield(map, &period, "", "period=", val); } else if (json_streq(map, field, "FCMask") && nz) { addfield(map, &fc_mask, "", "fc_mask=", val); } else if (json_streq(map, field, "PortMask") && nz) { @@ -695,22 +719,69 @@ static int json_events(const char *fn, addfield(map, &arch_std, "", "", val); for (s = arch_std; *s; s++) *s = tolower(*s); + } else if (json_streq(map, field, "Offcore")) { + /* Check the relevant MSR has been set */ + } else if (json_streq(map, field, "CounterType")) { + /* Unsupported Intel Offcore counters */ + } else if (json_streq(map, field, "UMaskExt")) { + /* Unsupported Intel Offcore counters */ + } else if (json_streq(map, field, "PDIR_COUNTER")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "CollectPEBSRecord")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "PEBScounters")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "Counter")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "CounterHTOff")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "PRECISE_STORE")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "L1_Hit_Indication")) { + /* Intel PEBS not supported */ + } else if (json_streq(map, field, "RetirementLatencyMin")) { + /* Intel TPEBS not supported */ + } else if (json_streq(map, field, "RetirementLatencyMax")) { + /* Intel TPEBS not supported */ + } else if (json_streq(map, field, "RetirementLatencyMean")) { + /* Intel TPEBS not supported */ + } else if (json_streq(map, field, "Speculative")) { + /* Intel informative */ + } else if (json_streq(map, field, "Experimental")) { + /* Intel informative */ + } else if (json_streq(map, field, "ELLC")) { + /* Intel informative */ + } else if (json_streq(map, field, "TakenAlone")) { + /* + * Do not measure with other counters, usually + * this is because it uses an MSR to filter the + * event in a way that affects other counters. + */ + if (json_streq(map, val, "1")) + addfield(map, &event, ",", "alone", NULL); } else { /* - * We shouldn't ignore unknown fields that - * makes the counter invalid! + * We shouldn't ignore unknown fields that may + * make the counter invalid! + * + * Often the JSON definitions are copied + * without checking if any fields require + * handling. */ json_copystr(map, field, buf, sizeof(buf)); fprintf(stderr, "Unknown field '%s'!\n", buf); + _Exit(1); } } if (precise && je.desc && !strstr(je.desc, "(Precise Event)")) { - if (json_streq(map, precise, "2")) + if (json_streq(map, precise, "2")) { addfield(map, &extra_desc, " ", "(Must be precise)", NULL); - else + addfield(map, &event, ",", "pebs=", precise); + } else { addfield(map, &extra_desc, " ", "(Precise event)", NULL); + } } if (configcode_present) snprintf(buf, sizeof buf, "config=%#llx", configcode);home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?69b0992a.3f8e0.623c0715>