[RFC][PATCH 0/2] x86, CPU: Run stuff on the BSP

[RFC][PATCH 0/2] x86, CPU: Run stuff on the BSP

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Ok,

this is just an RFC and it builds but no further tests have been done on
it. The first patch is a trivial cleanup and can go in.

For the second one I opted to look at c->x86_vendor in cpu/common.c
which is not very clean but still simpler than adding a miscellaneous
struct to contain the ->run_on_bsp() pointer and be statically allocated
as __initdata, in the cpu_dev fashion. We can always do that later if
there's more ofthe similar functionality needed.

Hmm, so please take a look and let me know if this is going in the right
direction.

Thanks.

[RFC][PATCH 1/2] x86, CPU: Drop second get_cpu_cap prototype

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

get_cpu_cap() external function prototype was declared twice so loose
one of them.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/cpu.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 1b22dcc..9d388bf 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -35,6 +35,5 @@ extern const struct cpu_dev *const __x86_cpu_dev_start[],
 
 extern void get_cpu_cap(struct cpuinfo_x86 *c);
 extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
-extern void get_cpu_cap(struct cpuinfo_x86 *c);
 
 #endif
-- 
1.7.4.rc2

[RFC][PATCH 2/2] x86, cpu, amd: Add a per-vendor BSP function

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Add a per-vendor function which runs everything that needs to run once
on the BSP during boot. Concentrate AMD-specific functionality there.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/amd.c    |    5 +++--
 arch/x86/kernel/cpu/common.c |   21 ++++++++++++++++-----
 arch/x86/kernel/cpu/cpu.h    |    2 +-
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index b6e3e87..45db331 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -410,7 +410,7 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 #endif
 }
 
-static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+void __init amd_run_on_bsp(struct cpuinfo_x86 *c)
 {
 	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
 
@@ -436,6 +436,8 @@ static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
 		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
 		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
 	}
+
+	init_amd_e400_c1e_mask();
 }
 
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
@@ -690,7 +692,6 @@ static const struct cpu_dev __cpuinitconst amd_cpu_dev = {
 	.c_size_cache	= amd_size_cache,
 #endif
 	.c_early_init   = early_init_amd,
-	.c_bsp_init	= bsp_init_amd,
 	.c_init		= init_amd,
 	.c_x86_vendor	= X86_VENDOR_AMD,
 };
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8ed394a..67f1d48 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -681,9 +681,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	filter_cpuid_features(c, false);
 
 	setup_smep(c);
-
-	if (this_cpu->c_bsp_init)
-		this_cpu->c_bsp_init(c);
 }
 
 void __init early_cpu_init(void)
@@ -902,16 +899,30 @@ static void vgetcpu_set_mode(void)
 }
 #endif
 
