| 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: <p73myzlcb2b.fsf@bingen.suse.de>
Date: 31 May 2007 16:38:52 +0200
From: Andi Kleen <andi@...stfloor.org>
To: Stephane Eranian <eranian@...nkl.hpl.hp.com>
Cc: linux-kernel@...r.kernel.org, eranian@....hp.com
Subject: Re: [PATCH 20/22] 2.6.22-rc3 perfmon2 : new x86_64 files
Stephane Eranian <eranian@...nkl.hpl.hp.com> writes:
> + /* test IA32_PMC0, IA32_PMC1 */
> + for (i=4; i < 6; i++) {
> + if (avail_to_resrv_perfctr_nmi(pfm_core_pmd_desc[i].hw_addr))
> + continue;
> + if (disabled != -1) {
> + PFM_INFO("more than one counter used by NMI, not supported");
> + return -1;
> + }
> + PFM_INFO("NMI watchdog using %s/%s, disabling them for perfmon",
There could be other users of perfctrs too, message is a bit misleading.
And why are multiple reserved counters not supported?
> + pfm_core_pmd_desc[i].desc,
> + pfm_core_pmc_desc[i].desc);
> +
> + pfm_core_pmc_desc[i].type = PFM_REG_NA;
> + pfm_core_pmu_info.pmc_addrs[i].reg_type = PFM_REGT_EN;
> +
> + pfm_core_pmd_desc[i].type = PFM_REG_NA;
> + pfm_core_pmu_info.pmd_addrs[i].reg_type = PFM_REGT_NA;
> + disabled = i;
> + }
> + /*
> + * if NMI uses IA32_PMC0 or IA32_PMC1, we cannot use global controls
> + * to start stop all counters. We disabled the global controls and
> + * mark generic counters as each having enbale bits
> + */
Hmm you're trying to detect what the main kernel does? But if it's
merged it should know. So trying to detect it dynamically doesn't
seem appropiate.
> +static int pfm_core_probe_pmu(void)
> +{
> + int ret;
> +
> + /*
> + * only works on Intel processors
> + */
> + if (cpu_data->x86_vendor != X86_VENDOR_INTEL) {
> + PFM_INFO("not running on Intel processor");
> + return -1;
> + }
> +
> + if (cpu_data->x86 != 6 || cpu_data->x86_model != 15)
> + return -1;
It seems a bit ingenious that when Intel does all the trouble
to define cpuid bits for them and then you do such checks anyways.
There should be probably two levels: simple support for the
architected counters using only cpuid capability bits
and then an exnteded check for known models etc
(probably table driven)
> +#ifdef CONFIG_SMP
> + proc_id = topology_physical_package_id(smp_processor_id());
> +#else
> + proc_id = 0;
> +#endif
That's not necessarily true for !SMP. e.g. consider the kdump
case.
> +
> + if (ctx->flags.system)
> + entry = &pfm_nb_sys_owners[proc_id];
> + else
> + entry = &pfm_nb_task_owner;
> +
> + old = cmpxchg(entry, NULL, ctx);
Non blocking way to aquire a resource? Looks unreliable.
> +/*
> + * detect if we need to active NorthBridge event access control
> + */
Can you please explain what this complex northbridge access control
logic is good for? There are a few shared counters, but normally
this can be easier handled by limitting access to one of the cores.
I suspect this could be done much simpler.
> +static int pfm_k8_setup_nb_event_control(void)
> +{
> + unsigned int c, n = 0;
> + unsigned int max_phys = 0;
> +
> +#ifdef CONFIG_SMP
> + for_each_present_cpu(c) {
possible cpus
> + if (cpu_data[c].phys_proc_id > max_phys)
> + max_phys = cpu_data[c].phys_proc_id;
But we don't necessarily know the phys_proc_id of all possible CPUs.
So that seems broken for the cpu hotplug case.
> + /*
> + * assume NMI watchdog is initialized before PMU description module
> + * auto-detect which perfctr/eventsel is used by NMI watchdog
> + */
See earlier comment for core2
> +
> + if (current_cpu_data.x86 != 15) {
> + PFM_INFO("unsupported family=%d", current_cpu_data.x86);
> + return -1;
> + }
Already obsolete with Barcelona and Griffin (16 and 17) but very similar
counters.
> --- linux-2.6.22.base/include/asm-x86_64/perfmon.h 1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.22/include/asm-x86_64/perfmon.h 2007-05-29 03:24:14.000000000 -0700
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2005-2006 Hewlett-Packard Development Company, L.P.
> + * Contributed by Stephane Eranian <eranian@....hp.com>
> + *
> + * This file contains X86-64 Processor Family specific definitions
> + * for the perfmon interface.
> + *
> + * This file MUST never be included directly. Use linux/perfmon.h.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + * 02111-1307 USA
> + */
> +#include <asm-i386/perfmon.h>
Lots of copyright for a single line
-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists