Re: [PATCH V2 07/13] perf/x86/intel: Support PERFEVTSEL extension

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

On 2024-06-25 9:34 p.m., Ian Rogers wrote:
> On Tue, Jun 25, 2024 at 11:22 AM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.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@linux.intel.com>
>> Co-developed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Signed-off-by: Kan Liang <kan.liang@linux.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
>>