Change in coreboot[master]: cpu/x86: Move some PARALLEL_MP prototypes

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: cpu/x86: Move some PARALLEL_MP prototypes
......................................................................

cpu/x86: Move some PARALLEL_MP prototypes

The implementations live inside platform directories,
but the function signatures must match those defined
by PARALLEL_MP implementation.

Change-Id: If05ee2e44504e5511f3a7a2c5dc6e48fc16a07b2
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/haswell.h
M src/cpu/intel/smm/gen1/smi.h
M src/include/cpu/x86/mp.h
M src/soc/intel/broadwell/include/soc/smm.h
M src/soc/intel/cannonlake/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/icelake/include/soc/smm.h
M src/soc/intel/skylake/include/soc/smm.h
8 files changed, 7 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34152/1

diff --git a/src/cpu/intel/haswell/haswell.h b/src/cpu/intel/haswell/haswell.h
index cd8d5cb..2c52caf 100644
--- a/src/cpu/intel/haswell/haswell.h
+++ b/src/cpu/intel/haswell/haswell.h
@@ -160,13 +160,6 @@
 /* Configure power limits for turbo mode */
 void set_power_limits(u8 power_limit_1_time);
 int cpu_config_tdp_levels(void);
-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);
 struct bus;
 void bsp_init_and_start_aps(struct bus *cpu_bus);
 /* Determine if HyperThreading is disabled. The variable is not valid until
diff --git a/src/cpu/intel/smm/gen1/smi.h b/src/cpu/intel/smm/gen1/smi.h
index 3d5149a..4c80230 100644
--- a/src/cpu/intel/smm/gen1/smi.h
+++ b/src/cpu/intel/smm/gen1/smi.h
@@ -24,11 +24,4 @@
 bool cpu_has_alternative_smrr(void);

/* parallel MP helper functions */
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
 void southbridge_smm_clear_state(void);
-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-			uintptr_t staggered_smbase);
-void smm_relocate(void);
-void smm_lock(void);
diff --git a/src/include/cpu/x86/mp.h b/src/include/cpu/x86/mp.h
index c04252e..05f2471 100644
--- a/src/include/cpu/x86/mp.h
+++ b/src/include/cpu/x86/mp.h
@@ -154,4 +154,11 @@
 /* Send SMI to self with single execution. */
 void smm_initiate_relocation(void);

+/* Parallel MP helper functions. */
+void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, size_t *smm_save_state_size);
+void smm_initialize(void);
+void smm_relocation_handler(int cpu, uintptr_t curr_smbase, uintptr_t staggered_smbase);
+void smm_relocate(void);
+void smm_lock(void);
+
 #endif /* _X86_MP_H_ */
diff --git a/src/soc/intel/broadwell/include/soc/smm.h b/src/soc/intel/broadwell/include/soc/smm.h
index d3e1cdd..fe38dc9 100644
--- a/src/soc/intel/broadwell/include/soc/smm.h
+++ b/src/soc/intel/broadwell/include/soc/smm.h
@@ -53,13 +53,6 @@
    return CONFIG_SMM_TSEG_SIZE;
 }

-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);

/* These helpers are for performing SMM relocation. */
 void southbridge_trigger_smi(void);
diff --git a/src/soc/intel/cannonlake/include/soc/smm.h b/src/soc/intel/cannonlake/include/soc/smm.h
index c0ab82f..5cf7407 100644
--- a/src/soc/intel/cannonlake/include/soc/smm.h
+++ b/src/soc/intel/cannonlake/include/soc/smm.h
@@ -50,12 +50,5 @@
 /* Mainboard handler for eSPI SMIs */
 void mainboard_smi_espi_handler(void);

-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);

#endif
diff --git a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
index 72aa7fa..a3400ff 100644
--- a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
+++ b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
@@ -54,13 +54,6 @@
    return CONFIG_SMM_TSEG_SIZE;
 }

-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);

/* These helpers are for performing SMM relocation. */
 void southbridge_trigger_smi(void);
