Change in coreboot[master]: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector"
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector"
This partially reverts commit 67910db907fb3d5feacdbfaa40952a88f673795a.
The symbol X86_RESET_VECTOR continues to live, for the time being, under soc/amd/picasso.
Change-Id: Ib6b2cc2b17133b3207758c72a54abe80fc6356b5 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/Kconfig M src/arch/x86/id.ld M src/arch/x86/memlayout.ld M src/cpu/x86/16bit/reset16.ld 4 files changed, 9 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/42348/1
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 18c1ed3..35f63ac 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -73,16 +73,6 @@ 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/id.ld b/src/arch/x86/id.ld index ea8d7e9..b69a8dc 100644 --- a/src/arch/x86/id.ld +++ b/src/arch/x86/id.ld @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
SECTIONS { - . = (CONFIG_X86_RESET_VECTOR - CONFIG_ID_SECTION_OFFSET) + 0x10 - (__id_end - __id_start); + . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; .id (.): { KEEP(*(.id)) } diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 12974ca..6f7ac89 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -29,7 +29,7 @@
#include "car.ld" #elif ENV_BOOTBLOCK - BOOTBLOCK(CONFIG_X86_RESET_VECTOR - CONFIG_C_ENV_BOOTBLOCK_SIZE + 0x10, + BOOTBLOCK(0xffffffff - CONFIG_C_ENV_BOOTBLOCK_SIZE + 1, CONFIG_C_ENV_BOOTBLOCK_SIZE)
#include "car.ld" diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index b90dd04..e00e0b4 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -1,13 +1,15 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-/* _RESET_VECTOR: typically the top of the ROM */ +/* + * _ROMTOP : The top of the ROM used where we + * need to put the reset vector. + */
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."); - - . = CONFIG_X86_RESET_VECTOR; + _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); + _ROMTOP = 0xfffffff0; + . = _ROMTOP; .reset . : { *(.reset); . = 15;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
CP bootblock/soc/amd/picasso/memlayout.ld i386-elf-ld.bfd: _start16bit too low. Please report.
So it picks >cpu/x86/16bit/reset16.ld>. While correct, it means means the somewhat ugly X86_RESET_VECTOR, which as a literal constant for 20 odd years, now would need to be defined outside picasso too.
Can you drop <cpu/x86/entry16.ld> and <cpu/x86/reset16.ld> from the mentionded file instead?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
Patch Set 1:
CP bootblock/soc/amd/picasso/memlayout.ld i386-elf-ld.bfd: _start16bit too low. Please report.
So it picks >cpu/x86/16bit/reset16.ld>. While correct, it means means the somewhat ugly X86_RESET_VECTOR, which as a literal constant for 20 odd years, now would need to be defined outside picasso too.
Can you drop <cpu/x86/entry16.ld> and <cpu/x86/reset16.ld> from the mentionded file instead?
Sure, I can take a look.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
CP bootblock/soc/amd/picasso/memlayout.ld i386-elf-ld.bfd: _start16bit too low. Please report.
So it picks >cpu/x86/16bit/reset16.ld>. While correct, it means means the somewhat ugly X86_RESET_VECTOR, which as a literal constant for 20 odd years, now would need to be defined outside picasso too.
Can you drop <cpu/x86/entry16.ld> and <cpu/x86/reset16.ld> from the mentionded file instead?
Sure, I can take a look.
I haven't looked at the entire patch stack, but can't we put this section aligned to 16 bytes as the last thing. Then we can relocate adding to cbfs? It should all just work from there w/o manually specifying the location in the linker script.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
I haven't looked at the entire patch stack, but can't we put this section aligned to 16 bytes as the last thing. Then we can relocate adding to cbfs? It should all just work from there w/o manually specifying the location in the linker script.
I have somewhat major rework in followups to get top-aligned bootblock for more efficient FLASH usage. Might be good to just separate PCO from the legacy layout completely.
There is CB:40876 as I though the separation of the linker scripts was unnecessary.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
Patch Set 1:
I haven't looked at the entire patch stack, but can't we put this section aligned to 16 bytes as the last thing. Then we can relocate adding to cbfs? It should all just work from there w/o manually specifying the location in the linker script.
I have somewhat major rework in followups to get top-aligned bootblock for more efficient FLASH usage. Might be good to just separate PCO from the legacy layout completely.
There is CB:40876 as I though the separation of the linker scripts was unnecessary.
OK. I need to go through your stack still, but I'm sure we can align things. I would expect PCO to link in the same way, but we may need to split to make things easier in the interim.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I haven't looked at the entire patch stack, but can't we put this section aligned to 16 bytes as the last thing. Then we can relocate adding to cbfs? It should all just work from there w/o manually specifying the location in the linker script.
I have somewhat major rework in followups to get top-aligned bootblock for more efficient FLASH usage. Might be good to just separate PCO from the legacy layout completely.
There is CB:40876 as I though the separation of the linker scripts was unnecessary.
OK. I need to go through your stack still, but I'm sure we can align things. I would expect PCO to link in the same way, but we may need to split to make things easier in the interim.
Any updates? Increased bootblock size with C_ENVIRONMENT_BOOTBLOCK was regression for some, top-aligned bootblock has now been blocked for 6 months, waiting on amd/picasso bootblock linking to get sorted out.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
I haven't looked at the entire patch stack, but can't we put this section aligned to 16 bytes as the last thing. Then we can relocate adding to cbfs? It should all just work from there w/o manually specifying the location in the linker script.
I have somewhat major rework in followups to get top-aligned bootblock for more efficient FLASH usage. Might be good to just separate PCO from the legacy layout completely.
There is CB:40876 as I though the separation of the linker scripts was unnecessary.
OK. I need to go through your stack still, but I'm sure we can align things. I would expect PCO to link in the same way, but we may need to split to make things easier in the interim.
Any updates? Increased bootblock size with C_ENVIRONMENT_BOOTBLOCK was regression for some, top-aligned bootblock has now been blocked for 6 months, waiting on amd/picasso bootblock linking to get sorted out.
Done here: https://review.coreboot.org/c/coreboot/+/43281. But I think there might be some more changes required as your current stack is pushed forward. I still have to look at the entire series here.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42348 )
Change subject: Revert "arch|cpu/x86: Add Kconfig option for x86 reset vector" ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)