Re: [PATCH v3] perf/x86: use hexidecimal value for cpuid

From: Liang, Kan
Date: Fri Mar 31 2023 - 09:26:48 EST

Next message: Sakari Ailus: "Re: [PATCH v7 10/13] media: ti: Add CSI2RX support for J721E"
Previous message: Juergen Gross: "Re: [PATCH v4 09/12] x86/mtrr: construct a memory map with cache modes"
In reply to: Zhenyu Wang: "Re: [PATCH v3] perf/x86: use hexidecimal value for cpuid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2023-03-30 8:43 p.m., Zhenyu Wang wrote:
>
> On 2023.03.24 09:33:19 -0400, Liang, Kan wrote:
>>
>
> sorry that I missed this one in my inbox...
>
>> On 2023-03-22 1:37 a.m., Zhenyu Wang wrote:
>>> It's easier to use hexidecimal value instead of decimal for reading
>>> and following with SDM doc, also align with other cpuid calls. As new
>>> cpuid leaf would be added in future, this tries to convert current
>>> forms and would take it as convention for new leaf definition.
>>>
>>> This changes name for both cpuid type and cpuid calls.
>>
>> It seems the patch touches 3 CPUIDs, 0xa, 0x1c and 0x14, right?
>> The patch also crosses several different subsystems and drivers.
>> I think it may be better to divide the patch by CPUID. Each patch to
>> handle one CPUID. It's easier for review.
>
> ok, I can do that.
>
>>
>> Also, can you please add a macro for the CPUID leaf number?
>> Please refer ARCH_PERFMON_EXT_LEAF (0x23).
>>
>
> As originally the purpose of this change is to use hex value in cpuid
> call and struct name, good to see that use for 0x23. If define every
> macro for these, e.g ARCH_PERFMON_LEAF(0xa), PT_LEAF(0x14),
> LBR_LEAF(0x1c), struct name needs change too? As in context of what
> source file you're reading, you already get idea what these cpuid
> numbers are for what kind of leaf...
>

No, only the hex value is good enough for an union name.

What I want is a consistent style for the leaf definition of the entire
X86 perf code.
For a union, e.g., cpuid_$hex_eax
For the leaf, e.g., #define __meaning_name_macro __hex

I think AMD has already done it. See EXT_PERFMON_DEBUG_FEATURES and
union cpuid_0x80000022_ebx.
If we have the same style, the code style will be consistent.

Thanks,
Kan

