2008-02-08 20:06:53

by Kasindorf, Barry

[permalink] [raw]
Subject: [PATCH 1/3] AMD Family10h IBS support for oProfile driver


Signed-off-by: Barry Kasindorf <[email protected]>
Signed-off-by: Mark Langsdorf <[email protected]>
---
nmi_int.c | 43 +++++++++++++++++++++++++++++++++++++++++--
op_counter.h | 9 +++++++++
2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index 9510b08..f47b614 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -31,6 +31,7 @@ static void nmi_stop(void);

/* 0 == registered but off, 1 == registered and on */
static int nmi_enabled = 0;
+int ibs_allowed; /* AMD Family 10h+ */

#ifdef CONFIG_PM

@@ -214,6 +215,11 @@ static int nmi_setup(void)
}

}
+
+ /*setup AMD Family10h IBS irq if needed */
+ if (ibs_allowed)
+ setup_ibs_nmi();
+
on_each_cpu(nmi_save_registers, NULL, 0, 1);
on_each_cpu(nmi_cpu_setup, NULL, 0, 1);
nmi_enabled = 1;
@@ -270,6 +276,10 @@ static void nmi_shutdown(void)
unregister_die_notifier(&profile_exceptions_nb);
model->shutdown(cpu_msrs);
free_msrs();
+
+ /*clear AMD Family 10h IBS irq if needed */
+ if (ibs_allowed)
+ clear_ibs_nmi();
}

static void nmi_cpu_start(void *dummy)
@@ -296,13 +306,15 @@ static void nmi_stop(void)
}

struct op_counter_config counter_config[OP_MAX_COUNTER];
+struct op_ibs_config ibs_config;

static int nmi_create_files(struct super_block *sb, struct dentry *root)
{
unsigned int i;
+ struct dentry *dir;

for (i = 0; i < model->num_counters; ++i) {
- struct dentry *dir;
+
char buf[4];

/* quick little hack to _not_ expose a counter if it is not
@@ -323,6 +335,31 @@ static int nmi_create_files(struct super_block *sb, struct dentry *root)
oprofilefs_create_ulong(sb, dir, "user", &counter_config[i].user);
}

+ /* Setup AMD Family 10h IBS control if needed */
+ if (ibs_allowed) {
+ char buf[12];
+
+ /* setup some reasonable defaults */
+ ibs_config.max_cnt_fetch = 250000;
+ ibs_config.FETCH_enabled = 0;
+ ibs_config.max_cnt_op = 250000;
+ ibs_config.OP_enabled = 0;
+ snprintf(buf, sizeof(buf), "ibs_fetch");
+ dir = oprofilefs_mkdir(sb, root, buf);
+ oprofilefs_create_ulong(sb, dir, "ran_enable",
+ &ibs_config.rand_en);
+ oprofilefs_create_ulong(sb, dir, "enable",
+ &ibs_config.FETCH_enabled);
+ oprofilefs_create_ulong(sb, dir, "max_count",
+ &ibs_config.max_cnt_fetch);
+ snprintf(buf, sizeof(buf), "ibs_uops");
+ dir = oprofilefs_mkdir(sb, root, buf);
+ oprofilefs_create_ulong(sb, dir, "enable",
+ &ibs_config.OP_enabled);
+ oprofilefs_create_ulong(sb, dir, "max_count",
+ &ibs_config.max_cnt_op);
+ }
+
return 0;
}

@@ -391,6 +428,7 @@ int __init op_nmi_init(struct oprofile_operations *ops)
__u8 vendor = boot_cpu_data.x86_vendor;
__u8 family = boot_cpu_data.x86;
char *cpu_type;
+ uint32_t eax, ebx, ecx, edx;

if (!cpu_has_apic)
return -ENODEV;
@@ -414,9 +452,17 @@ int __init op_nmi_init(struct oprofile_operations *ops)
break;
case 0x10:
model = &op_athlon_spec;
- cpu_type = "x86-64/family10";
+ cpu_type = "x86-64/family10h";
+ /* This CPU has IBS capability */
+ ibs_allowed = 1;
break;
}
+ /* see if IBS is available */
+ if (family >= 0x10) {
+ cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
+ if (ecx & 0x40)
+ ibs_allowed = 1;
+ }
break;

case X86_VENDOR_INTEL:
diff --git a/arch/x86/oprofile/op_counter.h b/arch/x86/oprofile/op_counter.h
index 2880b15..5681445 100644
--- a/arch/x86/oprofile/op_counter.h
+++ b/arch/x86/oprofile/op_counter.h
@@ -26,4 +26,13 @@ struct op_counter_config {

extern struct op_counter_config counter_config[];

+struct op_ibs_config {
+ unsigned long OP_enabled;
+ unsigned long FETCH_enabled;
+ unsigned long max_cnt_fetch;
+ unsigned long max_cnt_op;
+ unsigned long rand_en;
+};
+
+extern struct op_ibs_config ibs_config;
#endif /* OP_COUNTER_H */


2008-02-08 20:47:00

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 1/3] AMD Family10h IBS support for oProfile driver

Nitpicks only - and as I am not familiar
with this codebase I could not provide
proper code review.

Sam

> +
> + /*setup AMD Family10h IBS irq if needed */

Please add a space after '/*'
Several places.

> static int nmi_create_files(struct super_block *sb, struct dentry *root)
> {
> unsigned int i;
> + struct dentry *dir;
>
> for (i = 0; i < model->num_counters; ++i) {
> - struct dentry *dir;
> +
> char buf[4];
Please drop this empty line

> @@ -391,6 +428,7 @@ int __init op_nmi_init(struct oprofile_operations *ops)
> __u8 vendor = boot_cpu_data.x86_vendor;
> __u8 family = boot_cpu_data.x86;
> char *cpu_type;
> + uint32_t eax, ebx, ecx, edx;

We do not recommned use of uint32_t in the kernel. Use plain u32.
uint32_t belongs to a namespace outside the kernel.
(googling will find lots of discussion on the topic but I see code
just above using u8 so at least be consistent and use u32)

> + /* see if IBS is available */
> + if (family >= 0x10) {
> + cpuid(0x80000001, &eax, &ebx, &ecx, &edx);

Two hardcoded numbers on two lines??

> + if (ecx & 0x40)
And one more..