diff --git a/src/soc/intel/icelake/include/soc/smm.h b/src/soc/intel/icelake/include/soc/smm.h
index 991c593..9777599 100644
--- a/src/soc/intel/icelake/include/soc/smm.h
+++ b/src/soc/intel/icelake/include/soc/smm.h
@@ -49,12 +49,5 @@
 /* Mainboard handler for eSPI SMIs */
 void mainboard_smi_espi_handler(void);

-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);

#endif
diff --git a/src/soc/intel/skylake/include/soc/smm.h b/src/soc/intel/skylake/include/soc/smm.h
index 0c5e976..4dfa627 100644
--- a/src/soc/intel/skylake/include/soc/smm.h
+++ b/src/soc/intel/skylake/include/soc/smm.h
@@ -48,12 +48,5 @@
    int smm_save_state_in_msrs;
 };

-void smm_relocation_handler(int cpu, uintptr_t curr_smbase,
-				uintptr_t staggered_smbase);
-void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
-		size_t *smm_save_state_size);
-void smm_initialize(void);
-void smm_relocate(void);
-void smm_lock(void);

#endif

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34152

to look at the new patch set (#3).

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

soc/intel: Move some PARALLEL_MP prototypes

The implementations live inside platform directories,
but the function signatures must match those defined
by PARALLEL_MP implementation.

Change-Id: If05ee2e44504e5511f3a7a2c5dc6e48fc16a07b2
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/haswell.h
M src/cpu/intel/smm/gen1/smi.h
M src/include/cpu/x86/mp.h
M src/soc/intel/broadwell/include/soc/smm.h
M src/soc/intel/cannonlake/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/icelake/include/soc/smm.h
M src/soc/intel/skylake/include/soc/smm.h
8 files changed, 7 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34152/3

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:

When functions are called in local (platform scope) only, is it ok to provide global declarations?

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:

That these share the same declaration raises a question:
Why have 6 copies of the (nearly) same code in the first place?

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:

How about moving gen1/smi.h one level up?

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:
> Patch Set 4:
>
> That these share the same declaration raises a question:
> Why have 6 copies of the (nearly) same code in the first place?

The same thing happens for pretty much every cpu/soc, not just the 7 here. Initialisers of struct mp_ops are split across two or more files, otherwise we would just declare the functions with static together in the same file with const initialiser of mp_ops, right?

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:
> > That these share the same declaration raises a question:
> > Why have 6 copies of the (nearly) same code in the first place?
>
> The same thing happens for pretty much every cpu/soc, not just the 7 here. Initialisers of struct mp_ops are split across two or more files, otherwise we would just declare the functions with static together in the same file with const initialiser of mp_ops, right?

Yeah, I see the pattern. I just want to make sure that this
doesn't become a copy-pasta supporting change. There is some
more weirdness, e.g. smm_lock() seems to be declared somewhere
in arch/x86/ already. I'm not sure if it's the same function.

It's just hard to draw a line, imho. But I would like to keep
it to the functions that are directly pointed to by `mp_ops`.
smm_lock() and smm_relocate() seem more implementation specific.

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 4:
> > > That these share the same declaration raises a question:
> > > Why have 6 copies of the (nearly) same code in the first place?
> >
> > The same thing happens for pretty much every cpu/soc, not just the 7 here. Initialisers of struct mp_ops are split across two or more files, otherwise we would just declare the functions with static together in the same file with const initialiser of mp_ops, right?
>
> Yeah, I see the pattern. I just want to make sure that this
> doesn't become a copy-pasta supporting change. There is some
> more weirdness, e.g. smm_lock() seems to be declared somewhere
> in arch/x86/ already. I'm not sure if it's the same function.

Nvm, found the related patch.

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34152/5/src/include/cpu/x86/mp.h 
File src/include/cpu/x86/mp.h:

https://review.coreboot.org/c/coreboot/+/34152/5/src/include/cpu/x86/mp.h@15... 
PS5, Line 158: size_t
not defined.

Change in coreboot[master]: soc/intel: Move some PARALLEL_MP prototypes

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34152 )

Change subject: soc/intel: Move some PARALLEL_MP prototypes
......................................................................

Abandoned