Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld D src/arch/x86/prologue.inc M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 13 files changed, 68 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/1
diff --git a/src/arch/x86/bootblock_crt0.S b/src/arch/x86/bootblock_crt0.S index 3256731..d4dbf28 100644 --- a/src/arch/x86/bootblock_crt0.S +++ b/src/arch/x86/bootblock_crt0.S @@ -25,7 +25,9 @@ * Include the old code for reset vector and protected mode entry. That code has * withstood the test of time. */ -#include <arch/x86/prologue.inc> + +.section ".bootblock.top", "ax", @progbits + #include <cpu/x86/16bit/entry16.inc> #include <cpu/x86/16bit/reset16.inc> #include <cpu/x86/32bit/entry32.inc> @@ -69,3 +71,5 @@
/* We're done. Now it's up to platform-specific code */ jmp bootblock_pre_c_entry + +.previous diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index eff3738..38c4664 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -49,8 +49,6 @@
#include EARLY_MEMLAYOUT #elif ENV_BOOTBLOCK - BOOTBLOCK(CONFIG_X86_RESET_VECTOR - CONFIG_C_ENV_BOOTBLOCK_SIZE + 0x10, - CONFIG_C_ENV_BOOTBLOCK_SIZE)
#include EARLY_MEMLAYOUT
diff --git a/src/arch/x86/prologue.inc b/src/arch/x86/prologue.inc deleted file mode 100644 index 4036ff9..0000000 --- a/src/arch/x86/prologue.inc +++ /dev/null @@ -1,17 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <cpu/x86/post_code.h> - -.section ".rom.data", "a", @progbits -.section ".rom.text", "ax", @progbits diff --git a/src/arch/x86/walkcbfs.S b/src/arch/x86/walkcbfs.S index ded6558..82fd077 100644 --- a/src/arch/x86/walkcbfs.S +++ b/src/arch/x86/walkcbfs.S @@ -31,7 +31,7 @@
#define CBFS_FILE_STRUCTSIZE (CBFS_FILE_OFFSET + 4)
-.section .text +.section ".bootblock.top" .global walkcbfs_asm
/* @@ -131,3 +131,5 @@
filemagic: .ascii "LARCHIVE" + +.previous diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index 73618d9..65db322 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -23,6 +23,7 @@ #endif #define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE
+.section ".bootblock.top" .global bootblock_pre_c_entry
.code32 @@ -210,3 +211,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous diff --git a/src/cpu/intel/car/non-evict/cache_as_ram.S b/src/cpu/intel/car/non-evict/cache_as_ram.S index 4dee0a8..00e7b4f 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -22,6 +22,7 @@ #define NoEvictMod_MSR 0x2e0 #define BBL_CR_CTL3_MSR 0x11e
+.section ".bootblock.top" .global bootblock_pre_c_entry
.code32 @@ -254,3 +255,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 5262b18..94540ca 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -23,6 +23,7 @@ #endif #define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE
+.section ".bootblock.top" .global bootblock_pre_c_entry
.code32 @@ -198,3 +199,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous 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 fdeb0af..8a21b48 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -27,6 +27,7 @@ #endif #define XIP_ROM_SIZE CONFIG_C_ENV_BOOTBLOCK_SIZE
+.section ".bootblock.top" .global bootblock_pre_c_entry
.code32 @@ -409,3 +410,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous diff --git a/src/cpu/intel/microcode/microcode_asm.S b/src/cpu/intel/microcode/microcode_asm.S index 647f67c..2458185 100644 --- a/src/cpu/intel/microcode/microcode_asm.S +++ b/src/cpu/intel/microcode/microcode_asm.S @@ -54,7 +54,7 @@ * if the revision of the update is newer than what is installed */
-.section .text +.section ".bootblock.top" .global update_bsp_microcode
update_bsp_microcode: @@ -162,3 +162,5 @@ .string "cpu_microcode_blob.bin"
_update_bsp_microcode_end: + +.previous diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 40c0e99..b1aaa10 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -28,11 +28,11 @@ */
#include <arch/rom_segs.h> +#include <cpu/x86/post_code.h>
-/* Symbol _start16bit must be aligned to 4kB to start AP CPUs with - * Startup IPI message without RAM. +/* Symbol _start16bit must reachable from the reset vector, and be aligned to + * 4kB to start AP CPUs with Startup IPI message without RAM. */ -.align 4096 .code16 .globl _start16bit .type _start16bit, @function diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index ec01810..56672e8 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -14,9 +14,35 @@ /* _RESET_VECTOR: typically the top of the ROM */
SECTIONS { - /* Trigger an error if I have an unuseable start address */ - _TOO_LOW = CONFIG_X86_RESET_VECTOR - 0xfff0; - _bogus = ASSERT(_start16bit >= _TOO_LOW, "_start16bit too low. Please report."); + + /* + * Symbol ap_sipi_vector must be aligned to 4kB to start AP CPUs + * with Startup IPI message without RAM. Align .rom to next 4 byte + * boundary anyway. + */ + + .bogus ROMLOC_MIN : { + . = ALIGN(4); + } + + INCLUDE "bootblock/lib/program.ld" + + .bootblock_top . : { + . = CONFIG(SIPI_VECTOR_IN_ROM) ? ALIGN(4096) : ALIGN(4); + _realmode = .; + *(.bootblock.top); + _erealmode = .; + } + + PROGRAM_SZ = (_eprogram - _program + 16); + EARLYASM_SZ = (_erealmode - _realmode + 16) + (CONFIG(SIPI_VECTOR_IN_ROM) ? 4096 : 0); + + /* + * Allocation reserves extra 16 bytes here. Alignment requirements + * may cause the total size of a section to change when the start + * address gets applied. + */ + ROMLOC_MIN = CONFIG_X86_RESET_VECTOR - 0xf0 - (PROGRAM_SZ + EARLYASM_SZ);
. = CONFIG_X86_RESET_VECTOR; .reset . : { diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 1034992..d436805 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -21,16 +21,19 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/lapic_def.h> #include <cpu/x86/post_code.h>
+.section ".bootblock.top" + .code32 .globl _cache_as_ram_setup, _cache_as_ram_setup_end .global bootblock_pre_c_entry
_cache_as_ram_setup:
+#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) @@ -84,3 +87,4 @@ jmp stop
_cache_as_ram_setup_end: +.previous diff --git a/src/soc/amd/common/block/cpu/car/cache_as_ram.S b/src/soc/amd/common/block/cpu/car/cache_as_ram.S index 402da3a..e64b6d2 100644 --- a/src/soc/amd/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/amd/common/block/cpu/car/cache_as_ram.S @@ -21,10 +21,18 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
+.section ".bootblock.top" + +.code32 +.globl _cache_as_ram_setup, _cache_as_ram_setup_end + +_cache_as_ram_setup: + +#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) @@ -57,3 +65,6 @@ post_code(POST_DEAD_CODE) hlt jmp .halt_forever + +_cache_as_ram_setup_end: +.previous
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#2).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld D src/arch/x86/prologue.inc M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 13 files changed, 68 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 2: Code-Review+1
tested on p4 (cpuid 0xf65) http://dpaste.com/388PP50
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 33: *(.bootblock.top); The FIT table is also sitting within the last 4KiB. How are we ensuring we aren't packing things in that will collide?
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 37: 16 What are the 16 bytes for?
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 38: EARLYASM_SZ = (_erealmode - _realmode + 16) + (CONFIG(SIPI_VECTOR_IN_ROM) ? 4096 : 0); What are we trying to calculate? A comment would be helpful.
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 45: 0xf0 I'm not following this sequence either. I think there needs some more explanation.
Hello Patrick Rudolph, HAOUAS Elyes, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#3).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 67 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(3 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 33: *(.bootblock.top);
The FIT table is also sitting within the last 4KiB. […]
Only the FIT pointer is/was near the top, object section was in section .rom.data in CB:37955 for FIT.
Should we introduce .bootblock.id and .bootblock.fit ?
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 37: 16
What are the 16 bytes for?
I moved the comment below upwards.
It's copy paste from (now removed) https://review.coreboot.org/c/coreboot/+/37335/7/src/arch/x86/failover.ld
https://review.coreboot.org/c/coreboot/+/37895/2/src/cpu/x86/16bit/reset16.l... PS2, Line 45: 0xf0
I'm not following this sequence either. I think there needs some more explanation.
This was the hard-coded reserve for id.S and fit.S.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
Aaron, ping. Would you want to introduce new (sub)sections such that id.S and fit.S are properly accounted as sections instead of an arbitrary reserve below .reset?
Marshall, auxiliary utilility (flashrom) would like to see id.S at end of flash image. This condition will not be satisfied with amd/picasso where x86 bootblock will not reside at the end of the flash image. Do you want to leave id.S out from amd/picasso bootblock?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(1 comment)
It looks like you're being more aggressive than necessary in adding code within .bootblock_top, e.g. the CAR setup. Is this primarily to optimize space, or maybe I missed something?
Marshall, auxiliary utilility (flashrom) would like to see id.S at end of flash image. This condition will not be satisfied with amd/picasso where x86 bootblock will not reside at the end of the flash image. Do you want to leave id.S out from amd/picasso bootblock?
I'm completely removing the no-bootblock scenario from picasso right now so I need to decide what that ultimately looks like. We'll require a fully functional bootblock.elf to generate the compressed image for the PSP, and currently the arrangement places a traditional (unused) bootblock, with ID etc., at the top of flash in addition to the compressed image in amdfw.rom. The good news is I have bootblock down to about 32K now, and am not reusing the earlyram space for romstage as in the current WIP patches. (I hope to get that stack cleaned up and pushed today.)
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 46: ROMLOC_MIN = CONFIG_X86_RESET_VECTOR - 0xf0 - (PROGRAM_SZ + EARLYASM_SZ); I would've thought this would bee somewhat circular but I can't make it break, I guess I learned something.
Did you mean to insert 2 blank lines above?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
Patch Set 4:
Aaron, ping. Would you want to introduce new (sub)sections such that id.S and fit.S are properly accounted as sections instead of an arbitrary reserve below .reset?
I think that would be helpful. Ideally we can agree on how to construct that upper end for linking purposes w/o wasting space. Basically make the section small enough (w/ an appropriate alignment) to fit the required entries but not large enough that we don't fragment the space leaving it stranded. We can pursue this path soon-ish, but I don't think we're dependent on it at the moment. Let's wait for Marshall to update his patch stack that includes a normal transit through bootblock and romstage for the new boot architecture. Then we can improve things from there.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Aaron, ping. Would you want to introduce new (sub)sections such that id.S and fit.S are properly accounted as sections instead of an arbitrary reserve below .reset?
I think that would be helpful. Ideally we can agree on how to construct that upper end for linking purposes w/o wasting space. Basically make the section small enough (w/ an appropriate alignment) to fit the required entries but not large enough that we don't fragment the space leaving it stranded. We can pursue this path soon-ish, but I don't think we're dependent on it at the moment. Let's wait for Marshall to update his patch stack that includes a normal transit through bootblock and romstage for the new boot architecture. Then we can improve things from there.
The release requirement of !ROMCC_BOOTBLOCK triggered a side-effect when moving from top-aligned to bottom-aligned bootblock, there were some complaints of no longer being able to fit TianoCore payload on some installations with extremely tight SPI flash usage.
This commit targets for removal of a static C_ENV_BOOTBLOCK size. On my loal development trees I have tied the development of reduced-topology static devicetree for the bootblock to this, as well as development for 'CBMEM_CONSOLE-only bootblock, complex-console romstage with replay from CBMEM_CONSOLE'
On your request, this is now put on indefinete hold as well and is waiting for amd/picasso to progress.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
Patch Set 4:
The release requirement of !ROMCC_BOOTBLOCK triggered a side-effect when moving from top-aligned to bottom-aligned bootblock, there were some complaints of no longer being able to fit TianoCore payload on some installations with extremely tight SPI flash usage.
This commit targets for removal of a static C_ENV_BOOTBLOCK size. On my loal development trees I have tied the development of reduced-topology static devicetree for the bootblock to this, as well as development for 'CBMEM_CONSOLE-only bootblock, complex-console romstage with replay from CBMEM_CONSOLE'
On your request, this is now put on indefinete hold as well and is waiting for amd/picasso to progress.
Sorry. I wasn't trying to suggest stopping anything. I was responding to your specific question of adding new sections, but it sounds like my response was taken as sweeping for the whole patch. What are the conflicting changes in this patch vs development of another patch? Can they not proceed in tandem?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37895/4//COMMIT_MSG@8 PS4, Line 8: I think it would be helpful to have more elaborate description explaining reasoning, fixing regressions, etc. Being blank leaves no context for the reviewer.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(1 comment)
Patch Set 4:
Patch Set 4:
The release requirement of !ROMCC_BOOTBLOCK triggered a side-effect when moving from top-aligned to bottom-aligned bootblock, there were some complaints of no longer being able to fit TianoCore payload on some installations with extremely tight SPI flash usage.
This commit targets for removal of a static C_ENV_BOOTBLOCK size. On my loal development trees I have tied the development of reduced-topology static devicetree for the bootblock to this, as well as development for 'CBMEM_CONSOLE-only bootblock, complex-console romstage with replay from CBMEM_CONSOLE'
On your request, this is now put on indefinete hold as well and is waiting for amd/picasso to progress.
Sorry. I wasn't trying to suggest stopping anything. I was responding to your specific question of adding new sections, but it sounds like my response was taken as sweeping for the whole patch. What are the conflicting changes in this patch vs development of another patch? Can they not proceed in tandem?
CB:37490 adds new references to C_ENV_BOOTBLOCK_SIZE. It is not meaningful to try to optimise the actual .text size of bootblock.elf while the allocation from CBFS is a static Kconfig.
https://review.coreboot.org/c/coreboot/+/37895/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37895/4//COMMIT_MSG@8 PS4, Line 8:
I think it would be helpful to have more elaborate description explaining reasoning, fixing regressi […]
Will do when I next have a meaningful chance to rebase on a branch with up-to-date amd/picasso work.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 28: INCLUDE "bootblock/lib/program.ld" Have you given much thought into ensuring a minimum alignment for just the pieces (presumably bootblock.top along w/ reset vector)? And then also ensuring the bss/data comes prior or after depending on option?
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 30: .bootblock_top . : { This sequence will punch a hole between bss/heap and 2 text segments.
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 46: 0xf0 Why the 256 (0xf0 + 16) total offset subtracted from the calculation? I'm not following all the math as to the reasoning.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 46: 0xf0
Why the 256 (0xf0 + 16) total offset subtracted from the calculation? I'm not following all the math […]
It's the reserve for fit.S and id.S AFAIR. Like discussed, those should have proper subsections instead.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/4/src/cpu/x86/16bit/reset16.l... PS4, Line 46: 0xf0
It's the reserve for fit.S and id.S AFAIR. […]
I see. Can you add a comment then so it's clear. I agree, we should push those into certain sections. That would be helpful.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#6).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 66 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/6
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#9).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
TODO: _eprogram should be 4GiB TODO: facebook/fbg1701, soc/amd/picasso
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 66 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 9:
Raul, a related bug (that I cannot access) b:154957411 on amd/picasso.
https://review.coreboot.org/c/coreboot/+/37490/12/src/soc/amd/picasso/Makefi...
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 9:
Patch Set 9:
Raul, a related bug (that I cannot access) b:154957411 on amd/picasso.
https://review.coreboot.org/c/coreboot/+/37490/12/src/soc/amd/picasso/Makefi...
We could always use readelf in the makefile instead of modifying amdfwtool.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/9/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/9/src/cpu/x86/16bit/reset16.l... PS9, Line 44: ( This is going to stomp on the .persistent section used by Picasso. We need to move the persistent section somewhere else.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Raul, a related bug (that I cannot access) b:154957411 on amd/picasso.
https://review.coreboot.org/c/coreboot/+/37490/12/src/soc/amd/picasso/Makefi...
We could always use readelf in the makefile instead of modifying amdfwtool.
I think approach of first creating single amdfw.rom with util/amdfwtool and then placing it with util/cbfstool is not that convenient. You need parts of amdfw.rom in RO and others in RW ?
Due the lack of public PSP (directory table) documentation of metadata/header I cannot really share much of my ideas here. Try to discuss internally if current separation of util/cbfstool/amdcompress and util/amdfwtool works well for you.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/9/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/37895/9/src/cpu/x86/16bit/reset16.l... PS9, Line 44: (
This is going to stomp on the .persistent section used by Picasso. […]
It has not been decided how much of this will get merged. I kind of feel this file could be renamed to bootblock.ld and RESET_VECTOR_IN_RAM=y cases could use different linker script altogether.
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#10).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
TODO: _eprogram should be 4GiB TODO: facebook/fbg1701, soc/amd/picasso
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 64 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/10
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#11).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
TODO: soc/amd/picasso
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 64 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/11
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 11:
tested on NetBurst cpuid 0xf65 (and i945GC/ich7) here is the log (spew - hope it can help) : https://www.dropbox.com/s/1mjyb585m1grckc/37895.log?dl=0
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37895/11/src/cpu/x86/16bit/entry16.... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/37895/11/src/cpu/x86/16bit/entry16.... PS11, Line 31: reachable be reachable?
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37895
to look at the new patch set (#12).
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/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 M src/cpu/intel/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 62 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37895/12
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37895 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
HAOUAS Elyes (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Raul Rangel (Code Review)
-
Werner Zeh (Code Review)