| 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: <20080208205605.GH31984@uranus.ravnborg.org>
Date: Fri, 8 Feb 2008 21:56:05 +0100
From: Sam Ravnborg <sam@...nborg.org>
To: Barry Kasindorf <barry.kasindorf@....com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] AMD Family10h IBS support for oProfile driver
>
> Signed-off-by: Barry Kasindorf <barry.kasindorf@....com>
> Signed-off-by: Mark Langsdorf <mark.langsdorf@....com>
> ---
> kernel/apic_64.c | 1
> oprofile/op_model_athlon.c | 245
> ++++++++++++++++++++++++++++++++++++++++++++-
> oprofile/op_x86_model.h | 37 ++++++
> 3 files changed, 281 insertions(+), 2 deletions(-)
> diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
> index 1e417df..7dbe8b0 100644
> --- a/arch/x86/kernel/apic_32.c
> +++ b/arch/x86/kernel/apic_32.c
> @@ -224,6 +224,30 @@ static void __setup_APIC_LVTT(unsigned int clocks, int
> oneshot, int irqen)
> if (!oneshot)
> apic_write_around(APIC_TMICT, clocks/APIC_DIVISOR);
> }
> +#define APIC_EILVT_LVTOFF_MCE 0
> +#define APIC_EILVT_LVTOFF_IBS 1
> +
> +static void setup_APIC_eilvt(u8 lvt_off, u8 vector, u8 msg_type, u8 mask)
> +{
> + unsigned long reg = (lvt_off << 4) + APIC_EILVT0;
> + unsigned int v = (mask << 16) | (msg_type << 8) | vector;
> +
> + apic_write(reg, v);
> +}
> +
> +u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask)
> +{
> + setup_APIC_eilvt(APIC_EILVT_LVTOFF_MCE, vector, msg_type, mask);
> + return APIC_EILVT_LVTOFF_MCE;
> +}
There are no users of this global symbol - can we kill it?
> +
> +u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask)
> +{
> + setup_APIC_eilvt(APIC_EILVT_LVTOFF_IBS, vector, msg_type, mask);
> + return APIC_EILVT_LVTOFF_IBS;
> +}
> +EXPORT_SYMBOL(setup_APIC_eilvt_ibs);
Please document all new EXPORT_SYMBOLS with kernel-doc comments.
GPL export?
> +
>
> /*
> * Program the next event, relative to now
> diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
> index 286a396..bced2a6 100644
> --- a/arch/x86/kernel/apic_64.c
> +++ b/arch/x86/kernel/apic_64.c
> @@ -219,6 +219,7 @@ u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask)
> setup_APIC_eilvt(APIC_EILVT_LVTOFF_IBS, vector, msg_type, mask);
> return APIC_EILVT_LVTOFF_IBS;
> }
> +EXPORT_SYMBOL(setup_APIC_eilvt_ibs);
Is this new export properly documented?
> +++ b/arch/x86/oprofile/op_model_athlon.c
> @@ -8,9 +8,13 @@
> * @author John Levon
> * @author Philippe Elie
> * @author Graydon Hoare
> - */
> + * @author Barry Kasindorf
> +*/
>
> #include <linux/oprofile.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
Alphabetic order is preferred.
> +
> #include <asm/ptrace.h>
> #include <asm/msr.h>
> #include <asm/nmi.h>
> @@ -42,8 +46,69 @@
> #define CTRL_SET_HOST_ONLY(val, h) (val |= ((h & 1) << 9))
> #define CTRL_SET_GUEST_ONLY(val, h) (val |= ((h & 1) << 8))
>
> +/* high dword IbsFetchCtl[bit 49] */
> +#define IBS_FETCH_VALID_BIT 0x00020000
Then define it as (1 << 49) ?
> + * Add an AMD IBS sample. This may be called from any context. Pass
> + * smp_processor_id() as cpu. Passes IBS registers as a unsigned int[8]
> + */
> +void oprofile_add_ibs_op_sample(struct pt_regs * const regs,
> + unsigned int * const ibs_op);
> +
> +void oprofile_add_ibs_fetch_sample(struct pt_regs * const regs,
> + unsigned int * const ibs_fetch);
> +
These prototypes looks like they belong in a .h file.
Did sparse accept them?
> static unsigned long reset_value[NUM_COUNTERS];
> -
> +extern int ibs_allowed; /* AMD Family 10h+ */
This extern certainly does belong in a .h file.
>
> +/*
> + * Enable AMD extended PCI config space thru IO
> + * save previous state
> + */
> +static void
> + Enable_Extended_PCI_Config(void)
> +{
AnySpecificReasonForPascalCasing?
And keep function definotion on one line.
> + unsigned int low, high;
> + rdmsr(NB_CFG_MSR, low, high);
> + Extended_PCI_Enabled = high & ENABLE_CF8_EXT_CFG_MASK;
> + high |= ENABLE_CF8_EXT_CFG_MASK;
> + wrmsr(NB_CFG_MSR, low, high);
> +}
> +
> +/*
> + * Disable AMD extended PCI config space thru IO
> + * restore to previous state
> + */
> +static void
> + Disable_Extended_PCI_Config(void)
A_New_Style_For_Naming???
> +{
> + unsigned int low, high;
> + rdmsr(NB_CFG_MSR, low, high);
> + high &= ~ENABLE_CF8_EXT_CFG_MASK;
> + high |= Extended_PCI_Enabled;
> + wrmsr(NB_CFG_MSR, low, high);
> +}
> +/*
> + * Modified to use AMD extended PCI config space thru IO
> + * these 2 I/Os should be atomic but there is no easy way to do that.
> + * Should use the MMio version, will when it is fixed
> + */
> +
> +static void
> + PCI_Extended_Write(struct pci_dev *dev, unsigned int offset,
> + unsigned long val)
Again.
> +
> + /**** Be sure to run loop until NULL is returned to
> + decrement reference count on any pci_dev structures returned
> ****/
USe kernel style:
/*
* Bla
*/
And resubmit without line wraps.
--
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