+static void __init vendor_run_on_bsp(struct cpuinfo_x86 *c)
+{
+	switch (c->x86_vendor) {
+	case X86_VENDOR_AMD:
+		amd_run_on_bsp(c);
+		break;
+
+	default:
+		break;
+	}
+}
+
 void __init identify_boot_cpu(void)
 {
-	identify_cpu(&boot_cpu_data);
-	init_amd_e400_c1e_mask();
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	identify_cpu(c);
 #ifdef CONFIG_X86_32
 	sysenter_setup();
 	enable_sep_cpu();
 #else
 	vgetcpu_set_mode();
 #endif
+	vendor_run_on_bsp(c);
 }
 
 void __cpuinit identify_secondary_cpu(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 9d388bf..a41d94e 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -18,7 +18,6 @@ struct cpu_dev {
 	struct		cpu_model_info c_models[4];
 
 	void            (*c_early_init)(struct cpuinfo_x86 *);
-	void		(*c_bsp_init)(struct cpuinfo_x86 *);
 	void		(*c_init)(struct cpuinfo_x86 *);
 	void		(*c_identify)(struct cpuinfo_x86 *);
 	unsigned int	(*c_size_cache)(struct cpuinfo_x86 *, unsigned int);
@@ -35,5 +34,6 @@ extern const struct cpu_dev *const __x86_cpu_dev_start[],
 
 extern void get_cpu_cap(struct cpuinfo_x86 *c);
 extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
+extern void amd_run_on_bsp(struct cpuinfo_x86 *c);
 
 #endif
-- 
1.7.4.rc2

Re: [RFC][PATCH 2/2] x86, cpu, amd: Add a per-vendor BSP function

[H. Peter Anvin]

On 08/08/2011 01:57 PM, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Add a per-vendor function which runs everything that needs to run once
> on the BSP during boot. Concentrate AMD-specific functionality there.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
>  
> +static void __init vendor_run_on_bsp(struct cpuinfo_x86 *c)
> +{
> +	switch (c->x86_vendor) {
> +	case X86_VENDOR_AMD:
> +		amd_run_on_bsp(c);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +}
> +

This is totally going backwards.  We *should* be using struct cpu_dev
rather than switch statements for this.

	-hpa

Re: [RFC][PATCH 2/2] x86, cpu, amd: Add a per-vendor BSP function

[Borislav Petkov]

On Mon, Aug 08, 2011 at 03:56:08PM -0500, H. Peter Anvin wrote:
> On 08/08/2011 01:57 PM, Borislav Petkov wrote:
> > From: Borislav Petkov <borislav.petkov@amd.com>
> > 
> > Add a per-vendor function which runs everything that needs to run once
> > on the BSP during boot. Concentrate AMD-specific functionality there.
> > 
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> >  
> > +static void __init vendor_run_on_bsp(struct cpuinfo_x86 *c)
> > +{
> > +	switch (c->x86_vendor) {
> > +	case X86_VENDOR_AMD:
> > +		amd_run_on_bsp(c);
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> 
> This is totally going backwards.  We *should* be using struct cpu_dev
> rather than switch statements for this.

Right, but all the cpu_dev things are annotated with __cpuinitconst
because they're used in CONFIG_HOTPLUG_CPU. __init, OTOH, will be
discarded once we're done booting. So, we can't convert cpu_dev
to __initdata because we need it for cpu hotplug and we want the
run_on_bsp() functionality to be __init since it runs once on boot.

Maybe leave cpu_dev in __cpuinit let it have an __init member which is
the ->run_on_bsp()? Does that even work?

Hmm..

-- 
Regards/Gruss,
    Boris.

Re: [RFC][PATCH 2/2] x86, cpu, amd: Add a per-vendor BSP function

[H. Peter Anvin]

On 08/08/2011 04:57 PM, Borislav Petkov wrote:
>>
>> This is totally going backwards.  We *should* be using struct cpu_dev
>> rather than switch statements for this.
> 
> Right, but all the cpu_dev things are annotated with __cpuinitconst
> because they're used in CONFIG_HOTPLUG_CPU. __init, OTOH, will be
> discarded once we're done booting. So, we can't convert cpu_dev
> to __initdata because we need it for cpu hotplug and we want the
> run_on_bsp() functionality to be __init since it runs once on boot.
> 
> Maybe leave cpu_dev in __cpuinit let it have an __init member which is
> the ->run_on_bsp()? Does that even work?
> 

I don't think so, which is a fundamental shortcoming of our way of
handling these kinds of pointers.  One way to deal with it would be to
make struct cpu_dev __initconst and copy it into a __cpuinit variable at
init time.

Either way, I'd rather leave the routines in cpuinit memory than adding
another multiplex.

	-hpa

Re: [RFC][PATCH 2/2] x86, cpu, amd: Add a per-vendor BSP function

[Borislav Petkov]

On Mon, Aug 08, 2011 at 06:13:17PM -0400, H. Peter Anvin wrote:
> On 08/08/2011 04:57 PM, Borislav Petkov wrote:
> >>
> >> This is totally going backwards.  We *should* be using struct cpu_dev
> >> rather than switch statements for this.
> > 
> > Right, but all the cpu_dev things are annotated with __cpuinitconst
> > because they're used in CONFIG_HOTPLUG_CPU. __init, OTOH, will be
> > discarded once we're done booting. So, we can't convert cpu_dev
> > to __initdata because we need it for cpu hotplug and we want the
> > run_on_bsp() functionality to be __init since it runs once on boot.
> > 
> > Maybe leave cpu_dev in __cpuinit let it have an __init member which is
> > the ->run_on_bsp()? Does that even work?
> > 
> 
> I don't think so, which is a fundamental shortcoming of our way of
> handling these kinds of pointers.  One way to deal with it would be to
> make struct cpu_dev __initconst and copy it into a __cpuinit variable at
> init time.

How about we shut up modpost by allowing __cpuinitconst to reference
__init functions - I mean, __cpuinitconst stays while __init gets
discarded and if we take a special care to not call the ->bsp_on_init()
pointer after boot, I don't see why not. I.e., mimick something like the
__initdata_refok semantics but for __cpuinitconst sections referencing
__init functions.

I.e., something like the clumsy proof-of-concept below:


diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index b6e3e87..dc2a411 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -410,7 +410,7 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 #endif
 }
 
-static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+static void __init bsp_init_amd(struct cpuinfo_x86 *c)
 {
 	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 413c536..f288756 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -865,6 +865,7 @@ static const char *init_exit_sections[] =
 
 /* data section */
 static const char *data_sections[] = { DATA_SECTIONS, NULL };
+static const char *cpuinit_sections[] = { CPU_INIT_SECTIONS, NULL };
 
 
 /* symbols in .data that may refer to init/exit sections */
@@ -943,7 +944,7 @@ const struct sectioncheck sectioncheck[] = {
 	.fromsec = { ALL_XXXINIT_SECTIONS, NULL },
 	.tosec   = { INIT_SECTIONS, NULL },
 	.mismatch = XXXINIT_TO_SOME_INIT,
-	.symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
+	.symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, "*_cpu_dev", NULL },
 },
 /* Do not reference cpuinit code/data from meminit code/data */
 {
@@ -1081,9 +1082,11 @@ static int secref_whitelist(const struct sectioncheck *mismatch,
 
 	/* Check for pattern 2 */
 	if (match(tosec, init_exit_sections) &&
-	    match(fromsec, data_sections) &&
-	    match(fromsym, mismatch->symbol_white_list))
+	    (match(fromsec, data_sections) ||
+	     match(fromsec, cpuinit_sections)) &&
+	    match(fromsym, mismatch->symbol_white_list)) {
 		return 0;
+	}
 
 	/* Check for pattern 3 */
 	if (match(fromsec, head_sections) &&



Hmmm..?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551