>>
>>>
>>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>>> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>>> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>>> Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>> Cc: CodyYao-oc <CodyYao-oc@xxxxxxxxxxx>
>>> Signed-off-by: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
>>> ---
>>> v3:
>>> - add more explanation in commit message for purpose of this
>>> - use lowercase number in call to align with current code
>>>
>>> v2:
>>> - rename in cpuid data type as well
>>>
>>> arch/x86/events/intel/core.c | 10 +++++-----
>>> arch/x86/events/intel/lbr.c | 8 ++++----
>>> arch/x86/events/intel/pt.c | 2 +-
>>> arch/x86/events/zhaoxin/core.c | 8 ++++----
>>> arch/x86/include/asm/perf_event.h | 12 ++++++------
>>> arch/x86/kvm/cpuid.c | 4 ++--
>>> arch/x86/kvm/vmx/pmu_intel.c | 4 ++--
>>> 7 files changed, 24 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>> index a3fb996a86a1..5487a39d4975 100644
>>> --- a/arch/x86/events/intel/core.c
>>> +++ b/arch/x86/events/intel/core.c
>>> @@ -5170,7 +5170,7 @@ static __init void intel_arch_events_quirk(void)
>>>
>>> static __init void intel_nehalem_quirk(void)
>>> {
>>> - union cpuid10_ebx ebx;
>>> + union cpuid_0xa_ebx ebx;
>>>
>>> ebx.full = x86_pmu.events_maskl;
>>> if (ebx.split.no_branch_misses_retired) {
>>> @@ -5878,9 +5878,9 @@ __init int intel_pmu_init(void)
>>> struct attribute **td_attr = &empty_attrs;
>>> struct attribute **mem_attr = &empty_attrs;
>>> struct attribute **tsx_attr = &empty_attrs;
>>> - union cpuid10_edx edx;
>>> - union cpuid10_eax eax;
>>> - union cpuid10_ebx ebx;
>>> + union cpuid_0xa_edx edx;
>>> + union cpuid_0xa_eax eax;
>>> + union cpuid_0xa_ebx ebx;
>>> unsigned int fixed_mask;
>>> bool pmem = false;
>>> int version, i;
>>> @@ -5903,7 +5903,7 @@ __init int intel_pmu_init(void)
>>> * Check whether the Architectural PerfMon supports
>>> * Branch Misses Retired hw_event or not.
>>> */
>>> - cpuid(10, &eax.full, &ebx.full, &fixed_mask, &edx.full);
>>> + cpuid(0xa, &eax.full, &ebx.full, &fixed_mask, &edx.full);
>>> if (eax.split.mask_length < ARCH_PERFMON_EVENTS_COUNT)
>>> return -ENODEV;
>>>
>>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>>> index c3b0d15a9841..616a6904af03 100644
>>> --- a/arch/x86/events/intel/lbr.c
>>> +++ b/arch/x86/events/intel/lbr.c
>>> @@ -1497,16 +1497,16 @@ static bool is_arch_lbr_xsave_available(void)
>>> void __init intel_pmu_arch_lbr_init(void)
>>> {
>>> struct pmu *pmu = x86_get_pmu(smp_processor_id());
>>> - union cpuid28_eax eax;
>>> - union cpuid28_ebx ebx;
>>> - union cpuid28_ecx ecx;
>>> + union cpuid_0x1c_eax eax;
>>> + union cpuid_0x1c_ebx ebx;
>>> + union cpuid_0x1c_ecx ecx;
>>> unsigned int unused_edx;
>>> bool arch_lbr_xsave;
>>> size_t size;
>>> u64 lbr_nr;
>>>
>>> /* Arch LBR Capabilities */
>>> - cpuid(28, &eax.full, &ebx.full, &ecx.full, &unused_edx);
>>> + cpuid(0x1c, &eax.full, &ebx.full, &ecx.full, &unused_edx);
>>>
>>> lbr_nr = fls(eax.split.lbr_depth_mask) * 8;
>>> if (!lbr_nr)
>>> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
>>> index 42a55794004a..da3c5d748365 100644
>>> --- a/arch/x86/events/intel/pt.c
>>> +++ b/arch/x86/events/intel/pt.c
>>> @@ -235,7 +235,7 @@ static int __init pt_pmu_hw_init(void)
>>> }
>>>
>>> for (i = 0; i < PT_CPUID_LEAVES; i++) {
>>> - cpuid_count(20, i,
>>> + cpuid_count(0x14, i,
>>> &pt_pmu.caps[CPUID_EAX + i*PT_CPUID_REGS_NUM],
>>> &pt_pmu.caps[CPUID_EBX + i*PT_CPUID_REGS_NUM],
>>> &pt_pmu.caps[CPUID_ECX + i*PT_CPUID_REGS_NUM],
>>> diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
>>> index 3e9acdaeed1e..1d071974f4db 100644
>>> --- a/arch/x86/events/zhaoxin/core.c
>>> +++ b/arch/x86/events/zhaoxin/core.c
>>> @@ -504,9 +504,9 @@ static __init void zhaoxin_arch_events_quirk(void)
>>>
>>> __init int zhaoxin_pmu_init(void)
>>> {
>>> - union cpuid10_edx edx;
>>> - union cpuid10_eax eax;
>>> - union cpuid10_ebx ebx;
>>> + union cpuid_0xa_edx edx;
>>> + union cpuid_0xa_eax eax;
>>> + union cpuid_0xa_ebx ebx;
>>> struct event_constraint *c;
>>> unsigned int unused;
>>> int version;
>>> @@ -517,7 +517,7 @@ __init int zhaoxin_pmu_init(void)
>>> * Check whether the Architectural PerfMon supports
>>> * hw_event or not.
>>> */
>>> - cpuid(10, &eax.full, &ebx.full, &unused, &edx.full);
>>> + cpuid(0xa, &eax.full, &ebx.full, &unused, &edx.full);
>>>
>>> if (eax.split.mask_length < ARCH_PERFMON_EVENTS_COUNT - 1)
>>> return -ENODEV;
>>> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
>>> index 8fc15ed5e60b..0d2d735c8167 100644
>>> --- a/arch/x86/include/asm/perf_event.h
>>> +++ b/arch/x86/include/asm/perf_event.h
>>> @@ -125,7 +125,7 @@
>>> * Intel "Architectural Performance Monitoring" CPUID
>>> * detection/enumeration details:
>>> */
>>> -union cpuid10_eax {
>>> +union cpuid_0xa_eax {
>>> struct {
>>> unsigned int version_id:8;
>>> unsigned int num_counters:8;
>>> @@ -135,7 +135,7 @@ union cpuid10_eax {
>>> unsigned int full;
>>> };
>>>
>>> -union cpuid10_ebx {
>>> +union cpuid_0xa_ebx {
>>> struct {
>>> unsigned int no_unhalted_core_cycles:1;
>>> unsigned int no_instructions_retired:1;
>>> @@ -148,7 +148,7 @@ union cpuid10_ebx {
>>> unsigned int full;
>>> };
>>>
>>> -union cpuid10_edx {
>>> +union cpuid_0xa_edx {
>>> struct {
>>> unsigned int num_counters_fixed:5;
>>> unsigned int bit_width_fixed:8;
>>> @@ -170,7 +170,7 @@ union cpuid10_edx {
>>> /*
>>> * Intel Architectural LBR CPUID detection/enumeration details:
>>> */
>>> -union cpuid28_eax {
>>> +union cpuid_0x1c_eax {
>>> struct {
>>> /* Supported LBR depth values */
>>> unsigned int lbr_depth_mask:8;
>>> @@ -183,7 +183,7 @@ union cpuid28_eax {
>>> unsigned int full;
>>> };
>>>
>>> -union cpuid28_ebx {
>>> +union cpuid_0x1c_ebx {
>>> struct {
>>> /* CPL Filtering Supported */
>>> unsigned int lbr_cpl:1;
>>> @@ -195,7 +195,7 @@ union cpuid28_ebx {
>>> unsigned int full;
>>> };
>>>
>>> -union cpuid28_ecx {
>>> +union cpuid_0x1c_ecx {
>>> struct {
>>> /* Mispredict Bit Supported */
>>> unsigned int lbr_mispred:1;
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 599aebec2d52..57f43dc87538 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -967,8 +967,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>> }
>>> break;
>>> case 0xa: { /* Architectural Performance Monitoring */
>>> - union cpuid10_eax eax;
>>> - union cpuid10_edx edx;
>>> + union cpuid_0xa_eax eax;
>>> + union cpuid_0xa_edx edx;
>>>
>>> if (!static_cpu_has(X86_FEATURE_ARCH_PERFMON)) {
>>> entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>>> index e8a3be0b9df9..f4b165667ca9 100644
>>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>>> @@ -512,8 +512,8 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>>> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>>> struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
>>> struct kvm_cpuid_entry2 *entry;
>>> - union cpuid10_eax eax;
>>> - union cpuid10_edx edx;
>>> + union cpuid_0xa_eax eax;
>>> + union cpuid_0xa_edx edx;
>>> u64 perf_capabilities;
>>> u64 counter_mask;
>>> int i;

Next message: Sakari Ailus: "Re: [PATCH v7 10/13] media: ti: Add CSI2RX support for J721E"
Previous message: Juergen Gross: "Re: [PATCH v4 09/12] x86/mtrr: construct a memory map with cache modes"
In reply to: Zhenyu Wang: "Re: [PATCH v3] perf/x86: use hexidecimal value for cpuid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]