Change in coreboot[master]: cpu/intel/car: Correctly Cache the bootblock with C_ENVIRONMENT_BOOTB...
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly Cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly Cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 34 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/1
diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index f8fa806..c7034f8 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -18,6 +18,15 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG_C_ENVIRONMENT_BOOTBLOCK +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + .global bootblock_pre_c_entry
.code32 @@ -148,13 +157,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
post_code(0x28) diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 9a7dc5f..c025187 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -18,6 +18,15 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG_C_ENVIRONMENT_BOOTBLOCK +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + .global bootblock_pre_c_entry
.code32 @@ -136,13 +145,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
post_code(0x2e) diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index b7eb37b..4287583 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -26,6 +26,16 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG_C_ENVIRONMENT_BOOTBLOCK +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + + .global bootblock_pre_c_entry
.code32 @@ -354,13 +364,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
fill_cache:
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35990
to look at the new patch set (#2).
Change subject: cpu/intel/car: Correctly Cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly Cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 34 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/2
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35990
to look at the new patch set (#3).
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 34 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 3:
(1 comment)
What difference does it make? Do we expect trouble if we keep the old cache setup for C-env bootblock?
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... File src/cpu/intel/car/core2/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... PS3, Line 28: #endif Move to `mtrr.h`?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
What difference does it make? Do we expect trouble if we keep the old cache setup for C-env bootblock?
Yes, this way your bootblock runs cached and this results in a measurable difference iirc. Maybe the commit message should clarify that the symbols used are now those of the bootblock and that this now means that the bootblock is caching itself.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 3: Code-Review+1
Would really like to see the #cpp stuff in `mtrr.h` even if we can clean it up soon anyway.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 3:
I guess the old behavior was introduced for 1. FSP-T 2. Speed up stage/MRC cache loading Wouldn't that cause regressions on those platforms?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 3:
Nevermind, just noticed that it's only platforms without FSP-T support.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35990
to look at the new patch set (#4).
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 25 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... File src/cpu/intel/car/core2/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... PS3, Line 28: #endif
Move to `mtrr.h`?
I actually don't think it is such a good idea. This limitation does not hold on CPU's that have a non-eviction mode and there is no Kconfig symbol for that.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35990
to look at the new patch set (#5).
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 24 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... File src/cpu/intel/car/core2/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/35990/3/src/cpu/intel/car/core2/cac... PS3, Line 28: #endif
Move to `mtrr.h`? […]
Ah, I was more thinking about the calculation... but that depends on the check, which breaks other things... I see.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35990
to look at the new patch set (#6).
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 33 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35990/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35990 )
Change subject: cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK ......................................................................
cpu/intel/car: Correctly cache the bootblock with C_ENVIRONMENT_BOOTBLOCK
With CONFIG_C_ENVIRONMENT_BOOTBLOCK it makes more sense to rely on the size of the bootblock over CONFIG_XIP_ROM_SIZE. To make this work, only powers of 2 are allowed as bootblock size.
Change-Id: Ic8104ca9c51e4d2eccdb277e4c2111d2da662f3e Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/35990 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S 3 files changed, 33 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index f8fa806..a1bec12 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -18,6 +18,15 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG(C_ENVIRONMENT_BOOTBLOCK) +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + .global bootblock_pre_c_entry
.code32 @@ -148,13 +157,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
post_code(0x28) diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 9a7dc5f..33f0bfd 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -18,6 +18,15 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG(C_ENVIRONMENT_BOOTBLOCK) +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + .global bootblock_pre_c_entry
.code32 @@ -136,13 +145,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
post_code(0x2e) diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index b7eb37b..2cd0c5e 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -26,6 +26,15 @@ #define CACHE_AS_RAM_SIZE CONFIG_DCACHE_RAM_SIZE #define CACHE_AS_RAM_BASE CONFIG_DCACHE_RAM_BASE
+#if CONFIG(C_ENVIRONMENT_BOOTBLOCK) +#if ((CONFIG_C_ENV_BOOTBLOCK_SIZE & (CONFIG_C_ENV_BOOTBLOCK_SIZE - 1)) != 0) +#error "CONFIG_C_ENV_BOOTBLOCK_SIZE must be a power of 2!" +#endif +#define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE +#else +#define XIP_ROM_SIZE CONFIG_XIP_ROM_SIZE +#endif + .global bootblock_pre_c_entry
.code32 @@ -354,13 +363,13 @@ * https://mail.coreboot.org/pipermail/coreboot/2010-October/060922.html */ movl $_program, %eax - andl $(~(CONFIG_XIP_ROM_SIZE - 1)), %eax + andl $(~(XIP_ROM_SIZE - 1)), %eax orl $MTRR_TYPE_WRPROT, %eax wrmsr
movl $MTRR_PHYS_MASK(1), %ecx rdmsr - movl $(~(CONFIG_XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax + movl $(~(XIP_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax wrmsr
fill_cache:
-
Arthur Heymans (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)