| 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
| ||
|
Message-ID: <d7ef73fb-aa89-4f9c-d54a-7bd0562e9ea3@zhaoxin.com>
Date: Thu, 16 Apr 2020 14:26:32 +0800
From: CodyYao-oc <CodyYao-oc@...oxin.com>
To: Peter Zijlstra <peterz@...radead.org>
CC: <mingo@...hat.com>, <acme@...nel.org>, <mark.rutland@....com>,
<alexander.shishkin@...ux.intel.com>, <jolsa@...hat.com>,
<namhyung@...nel.org>, <tglx@...utronix.de>, <bp@...en8.de>,
<x86@...nel.org>, <hpa@...or.com>, <linux-kernel@...r.kernel.org>,
<cooperyan@...oxin.com>, <codyyao@...oxin.com>
Subject: Re: [PATCH] x86/perf: Add hardware performance events support for
Zhaoxin CPU.
On 2020/4/15 下午6:23, Peter Zijlstra wrote:
> On Mon, Apr 13, 2020 at 11:14:29AM +0800, CodyYao-oc wrote:
>> Zhaoxin CPU has provided facilities for monitoring performance
>> via PMU(Performance Monitor Unit), but the functionality is unused so far.
>> Therefore, add support for zhaoxin pmu to make performance related
>> hardware events available.
>>
>> Signed-off-by: CodyYao-oc <CodyYao-oc@...oxin.com>
>> Reported-by: kbuild test robot <lkp@...el.com>
>
> What's that reported-by thing? Did the robot complain you didn't have a
> PMU implementation?
>
Hi Peter, it's a warning message about uninitialized variable, paste the
log below, sorry for miss it. Futhermore, it will disappear base on your
newly modified patch. Thanks!
[All warnings (new ones prefixed by >>):
>> arch/x86/events/zhaoxin/core.c:362:6: warning: variable 'is_zxc' is
used uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
if (boot_cpu_data.x86 == 0x06 &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/events/zhaoxin/core.c:369:6: note: uninitialized use occurs
here
if (is_zxc)
^~~~~~
arch/x86/events/zhaoxin/core.c:362:2: note: remove the 'if' if its
condition is always true
if (boot_cpu_data.x86 == 0x06 &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/x86/events/zhaoxin/core.c:362:6: warning: variable 'is_zxc' is
used uninitialized whenever '&&' condition is false
[-Wsometimes-uninitialized]
if (boot_cpu_data.x86 == 0x06 &&
^~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/events/zhaoxin/core.c:369:6: note: uninitialized use occurs
here
if (is_zxc)
^~~~~~
arch/x86/events/zhaoxin/core.c:362:6: note: remove the '&&' if its
condition is always true
if (boot_cpu_data.x86 == 0x06 &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/events/zhaoxin/core.c:352:13: note: initialize the variable
'is_zxc' to silence this warning
bool is_zxc;
^
= 0
2 warnings generated.]
Reported-by: kbuild test rebot<lkp@...el.com>
> Anyway, I've made the below changes to the patch.
>
> ---
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -618,6 +618,7 @@ struct x86_pmu {
>
> /* PMI handler bits */
> unsigned int late_ack :1,
> + enabled_ack :1,
> counter_freezing :1;
> /*
> * sysfs attrs
> --- a/arch/x86/events/zhaoxin/core.c
> +++ b/arch/x86/events/zhaoxin/core.c
> @@ -357,10 +357,9 @@ static int zhaoxin_pmu_handle_irq(struct
> {
> struct perf_sample_data data;
> struct cpu_hw_events *cpuc;
> - int bit;
> - u64 status;
> - bool is_zxc = false;
> int handled = 0;
> + u64 status;
> + int bit;
>
> cpuc = this_cpu_ptr(&cpu_hw_events);
> apic_write(APIC_LVTPC, APIC_DM_NMI);
> @@ -369,14 +368,8 @@ static int zhaoxin_pmu_handle_irq(struct
> if (!status)
> goto done;
>
> - if (boot_cpu_data.x86 == 0x06 &&
> - (boot_cpu_data.x86_model == 0x0f ||
> - boot_cpu_data.x86_model == 0x19))
> - is_zxc = true;
> again:
> -
> - /*Clearing status works only if the global control is enable on zxc.*/
> - if (is_zxc)
> + if (x86_pmu.enabled_ack)
> zxc_pmu_ack_status(status);
> else
> zhaoxin_pmu_ack_status(status);
> @@ -504,12 +497,10 @@ static __init void zhaoxin_arch_events_q
> int bit;
>
> /* disable event that reported as not presend by cpuid */
> - for_each_set_bit(bit, x86_pmu.events_mask,
> - ARRAY_SIZE(zx_arch_events_map)) {
> -
> + for_each_set_bit(bit, x86_pmu.events_mask, ARRAY_SIZE(zx_arch_events_map)) {
> zx_pmon_event_map[zx_arch_events_map[bit].id] = 0;
> pr_warn("CPUID marked event: \'%s\' unavailable\n",
> - zx_arch_events_map[bit].name);
> + zx_arch_events_map[bit].name);
> }
> }
>
> @@ -534,12 +525,12 @@ __init int zhaoxin_pmu_init(void)
> return -ENODEV;
>
> version = eax.split.version_id;
> - if (version == 2) {
> - x86_pmu = zhaoxin_pmu;
> - pr_info("Version check pass!\n");
> - } else
> + if (version != 2)
> return -ENODEV;
>
> + x86_pmu = zhaoxin_pmu;
> + pr_info("Version check pass!\n");
> +
> x86_pmu.version = version;
> x86_pmu.num_counters = eax.split.num_counters;
> x86_pmu.cntval_bits = eax.split.bit_width;
> @@ -552,11 +543,13 @@ __init int zhaoxin_pmu_init(void)
>
> switch (boot_cpu_data.x86) {
> case 0x06:
> - if (boot_cpu_data.x86_model == 0x0f ||
> - boot_cpu_data.x86_model == 0x19) {
> + if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
>
> x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
>
> + /* Clearing status works only if the global control is enable on zxc. */
> + x86_pmu.enabled_ack = 1;
> +
> x86_pmu.event_constraints = zxc_event_constraints;
> zx_pmon_event_map[PERF_COUNT_HW_INSTRUCTIONS] = 0;
> zx_pmon_event_map[PERF_COUNT_HW_CACHE_REFERENCES] = 0;
> @@ -564,40 +557,37 @@ __init int zhaoxin_pmu_init(void)
> zx_pmon_event_map[PERF_COUNT_HW_BUS_CYCLES] = 0;
>
> pr_cont("ZXC events, ");
> - } else
> - return -ENODEV;
> - break;
> + break;
> + }
> + return -ENODEV;
> +
> case 0x07:
> zx_pmon_event_map[PERF_COUNT_HW_STALLED_CYCLES_FRONTEND] =
> - X86_CONFIG(.event = 0x01, .umask = 0x01, .inv = 0x01, .cmask = 0x01);
> + X86_CONFIG(.event = 0x01, .umask = 0x01, .inv = 0x01, .cmask = 0x01);
>
> zx_pmon_event_map[PERF_COUNT_HW_STALLED_CYCLES_BACKEND] =
> - X86_CONFIG(.event = 0x0f, .umask = 0x04, .inv = 0, .cmask = 0);
> + X86_CONFIG(.event = 0x0f, .umask = 0x04, .inv = 0, .cmask = 0);
>
> switch (boot_cpu_data.x86_model) {
> case 0x1b:
> memcpy(hw_cache_event_ids, zxd_hw_cache_event_ids,
> - sizeof(hw_cache_event_ids));
> + sizeof(hw_cache_event_ids));
>
> x86_pmu.event_constraints = zxd_event_constraints;
>
> - zx_pmon_event_map[PERF_COUNT_HW_BRANCH_INSTRUCTIONS]
> - = 0x0700;
> - zx_pmon_event_map[PERF_COUNT_HW_BRANCH_MISSES]
> - = 0x0709;
> + zx_pmon_event_map[PERF_COUNT_HW_BRANCH_INSTRUCTIONS] = 0x0700;
> + zx_pmon_event_map[PERF_COUNT_HW_BRANCH_MISSES] = 0x0709;
>
> pr_cont("ZXD events, ");
> break;
> case 0x3b:
> memcpy(hw_cache_event_ids, zxe_hw_cache_event_ids,
> - sizeof(hw_cache_event_ids));
> + sizeof(hw_cache_event_ids));
>
> x86_pmu.event_constraints = zxd_event_constraints;
>
> - zx_pmon_event_map[PERF_COUNT_HW_BRANCH_INSTRUCTIONS]
> - = 0x0028;
> - zx_pmon_event_map[PERF_COUNT_HW_BRANCH_MISSES]
> - = 0x0029;
> + zx_pmon_event_map[PERF_COUNT_HW_BRANCH_INSTRUCTIONS] = 0x0028;
> + zx_pmon_event_map[PERF_COUNT_HW_BRANCH_MISSES] = 0x0029;
>
> pr_cont("ZXE events, ");
> break;
> @@ -605,13 +595,13 @@ __init int zhaoxin_pmu_init(void)
> return -ENODEV;
> }
> break;
> +
> default:
> return -ENODEV;
> }
>
> x86_pmu.intel_ctrl = (1 << (x86_pmu.num_counters)) - 1;
> - x86_pmu.intel_ctrl |=
> - ((1LL << x86_pmu.num_counters_fixed)-1) << INTEL_PMC_IDX_FIXED;
> + x86_pmu.intel_ctrl |= ((1LL << x86_pmu.num_counters_fixed)-1) << INTEL_PMC_IDX_FIXED;
>
> if (x86_pmu.event_constraints) {
> for_each_event_constraint(c, x86_pmu.event_constraints) {
> --- a/arch/x86/kernel/cpu/perfctr-watchdog.c
> +++ b/arch/x86/kernel/cpu/perfctr-watchdog.c
> @@ -63,6 +63,7 @@ static inline unsigned int nmi_perfctr_m
> case 15:
> return msr - MSR_P4_BPU_PERFCTR0;
> }
> + fallthrough;
> case X86_VENDOR_ZHAOXIN:
> case X86_VENDOR_CENTAUR:
> return msr - MSR_ARCH_PERFMON_PERFCTR0;
> @@ -95,6 +96,7 @@ static inline unsigned int nmi_evntsel_m
> case 15:
> return msr - MSR_P4_BSU_ESCR0;
> }
> + fallthrough;
> case X86_VENDOR_ZHAOXIN:
> case X86_VENDOR_CENTAUR:
> return msr - MSR_ARCH_PERFMON_EVENTSEL0;
> .
>
Powered by blists - more mailing lists