Change in coreboot[master]: intel/microcode: Remove uses of __PRE_RAM__

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

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

Change subject: intel/microcode: Remove uses of __PRE_RAM__
......................................................................

intel/microcode: Remove uses of __PRE_RAM__

Include file <smp/spinlock.h> has the required logic
to define empty stubs for spinlocks.

Change-Id: I00da5c2b0570c26f2e3bb464274485cc2c08c8f0
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/microcode/microcode.c
1 file changed, 2 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/34929/1

diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c
index 44f7f1f..7bd072f 100644
--- a/src/cpu/intel/microcode/microcode.c
+++ b/src/cpu/intel/microcode/microcode.c
@@ -27,11 +27,9 @@
 #include <arch/cpu.h>
 #include <cpu/x86/msr.h>
 #include <cpu/intel/microcode.h>
-
-#if !defined(__PRE_RAM__)
 #include <smp/spinlock.h>
+
 DECLARE_SPIN_LOCK(microcode_lock)
-#endif

struct microcode {
    u32 hdrver;	/* Header Version */
@@ -152,7 +150,7 @@
    unsigned int x86_model, x86_family;
    msr_t msr;

-#ifdef __ROMCC__
+#if defined(__ROMCC__)
    struct cbfs_file *microcode_file;

microcode_file = walkcbfs_head((char *) MICROCODE_CBFS_FILE);
@@ -228,15 +226,11 @@
 {
    const void *patch = intel_microcode_find();

-#if !defined(__ROMCC__) && !defined(__PRE_RAM__)
    spin_lock(&microcode_lock);
-#endif

intel_microcode_load_unlocked(patch);

-#if !defined(__ROMCC__) && !defined(__PRE_RAM__)
    spin_unlock(&microcode_lock);
-#endif
 }

#if ENV_RAMSTAGE

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

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

Change subject: intel/microcode: Remove uses of __PRE_RAM__
......................................................................

Patch Set 1: Code-Review-2

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

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

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

arch/x86: Fix spinlocks for cases of __PRE_RAM__

Include file <smp/spinlock.h> has the required logic
to define empty stubs for spinlocks.

Change-Id: I00da5c2b0570c26f2e3bb464274485cc2c08c8f0
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/include/arch/smp/spinlock.h
M src/console/printk.c
M src/cpu/amd/microcode/microcode.c
M src/cpu/amd/pi/00730F01/microcode_fam16h.c
M src/cpu/intel/microcode/microcode.c
M src/drivers/pc80/rtc/mc146818rtc.c
6 files changed, 42 insertions(+), 80 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/34929/2

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
File src/arch/x86/include/arch/smp/spinlock.h:

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
PS3, Line 34: #define HAS_ROMSTAGE_SPINLOCKS \
Macros with complex values should be enclosed in parentheses

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
File src/arch/x86/include/arch/smp/spinlock.h:

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
PS3, Line 39: ENV_STAGE_HAS_DATA_SECTION
Even if a stage has a data section (say we add that ability to CAR stages: bootblock, verstage, romstage), it's not enough to allow locking bus transactions. I suspect we'll need to try things on a case by case basis because performing atomic instructions on CAR-backed variables might not work on every platform. Something to keep in mind. The other thing to think about here is if we would want spinlocks enabled for every stage just because it has a data section. I'm not sure that's necessarily the case. We probably should break out those things.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

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

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

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
File src/arch/x86/include/arch/smp/spinlock.h:

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
PS3, Line 39: ENV_STAGE_HAS_DATA_SECTION
> Even if a stage has a data section (say we add that ability to CAR stages: bootblock, verstage, roms […]

I agree. I know AGESA fam14 has cache coherency issues. Based on some of the feedback, fam10-15 might as well, even though these extra romstage spinlocks were infact implemented for those platforms.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/4/src/arch/x86/include/arch/s... 
File src/arch/x86/include/arch/smp/spinlock.h:

https://review.coreboot.org/c/coreboot/+/34929/4/src/arch/x86/include/arch/s... 
PS4, Line 34: #define HAS_ROMSTAGE_SPINLOCKS \
Macros with complex values should be enclosed in parentheses

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

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

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

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:

This commit may be the last place evaluating defined(__PRE_RAM__) and I don't have too many ideas of howto proceed with this to make it clean.

I am not sure if we need/want -D__PRE_RAM__ anymore for CPPFLAGS.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:
> Patch Set 4:
>
> This commit may be the last place evaluating defined(__PRE_RAM__) and I don't have too many ideas of howto proceed with this to make it clean.
>
> I am not sure if we need/want -D__PRE_RAM__ anymore for CPPFLAGS.

I skimmed the patch and didn't see a __PRE_RAM__ reference. If we waren't using it any longer I think it's fine to remove from CPPFLAGS.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c@29 
PS4, Line 29: if (ENV_STAGE_HAS_DATA_SECTION)
            : 		spin_lock(&console_lock);
            : 	else if (ENV_ROMSTAGE && CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK))
            : 		spin_lock(romstage_console_lock());
Just curious: You are not using a helper function here to get a pointer to the spinlock because the helper function could return NULL and spin_lock() might not necessarily be NOP depending upon which romstage spinlock configs have been selected.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

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

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

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c@29 
PS4, Line 29: if (ENV_STAGE_HAS_DATA_SECTION)
            : 		spin_lock(&console_lock);
            : 	else if (ENV_ROMSTAGE && CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK))
            : 		spin_lock(romstage_console_lock());
