Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
arch|cpu/x86: Add Kconfig option for x86 reset vector
Prepare for an implementation supporting the reset vector in RAM and not the traditional 0xfffffff0. Add a Kconfig symbol that can be used in place of hardcoded values.
Change-Id: I6a814f7179ee4251aeeccb2555221616e944e03d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/failover.ld M src/arch/x86/id.ld M src/arch/x86/memlayout.ld M src/cpu/intel/fit/fit.ld M src/cpu/x86/16bit/reset16.ld 6 files changed, 19 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37485/1
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 0e6f486..e27aec2 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -87,6 +87,16 @@ default n depends on ARCH_X86 && SMP
+config X86_RESET_VECTOR + hex + depends on ARCH_X86 + default 0xfffffff0 + help + Specify the location of the x86 reset vector. In traditional devices + this must match the architectural reset vector to produce a bootable + image. Nontraditional designs may use this to position the reset + vector into its desired location. + config RESET_VECTOR_IN_RAM bool depends on ARCH_X86 diff --git a/src/arch/x86/failover.ld b/src/arch/x86/failover.ld index 334145a..8015d66 100644 --- a/src/arch/x86/failover.ld +++ b/src/arch/x86/failover.ld @@ -14,7 +14,7 @@ ENTRY(_start)
MEMORY { - rom : ORIGIN = 0xffff0000, LENGTH = 64K + rom : ORIGIN = (CONFIG_X86_RESET_VECTOR - 0xfff0, LENGTH = 64K }
TARGET(binary) diff --git a/src/arch/x86/id.ld b/src/arch/x86/id.ld index 2a50f9c..3d9ef37 100644 --- a/src/arch/x86/id.ld +++ b/src/arch/x86/id.ld @@ -12,7 +12,7 @@ */
SECTIONS { - . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; + . = (CONFIG_X86_RESET_VECTOR - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 0x10; .id (.): { KEEP(*(.id)) } diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index f8ae9f3..b14fd62 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -50,7 +50,7 @@ #include EARLY_MEMLAYOUT #elif ENV_BOOTBLOCK /* arch/x86/bootblock.ld contains the logic for the ROMCC_BOOTBLOCK linking. */ - BOOTBLOCK(0xffffffff - CONFIG_C_ENV_BOOTBLOCK_SIZE + 1, + BOOTBLOCK(CONFIG_X86_RESET_VECTOR - CONFIG_C_ENV_BOOTBLOCK_SIZE + 0x10, CONFIG_C_ENV_BOOTBLOCK_SIZE)
#include EARLY_MEMLAYOUT diff --git a/src/cpu/intel/fit/fit.ld b/src/cpu/intel/fit/fit.ld index 6e30ea1..2e65186 100644 --- a/src/cpu/intel/fit/fit.ld +++ b/src/cpu/intel/fit/fit.ld @@ -12,7 +12,7 @@ */
SECTIONS { - . = 0xffffffc0; + . = CONFIG_X86_RESET_VECTOR - 0x30; /* 0xffffffc0 */ .fit_pointer (.): { KEEP(*(.fit_pointer)) } diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index c57cc96..ec01810 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -11,16 +11,14 @@ * GNU General Public License for more details. */
-/* - * _ROMTOP : The top of the ROM used where we - * need to put the reset vector. - */ +/* _RESET_VECTOR: typically the top of the ROM */
SECTIONS { /* Trigger an error if I have an unuseable start address */ - _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); - _ROMTOP = 0xfffffff0; - . = _ROMTOP; + _TOO_LOW = CONFIG_X86_RESET_VECTOR - 0xfff0; + _bogus = ASSERT(_start16bit >= _TOO_LOW, "_start16bit too low. Please report."); + + . = CONFIG_X86_RESET_VECTOR; .reset . : { *(.reset); . = 15;
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37485
to look at the new patch set (#2).
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
arch|cpu/x86: Add Kconfig option for x86 reset vector
Prepare for an implementation supporting the reset vector in RAM and not the traditional 0xfffffff0. Add a Kconfig symbol that can be used in place of hardcoded values.
Change-Id: I6a814f7179ee4251aeeccb2555221616e944e03d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/failover.ld M src/arch/x86/id.ld M src/arch/x86/memlayout.ld M src/cpu/intel/fit/fit.ld M src/cpu/x86/16bit/reset16.ld 6 files changed, 19 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/37485/2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
arch|cpu/x86: Add Kconfig option for x86 reset vector
Prepare for an implementation supporting the reset vector in RAM and not the traditional 0xfffffff0. Add a Kconfig symbol that can be used in place of hardcoded values.
Change-Id: I6a814f7179ee4251aeeccb2555221616e944e03d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37485 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/arch/x86/Kconfig M src/arch/x86/failover.ld M src/arch/x86/id.ld M src/arch/x86/memlayout.ld M src/cpu/intel/fit/fit.ld M src/cpu/x86/16bit/reset16.ld 6 files changed, 19 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 0e6f486..e27aec2 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -87,6 +87,16 @@ default n depends on ARCH_X86 && SMP
+config X86_RESET_VECTOR + hex + depends on ARCH_X86 + default 0xfffffff0 + help + Specify the location of the x86 reset vector. In traditional devices + this must match the architectural reset vector to produce a bootable + image. Nontraditional designs may use this to position the reset + vector into its desired location. + config RESET_VECTOR_IN_RAM bool depends on ARCH_X86 diff --git a/src/arch/x86/failover.ld b/src/arch/x86/failover.ld index 334145a..1391365 100644 --- a/src/arch/x86/failover.ld +++ b/src/arch/x86/failover.ld @@ -14,7 +14,7 @@ ENTRY(_start)
MEMORY { - rom : ORIGIN = 0xffff0000, LENGTH = 64K + rom : ORIGIN = CONFIG_X86_RESET_VECTOR - 0xfff0, LENGTH = 64K }
TARGET(binary) diff --git a/src/arch/x86/id.ld b/src/arch/x86/id.ld index 2a50f9c..3d9ef37 100644 --- a/src/arch/x86/id.ld +++ b/src/arch/x86/id.ld @@ -12,7 +12,7 @@ */
SECTIONS { - . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; + . = (CONFIG_X86_RESET_VECTOR - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 0x10; .id (.): { KEEP(*(.id)) } diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index f8ae9f3..b14fd62 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -50,7 +50,7 @@ #include EARLY_MEMLAYOUT #elif ENV_BOOTBLOCK /* arch/x86/bootblock.ld contains the logic for the ROMCC_BOOTBLOCK linking. */ - BOOTBLOCK(0xffffffff - CONFIG_C_ENV_BOOTBLOCK_SIZE + 1, + BOOTBLOCK(CONFIG_X86_RESET_VECTOR - CONFIG_C_ENV_BOOTBLOCK_SIZE + 0x10, CONFIG_C_ENV_BOOTBLOCK_SIZE)
#include EARLY_MEMLAYOUT diff --git a/src/cpu/intel/fit/fit.ld b/src/cpu/intel/fit/fit.ld index 6e30ea1..2e65186 100644 --- a/src/cpu/intel/fit/fit.ld +++ b/src/cpu/intel/fit/fit.ld @@ -12,7 +12,7 @@ */
SECTIONS { - . = 0xffffffc0; + . = CONFIG_X86_RESET_VECTOR - 0x30; /* 0xffffffc0 */ .fit_pointer (.): { KEEP(*(.fit_pointer)) } diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index c57cc96..ec01810 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -11,16 +11,14 @@ * GNU General Public License for more details. */
-/* - * _ROMTOP : The top of the ROM used where we - * need to put the reset vector. - */ +/* _RESET_VECTOR: typically the top of the ROM */
SECTIONS { /* Trigger an error if I have an unuseable start address */ - _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); - _ROMTOP = 0xfffffff0; - . = _ROMTOP; + _TOO_LOW = CONFIG_X86_RESET_VECTOR - 0xfff0; + _bogus = ASSERT(_start16bit >= _TOO_LOW, "_start16bit too low. Please report."); + + . = CONFIG_X86_RESET_VECTOR; .reset . : { *(.reset); . = 15;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
Patch Set 3:
(3 comments)
I am late for the review... -1 from me.
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld File src/arch/x86/failover.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld@17 PS3, Line 17: rom : ORIGIN = CONFIG_X86_RESET_VECTOR - 0xfff0, LENGTH = 64K This file is for romcc bootblocks only, I think you should have just leave this untouched.
The number 0xfffffff0 below is in some way tied to the default reset vector location so this script probably has some conflict now with a different X86_RESET_VECTOR value.
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/id.ld File src/arch/x86/id.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/id.ld@16 PS3, Line 16: .id (.): { I thought ID_SECTION_OFFSET is "defined" to be offset from the end of non-volatile memory and not really part of the bootblock stage.
https://review.coreboot.org/c/coreboot/+/37485/3/src/cpu/intel/fit/fit.ld File src/cpu/intel/fit/fit.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/cpu/intel/fit/fit.ld@16 PS3, Line 16: .fit_pointer (.): { This should never move?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
Patch Set 3:
(3 comments)
I am late for the review... -1 from me.
I've been meaning to ask what you'd like to do with this change.
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld File src/arch/x86/failover.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld@17 PS3, Line 17: rom : ORIGIN = CONFIG_X86_RESET_VECTOR - 0xfff0, LENGTH = 64K
This file is for romcc bootblocks only, I think you should have just leave this untouched. […]
I'd missed the one below. I'm OK with making the one above hardcoded again.
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/id.ld File src/arch/x86/id.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/id.ld@16 PS3, Line 16: .id (.): {
I thought ID_SECTION_OFFSET is "defined" to be offset from the end of non-volatile memory and not re […]
You might be right. I looked back to ~2012 and didn't see much in the way of a definition.
Having this here currently ensures the linking works properly when picasso's build is bootblock/romstage. However, I could #if this file out memlayout.ld when RESET_VECTOR_IN_RAM.
https://review.coreboot.org/c/coreboot/+/37485/3/src/cpu/intel/fit/fit.ld File src/cpu/intel/fit/fit.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/cpu/intel/fit/fit.ld@16 PS3, Line 16: .fit_pointer (.): {
This should never move?
I believe that's right, and X86_RESET_VECTOR should always be fffffff0 in Intel systems. This file is only included when CONFIG(CPU_INTEL_FIRMWARE_INTERFACE_TABLE). I'd be fine hardcoding it again.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37485 )
Change subject: arch|cpu/x86: Add Kconfig option for x86 reset vector ......................................................................
Patch Set 3:
(1 comment)
pushed CB:37877
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld File src/arch/x86/failover.ld:
https://review.coreboot.org/c/coreboot/+/37485/3/src/arch/x86/failover.ld@17 PS3, Line 17: rom : ORIGIN = CONFIG_X86_RESET_VECTOR - 0xfff0, LENGTH = 64K
I'd missed the one below. I'm OK with making the one above hardcoded again.
N/A now.
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)