| 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
| ||
|
Date: Wed, 26 Jun 2024 09:25:27 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: peterz@...radead.org, mingo@...nel.org, acme@...nel.org,
namhyung@...nel.org, adrian.hunter@...el.com,
alexander.shishkin@...ux.intel.com, linux-kernel@...r.kernel.org,
ak@...ux.intel.com, eranian@...gle.com,
Dapeng Mi <dapeng1.mi@...ux.intel.com>
Subject: Re: [PATCH V2 07/13] perf/x86/intel: Support PERFEVTSEL extension
On 2024-06-25 9:34 p.m., Ian Rogers wrote:
> On Tue, Jun 25, 2024 at 11:22 AM <kan.liang@...ux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> Two new fields (the unit mask2, and the equal flag) are added in the
>> IA32_PERFEVTSELx MSRs. They can be enumerated by the CPUID.23H.0.EBX.
>>
>> Update the config_mask in x86_pmu and x86_hybrid_pmu for the true layout
>> of the PERFEVTSEL.
>> Expose the new formats into sysfs if they are available. The umask
>> extension reuses the same format attr name "umask" as the previous
>> umask. Add umask2_show to determine/display the correct format
>> for the current machine.
>>
>> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>> Co-developed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>> arch/x86/events/intel/core.c | 69 +++++++++++++++++++++++++++++--
>> arch/x86/include/asm/perf_event.h | 4 ++
>> 2 files changed, 69 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 23e074fd25e1..9d50e1049e30 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4632,8 +4632,55 @@ PMU_FORMAT_ATTR(pc, "config:19" );
>> PMU_FORMAT_ATTR(any, "config:21" ); /* v3 + */
>> PMU_FORMAT_ATTR(inv, "config:23" );
>> PMU_FORMAT_ATTR(cmask, "config:24-31" );
>> -PMU_FORMAT_ATTR(in_tx, "config:32");
>> -PMU_FORMAT_ATTR(in_tx_cp, "config:33");
>> +PMU_FORMAT_ATTR(in_tx, "config:32" );
>> +PMU_FORMAT_ATTR(in_tx_cp, "config:33" );
>
> nit: It seems unfortunate these 2 lines change for the sake of spaces
> before the ')'. Perhaps leave unchanged.
The two lines don't follow the existing formats. It kind of ugly when
reading the code.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/intel/core.c#n4569
I just want to make the format consistent to facilitate the future code
review.
Thanks,
Kan
>
> Thanks,
> Ian
>
>> +PMU_FORMAT_ATTR(eq, "config:36" ); /* v6 + */
>> +
>> +static ssize_t umask2_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *page)
>> +{
>> + u64 mask = hybrid(dev_get_drvdata(dev), config_mask) & ARCH_PERFMON_EVENTSEL_UMASK2;
>> +
>> + if (mask == ARCH_PERFMON_EVENTSEL_UMASK2)
>> + return sprintf(page, "config:8-15,40-47\n");
>> +
>> + /* Roll back to the old format if umask2 is not supported. */
>> + return sprintf(page, "config:8-15\n");
>> +}
>> +
>> +static struct device_attribute format_attr_umask2 =
>> + __ATTR(umask, 0444, umask2_show, NULL);
>> +
>> +static struct attribute *format_evtsel_ext_attrs[] = {
>> + &format_attr_umask2.attr,
>> + &format_attr_eq.attr,
>> + NULL
>> +};
>> +
>> +static umode_t
>> +evtsel_ext_is_visible(struct kobject *kobj, struct attribute *attr, int i)
>> +{
>> + struct device *dev = kobj_to_dev(kobj);
>> + u64 mask;
>> +
>> + /*
>> + * The umask and umask2 have different formats but share the
>> + * same attr name. In update mode, the previous value of the
>> + * umask is unconditionally removed before is_visible. If
>> + * umask2 format is not enumerated, it's impossible to roll
>> + * back to the old format.
>> + * Does the check in umask2_show rather than is_visible.
>> + */
>> + if (i == 0)
>> + return attr->mode;
>> +
>> + mask = hybrid(dev_get_drvdata(dev), config_mask);
>> + if (i == 1)
>> + return (mask & ARCH_PERFMON_EVENTSEL_EQ) ? attr->mode : 0;
>> +
>> + return 0;
>> +}
>>
>> static struct attribute *intel_arch_formats_attr[] = {
>> &format_attr_event.attr,
>> @@ -4786,8 +4833,14 @@ static inline bool intel_pmu_broken_perf_cap(void)
>>
>> static void update_pmu_cap(struct x86_hybrid_pmu *pmu)
>> {
>> - unsigned int sub_bitmaps = cpuid_eax(ARCH_PERFMON_EXT_LEAF);
>> - unsigned int eax, ebx, ecx, edx;
>> + unsigned int sub_bitmaps, eax, ebx, ecx, edx;
>> +
>> + cpuid(ARCH_PERFMON_EXT_LEAF, &sub_bitmaps, &ebx, &ecx, &edx);
>> +
>> + if (ebx & ARCH_PERFMON_EXT_UMASK2)
>> + pmu->config_mask |= ARCH_PERFMON_EVENTSEL_UMASK2;
>> + if (ebx & ARCH_PERFMON_EXT_EQ)
>> + pmu->config_mask |= ARCH_PERFMON_EVENTSEL_EQ;
>>
>> if (sub_bitmaps & ARCH_PERFMON_NUM_COUNTER_LEAF_BIT) {
>> cpuid_count(ARCH_PERFMON_EXT_LEAF, ARCH_PERFMON_NUM_COUNTER_LEAF,
>> @@ -5810,6 +5863,12 @@ static struct attribute_group group_format_extra_skl = {
>> .is_visible = exra_is_visible,
>> };
>>
>> +static struct attribute_group group_format_evtsel_ext = {
>> + .name = "format",
>> + .attrs = format_evtsel_ext_attrs,
>> + .is_visible = evtsel_ext_is_visible,
>> +};
>> +
>> static struct attribute_group group_default = {
>> .attrs = intel_pmu_attrs,
>> .is_visible = default_is_visible,
>> @@ -5823,6 +5882,7 @@ static const struct attribute_group *attr_update[] = {
>> &group_caps_lbr,
>> &group_format_extra,
>> &group_format_extra_skl,
>> + &group_format_evtsel_ext,
>> &group_default,
>> NULL,
>> };
>> @@ -6042,6 +6102,7 @@ static const struct attribute_group *hybrid_attr_update[] = {
>> &group_caps_gen,
>> &group_caps_lbr,
>> &hybrid_group_format_extra,
>> + &group_format_evtsel_ext,
>> &group_default,
>> &hybrid_group_cpus,
>> NULL,
>> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
>> index 400c909b8658..91b73571412f 100644
>> --- a/arch/x86/include/asm/perf_event.h
>> +++ b/arch/x86/include/asm/perf_event.h
>> @@ -32,6 +32,8 @@
>> #define ARCH_PERFMON_EVENTSEL_INV (1ULL << 23)
>> #define ARCH_PERFMON_EVENTSEL_CMASK 0xFF000000ULL
>> #define ARCH_PERFMON_EVENTSEL_BR_CNTR (1ULL << 35)
>> +#define ARCH_PERFMON_EVENTSEL_EQ (1ULL << 36)
>> +#define ARCH_PERFMON_EVENTSEL_UMASK2 (0xFFULL << 40)
>>
>> #define INTEL_FIXED_BITS_MASK 0xFULL
>> #define INTEL_FIXED_BITS_STRIDE 4
>> @@ -185,6 +187,8 @@ union cpuid10_edx {
>> * detection/enumeration details:
>> */
>> #define ARCH_PERFMON_EXT_LEAF 0x00000023
>> +#define ARCH_PERFMON_EXT_UMASK2 0x1
>> +#define ARCH_PERFMON_EXT_EQ 0x2
>> #define ARCH_PERFMON_NUM_COUNTER_LEAF_BIT 0x1
>> #define ARCH_PERFMON_NUM_COUNTER_LEAF 0x1
>>
>> --
>> 2.35.1
>>
Powered by blists - more mailing lists