> Just curious: You are not using a helper function here to get a pointer to the spinlock because the  […]

Would you rephrase the comment (or was there a question)?

Use of the helper function (present in original work already) was forced by:

1. CAR_GLOBAL_MIGRATION=y, romstage needs to take the form spin_lock(car_get_var_ptr(&console_lock)) instead.
2. Lack of .data means DECLARE_SPIN_LOCK() does not work for stages in CAR. Those spinlock variables are explicitly defined as CAR_GLOBALs and unlocked in mainboard/ romstage.c files (yuck).

The two boards (asus/kcma-d8,kgpe-d16) with any 'select HAVE_ROMSTAGE_xxx_SPINLOCK' will hit 3 of 3 deprecation criteria on next release. Can we just drop current implementation in all its uglyness, together with these Kconfigs, and have a more generic solution?

https://review.coreboot.org/c/coreboot/+/34929/4/src/cpu/amd/pi/00730F01/mic... 
File src/cpu/amd/pi/00730F01/microcode_fam16h.c:

PS4: 
Added in CB:29792 but since CB:29793 was abandoned we only need this file built for ramstage. And the platform in question does not declare romstage spinlocks.

Change in coreboot[master]: arch/x86: Fix spinlocks for cases of __PRE_RAM__

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Fix spinlocks for cases of __PRE_RAM__
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c@29 
PS4, Line 29: if (ENV_STAGE_HAS_DATA_SECTION)
            : 		spin_lock(&console_lock);
            : 	else if (ENV_ROMSTAGE && CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK))
            : 		spin_lock(romstage_console_lock());
> Would you rephrase the comment (or was there a question)? […]

I meant you can have a helper function here as well:
static spinlock_t *get_console_spinlock(void)
{

}

which returns the right spinlock that can be used for calling spin_lock()/spin_unlock() so that you don't have to repeat the same logic twice.

As to whether this should really be supported, if the boards really needing this special logic will be deprecated in the next release, it might make sense to completely drop the ugliness.

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

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

Hello Patrick Rudolph, Julius Werner, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

arch/x86: Remove spinlocks inside CAR

This was only used with amdfam10h-15h, where cache
coherency between nodes was supposed to be guaranteed
with this code. We could want a cleaner and more generic
approach for this, possibly utilising .data sections.

Change-Id: I00da5c2b0570c26f2e3bb464274485cc2c08c8f0
Signed-off-by: Kyösti Mälkki 
---
M src/Kconfig
M src/arch/x86/include/arch/smp/spinlock.h
M src/console/printk.c
3 files changed, 6 insertions(+), 50 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/34929/5

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

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

Kyösti Mälkki has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

Removed Code-Review-2 by Kyösti Mälkki

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

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

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

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
File src/arch/x86/include/arch/smp/spinlock.h:

https://review.coreboot.org/c/coreboot/+/34929/3/src/arch/x86/include/arch/s... 
PS3, Line 39: ENV_STAGE_HAS_DATA_SECTION
> I agree. I know AGESA fam14 has cache coherency issues. […]

I essentially reverted spinlocks in CAR now.

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/c/coreboot/+/34929/4/src/console/printk.c@29 
PS4, Line 29: if (ENV_STAGE_HAS_DATA_SECTION)
            : 		spin_lock(&console_lock);
            : 	else if (ENV_ROMSTAGE && CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK))
            : 		spin_lock(romstage_console_lock());
> I meant you can have a helper function here as well: […]

Ack

https://review.coreboot.org/c/coreboot/+/34929/4/src/cpu/amd/pi/00730F01/mic... 
File src/cpu/amd/pi/00730F01/microcode_fam16h.c:

PS4:
> Added in CB:29792 but since CB:29793 was abandoned we only need this file built for ramstage. […]

Ack

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34929 )

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: arch/x86: Remove spinlocks inside CAR

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

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

Change subject: arch/x86: Remove spinlocks inside CAR
......................................................................

arch/x86: Remove spinlocks inside CAR

This was only used with amdfam10h-15h, where cache
coherency between nodes was supposed to be guaranteed
with this code. We could want a cleaner and more generic
approach for this, possibly utilising .data sections.

