Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram
INVD is called below so if postcar is running in a cached environment it needs to happen.
NOTE: postcar cannot execute in a cached environment if clflush is not supported!
Change-Id: I37681ee1f1d2ae5f9dd824b5baf7b23b2883b1dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/exit_car.S 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/37212/1
diff --git a/src/arch/x86/exit_car.S b/src/arch/x86/exit_car.S index 8c28784..ceb0321 100644 --- a/src/arch/x86/exit_car.S +++ b/src/arch/x86/exit_car.S @@ -38,7 +38,14 @@ movl 4(%esp), %eax movl %eax, _cbmem_top_ptr #endif + /* Make sure _cbmem_top_ptr hits dram before invd */ + movl $1, %eax + cpuid + btl $19, %edx + jz skip_clflush + clflush _cbmem_top_ptr
+skip_clflush: /* chipset_teardown_car() is expected to disable cache-as-ram. */ call chipset_teardown_car
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37212/4/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/37212/4/src/arch/x86/exit_car.S@44 PS4, Line 44: btl $19, %edx Can we add #defines for some of these bit fields somewhere?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37212/4/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/37212/4/src/arch/x86/exit_car.S@44 PS4, Line 44: btl $19, %edx
Can we add #defines for some of these bit fields somewhere?
Done
Hello build bot (Jenkins), Subrata Banik, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37212
to look at the new patch set (#7).
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram
INVD is called below so if postcar is running in a cached environment it needs to happen.
NOTE: postcar cannot execute in a cached environment if clflush is not supported!
Change-Id: I37681ee1f1d2ae5f9dd824b5baf7b23b2883b1dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/exit_car.S M src/include/cpu/x86/cache.h 2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/37212/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 7: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37212/7/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/37212/7/src/arch/x86/exit_car.S@35 PS7, Line 35: clflush Use wbinv if clflush isn't supported?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37212/7/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/37212/7/src/arch/x86/exit_car.S@35 PS7, Line 35: clflush
Use wbinv if clflush isn't supported?
I think the goal is to keep nothing left from the CAR environment. wbinv might in some cases write to RAM (corrupting RAM on S3 resume) or to 'nothing' which might have some undesirable behaviour.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37212 )
Change subject: arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram ......................................................................
arch/x86/exit_car.S: Make sure _cbmem_top_ptr hits dram
INVD is called below so if postcar is running in a cached environment it needs to happen.
NOTE: postcar cannot execute in a cached environment if clflush is not supported!
Change-Id: I37681ee1f1d2ae5f9dd824b5baf7b23b2883b1dc Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/37212 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/exit_car.S M src/include/cpu/x86/cache.h 2 files changed, 10 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/arch/x86/exit_car.S b/src/arch/x86/exit_car.S index 806dc9c..dc356b2 100644 --- a/src/arch/x86/exit_car.S +++ b/src/arch/x86/exit_car.S @@ -2,6 +2,7 @@
#include <cpu/x86/mtrr.h> #include <cpu/x86/cr.h> +#include <cpu/x86/cache.h>
.section ".module_parameters", "aw", @progbits /* stack_top indicates the stack to pull MTRR information from. */ @@ -54,7 +55,14 @@ movl 4(%esp), %eax movl %eax, _cbmem_top_ptr #endif + /* Make sure _cbmem_top_ptr hits dram before invd */ + movl $1, %eax + cpuid + btl $CPUID_FEATURE_CLFLUSH_BIT, %edx + jz skip_clflush + clflush _cbmem_top_ptr
+skip_clflush: /* chipset_teardown_car() is expected to disable cache-as-ram. */ call chipset_teardown_car
diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h index 01b202e..6234110 100644 --- a/src/include/cpu/x86/cache.h +++ b/src/include/cpu/x86/cache.h @@ -8,6 +8,8 @@ #define CR0_CacheDisable (CR0_CD) #define CR0_NoWriteThrough (CR0_NW)
+#define CPUID_FEATURE_CLFLUSH_BIT 19 + #if !defined(__ASSEMBLER__)
static inline void wbinvd(void)
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)