Change-Id: I00da5c2b0570c26f2e3bb464274485cc2c08c8f0
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34929
Tested-by: build bot (Jenkins) 
Reviewed-by: Arthur Heymans 
Reviewed-by: Aaron Durbin 
---
M src/Kconfig
M src/arch/x86/include/arch/smp/spinlock.h
M src/console/printk.c
3 files changed, 6 insertions(+), 50 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Arthur Heymans: Looks good to me, approved

diff --git a/src/Kconfig b/src/Kconfig
index ba9ae86..8df5323 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -527,22 +527,6 @@
      same path as a regular boot. e.g. an x86 system runs from the
      reset vector at 0xfffffff0 on both resume and warm/cold boot.

-config HAVE_ROMSTAGE_CONSOLE_SPINLOCK
-	bool
-	default n
-
-config HAVE_ROMSTAGE_NVRAM_CBFS_SPINLOCK
-	bool
-	default n
-	help
-	  This should be enabled on certain plaforms, such as the AMD
-	  SR565x, that cannot handle concurrent CBFS accesses from
-	  multiple APs during early startup.
-
-config HAVE_ROMSTAGE_MICROCODE_CBFS_SPINLOCK
-	bool
-	default n
-
 config NO_MONOTONIC_TIMER
    def_bool n

diff --git a/src/arch/x86/include/arch/smp/spinlock.h b/src/arch/x86/include/arch/smp/spinlock.h
index f918678..8bdb125 100644
--- a/src/arch/x86/include/arch/smp/spinlock.h
+++ b/src/arch/x86/include/arch/smp/spinlock.h
@@ -14,11 +14,6 @@
 #ifndef ARCH_SMP_SPINLOCK_H
 #define ARCH_SMP_SPINLOCK_H

-#if !defined(__PRE_RAM__) \
-	|| CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK)	\
-	|| CONFIG(HAVE_ROMSTAGE_NVRAM_CBFS_SPINLOCK)	\
-	|| CONFIG(HAVE_ROMSTAGE_MICROCODE_CBFS_SPINLOCK)
-
 /*
  * Your basic SMP spinlocks, allowing only a single CPU anywhere
  */
@@ -27,23 +22,14 @@
    volatile unsigned int lock;
 } spinlock_t;

-#ifdef __PRE_RAM__
-spinlock_t *romstage_console_lock(void);
-void initialize_romstage_console_lock(void);
-spinlock_t *romstage_nvram_cbfs_lock(void);
-void initialize_romstage_nvram_cbfs_lock(void);
-spinlock_t *romstage_microcode_cbfs_lock(void);
-void initialize_romstage_microcode_cbfs_lock(void);
-#endif
-
 #define SPIN_LOCK_UNLOCKED { 1 }

-#ifndef __PRE_RAM__
+#define STAGE_HAS_SPINLOCKS !ENV_ROMSTAGE_OR_BEFORE
+
+#if STAGE_HAS_SPINLOCKS
+
 #define DECLARE_SPIN_LOCK(x)	\
    static spinlock_t x = SPIN_LOCK_UNLOCKED;
-#else
-#define DECLARE_SPIN_LOCK(x)
-#endif

/*
  * Simple spin lock operations.  There are two variants, one clears IRQ's
@@ -93,7 +79,7 @@
    __asm__ __volatile__("rep;nop" : : : "memory");
 }

-#else /* !__PRE_RAM__ */
+#else

#define DECLARE_SPIN_LOCK(x)
 #define barrier()		do {} while (0)
@@ -103,6 +89,6 @@
 #define spin_unlock(lock)	do {} while (0)
 #define cpu_relax()		do {} while (0)

-#endif /* !__PRE_RAM__ */
+#endif

#endif /* ARCH_SMP_SPINLOCK_H */
diff --git a/src/console/printk.c b/src/console/printk.c
index 4f9f547..fd590fa 100644
--- a/src/console/printk.c
+++ b/src/console/printk.c
@@ -23,9 +23,7 @@
 #include <trace.h>
 #include <timer.h>

-#if (!defined(__PRE_RAM__) && CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK)) || !CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK)
 DECLARE_SPIN_LOCK(console_lock)
-#endif

#define TRACK_CONSOLE_TIME (CONFIG(HAVE_MONOTONIC_TIMER) && \
    (ENV_RAMSTAGE || !CONFIG(CAR_GLOBAL_MIGRATION)))
@@ -95,13 +93,7 @@
    	return 0;

DISABLE_TRACE;
-#ifdef __PRE_RAM__
-#if CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK)
-	spin_lock(romstage_console_lock());
-#endif
-#else
    spin_lock(&console_lock);
-#endif

console_time_run();

@@ -114,13 +106,7 @@

console_time_stop();

-#ifdef __PRE_RAM__
-#if CONFIG(HAVE_ROMSTAGE_CONSOLE_SPINLOCK)
-	spin_unlock(romstage_console_lock());
-#endif
-#else
    spin_unlock(&console_lock);
-#endif
    ENABLE_TRACE;

return i;