Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
[NOT FOR MERGE]: Demo adding near_reset_vector symbol
In https://review.coreboot.org/c/coreboot/+/35035/8/src/arch/x86/early_dram.ld we had a discussion about whether an approach like for RESET_VECTOR_IN_RAM could potentially be used for a bootblock that needed to grow >64KB (to keep the first 16-bit jmp within reach).
This is a mockup of the test I ran to verify that would be practical. Using this code I built a google/grunt and recorded the following with readelf. Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS ffff0000 000060 0059d0 00 AX 0 0 32 [ 2] .rel.text REL 00000000 01335c 001378 08 I 10 1 4 [ 3] .car.data NOBITS 00030000 000000 005860 00 WA 0 0 4 [ 4] .near_reset_vecto PROGBITS fffffe00 00fe60 0000bb 00 AX 0 0 4 [ 5] .rel.near_reset_v REL 00000000 0146d4 000040 08 I 10 4 4 [ 6] .reset PROGBITS fffffff0 010050 000010 00 AX 0 0 1 [ 7] .rel.reset REL 00000000 014714 000008 08 I 10 6 4 [ 8] .id PROGBITS ffffff4a 00ffaa 000036 00 A 0 0 1 [ 9] .gnu_debuglink PROGBITS 00000000 010060 000014 00 0 0 4 [10] .symtab SYMTAB 00000000 010074 001a80 10 11 243 4 [11] .strtab STRTAB 00000000 011af4 001868 00 0 0 1 [12] .shstrtab STRTAB 00000000 01471c 000064 00 0 0 1
Note, however, that I did not grow the bootblock.elf file; only inserted the section and relocated the _start16bit code.
fffffe00 <_start16bit>: fffffe00: fa cli fffffe01: 66 89 c5 mov %ax,%bp fffffe04: b0 01 mov $0x1,%al fffffe06: e6 80 out %al,$0x80 fffffe08: 66 31 c0 xor %ax,%ax fffffe0b: 0f 22 d8 mov %eax,%cr3 fffffe0e: 8c c8 mov %cs,%eax fffffe10: c1 e0 04 shl $0x4,%eax fffffe13: bb 4c fe 29 c3 mov $0xc329fe4c,%ebx fffffe18: 2e 0f 01 1f lidtl %cs:(%edi) fffffe1c: bb 44 fe 29 c3 mov $0xc329fe44,%ebx fffffe21: 2e 66 0f 01 17 lgdtw %cs:(%edi) fffffe26: 0f 20 c0 mov %cr0,%eax fffffe29: 66 25 d1 ff and $0xffd1,%ax fffffe2d: fa cli fffffe2e: 7f 66 jg fffffe96 <__protected_start+0xb> fffffe30: 0d 01 00 00 60 or $0x60000001,%eax fffffe35: 0f 22 c0 mov %eax,%cr0 fffffe38: 66 89 e8 mov %bp,%ax fffffe3b: 66 ea 8b fe ff ff ljmpw $0xffff,$0xfe8b fffffe41: 08 00 or %al,(%eax) fffffe43: 90 nop
Growing the elf should be reasonably straightforward. Right now reset16.ld has a built-in assumption of 64KB. For RESET_VECTOR_IN_RAM I've added a size symbol to Kconfig (yet to land still) for locating the reset vector and near_reset_vector at the very top, and so that the calculations are expected to match the instructions given to the PSP.
Change-Id: I6e7888c778e1d1cc426e4160543f4a4662ebf834 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/36418/1
diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 9e00c55..6062abd 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -37,6 +37,9 @@ .align 4096 #endif .code16 + +.section ".near_reset_vector", "ax", %progbits + .globl _start16bit .type _start16bit, @function
diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index c57cc96..31716e0 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -17,6 +17,14 @@ */
SECTIONS { + _ROMTOP = 0xfffffff0; + _NEAR_RESET_VECTOR = _ROMTOP + 0x10 - 0x200; + + . = _NEAR_RESET_VECTOR; + .near_reset_vector . : { + *(.near_reset_vector); + } + /* Trigger an error if I have an unuseable start address */ _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); _ROMTOP = 0xfffffff0;
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
I'm not too surprised by the build errors: LINK cbfs/fallback/bootblock.debug i386-elf-ld.bfd: section .rom LMA [00000000fffffea0,00000000fffffeef] overlaps section .near_reset_vector LMA [00000000fffffe00,00000000fffffec1]
I needed to provide more room on grunt to prevent the overlap.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG@35 PS1, Line 35: fffffe00 <_start16bit>: You want objdump -M 8086 here
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG@35 PS1, Line 35: fffffe00 <_start16bit>:
You want objdump -M 8086 here
Oh, that's right. I was more concerned with showing where _start16bit was, and /searched down to "Disassembly of section .near_reset_vector:" (omitted). If there's any reason to repush the patch, I'll redo the text.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
Build test with aopen/dxplplusu shows regression.
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function With SIPI_VECTOR_IN_ROM _start16bit alignment has to be enforced. IPI message using ap_sipi_vector_in_rom discards bottom 12 bits of address (see entry16.ld). Yes, it is only old P4 that really needs it, but there is still some activity with such ancient platform.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR 0xfffff000 for ap_sipi_vector_in_rom ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
0xfffff000 for ap_sipi_vector_in_rom ?
Well it can't be any higher. Seems to waste some 3 KiB though, unless we pushed other asm source into that section too (walkcbfs.S microcode_asm.S cache_as_ram.S).
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
Well it can't be any higher. Seems to waste some 3 KiB though, unless we pushed other asm source into that section too (walkcbfs.S microcode_asm.S cache_as_ram.S).
with C_ENVIRONMENT_BOOTBLOCK a lot (still relatively little in comparison with todays flash sizes) of waste is expected in the bootblock as the region is bottom aligned. Might as well make use of it?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function
With SIPI_VECTOR_IN_ROM _start16bit alignment has to be enforced. […]
Is it not? The .align 4096 is sitting up there still on line 37.
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 20: _ROMTOP If we are going to define this symbol here we should remove it from line 30?
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
Well it can't be any higher. […]
The function is aligned to 4096 so yes, it will just pad things, but I don't think the linker will link with those 2 constraints (is that what jenkins was complaining about?).
I believe one needs to align down program counter to 4KiB based on CONFIG(SIPI_VECTOR_IN_ROM) option.
the other thing to incorporate correctly is the fit pointer. It lives at 0xffffffc0 and occupies 8 bytes. It's linker definition is being included conditionally (src/cpu/intel/fit/fit.ld), but this code should not collide with it.
That and we have this default which can encroach on all kinds of stuff as it seems to but up against 0xffffff40 in not the cleanest way: config ID_SECTION_OFFSET
-------hex -------default 0x80
I think the linkers are currently limiting in that they always assume an increasing program counter which we actually want to allocate downwards for these types of things. I ran into this problem trying to get data sections to work in CAR stages that I never fully solved.
That said, I'm thinking we should manually allocate these sections? We're trying to compose everything through #includes (src/arch/x86/memlayout.ld and src/arch/x86/bootblock.ld), but that might be the wrong approach. I'm not sure what the linker will do w/o having a strarting program address but then specify these higher addresses later. I'm beginning to wonder that we should specify alignment constraints for the associated sections, link at a low enough address and then relocate to the final destination. This is what we did for XIP romstage, but we've left bootblock the same. I'm thinking we should go down a different route so we can work around the linker limitations. Thoughts?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
I think the linkers are currently limiting in that they always assume an increasing program counter which we actually want to allocate downwards for these types of things. I ran into this problem trying to get data sections to work in CAR stages that I never fully solved.
That said, I'm thinking we should manually allocate these sections? We're trying to compose everything through #includes (src/arch/x86/memlayout.ld and src/arch/x86/bootblock.ld), but that might be the wrong approach. I'm not sure what the linker will do w/o having a strarting program address but then specify these higher addresses later. I'm beginning to wonder that we should specify alignment constraints for the associated sections, link at a low enough address and then relocate to the final destination. This is what we did for XIP romstage, but we've left bootblock the same. I'm thinking we should go down a different route so we can work around the linker limitations. Thoughts?
I like that approach. It would remove the need to specify the bootblock size. Are alignments something that can be/ are taken care of when relocating the program?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
I like that approach. It would remove the need to specify the bootblock size. Are alignments something that can be/ are taken care of when relocating the program?
On the first link all alignment requests will be honored. We'll get a size for the whole bootblock program. We can then relocate the whole thing to final destination.
One thing we need to look out for are those absolute addresses. If the max alignment is correctly honored (and those absolute addresses have less than the max alignments) then I think we can just move everything together to the final destination. That's what's required to make such a scheme work. Need to choose proper starting address (fake base one that gets changed on relocation) and then I think we just then relocate everything together.
It's certainly something to play with and see how it pans out.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
A bit off-topic, but I think the development effort should be in providing good methods for equivalent of normal/fallback boot flows with and without VBOOT installed. Should we support configuration where bootblock has only CBMEM console enabled, while SPI/EHCI consoles would be delayed to (fallback) romstage? Assuming one does not completely mess up the build of an experimental romstage, bootblock and VBOOT debug output from CBMEM could be replayed to SPI/EHCI even before raminit? This would often bring bootblock size downto 16 KiB or less, while not having undesirable speed impact for 'stable or normal' boot flow.
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function
Is it not? The .align 4096 is sitting up there still on line 37.
The section is defined with an absolute start address in reset16.ld.
fffffe00 A _NEAR_RESET_VECTOR fffffe00 T _start16bit
and
2 .near_reset_vector 000000bb fffffe00 fffffe00 00003e54 2**2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function
The section is defined with an absolute start address in reset16.ld. […]
Sure. But that's incorrect which is what I think the point of your comment. anyway, we can sort that bit out and fix it.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
A bit off-topic, but I think the development effort should be in providing good methods for equivalent of normal/fallback boot flows with and without VBOOT installed. Should we support configuration where bootblock has only CBMEM console enabled, while SPI/EHCI consoles would be delayed to (fallback) romstage? Assuming one does not completely mess up the build of an experimental romstage, bootblock and VBOOT debug output from CBMEM could be replayed to SPI/EHCI even before raminit? This would often bring bootblock size downto 16 KiB or less, while not having undesirable speed impact for 'stable or normal' boot flow.
Yes, please! That should only be a matter of changing the condition of the linker scripts. On that same topic, would it not be a good idea for the bootblock to conditionally set a global parameter to output over slower consoles. For instance if the bootblock detects X amount of failures set that parameter that affects next stages.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: [NOT FOR MERGE]: Demo adding near_reset_vector symbol ......................................................................
Patch Set 1:
On that same topic, would it not be a good idea for the bootblock to conditionally set a global parameter to output over slower consoles. For instance if the bootblock detects X amount of failures set that parameter that affects next stages.
Many aspects with consoles were hard and complicated with LATE_CBMEM_INIT and CAR_GLOBAL_MIGRATION around. Today, such runtime enablements should be easy and clean to accomplish, even more so after CAR_GLOBAL removal.
Hello Julius Werner, Arthur Heymans, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36418
to look at the new patch set (#2).
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
cpu/x86: Introduce .near_reset_vector
Add a section to ensure _start16bit is always within 64KB of the reset vector. Otherwise the first jmp may not be reachable in a larger program. Restrict its use only where it becomes necessary.
TEST=Artificially increase google/grunt's C_ENV_BOOTBLOCK_SIZE to 128KB and compare _start16bit at 0xfffe0000 vs. 0xfffff000 before/after.
Change-Id: I6e7888c778e1d1cc426e4160543f4a4662ebf834 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/36418/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 3:
(4 comments)
Acking all the old comments on the hack PS1. Since I added .near_reset_vector as a real patch, I thought it would be better to use the same Change-ID vs. using a fresh one.
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36418/1//COMMIT_MSG@35 PS1, Line 35: fffffe00 <_start16bit>:
Oh, that's right. […]
Acking but not sure it's important for the commit message anymore. As I mentioned in the newer version, it's reproducible by bumping C_ENV_BOOTBLOCK_SIZE over 64K.
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function
Sure. But that's incorrect which is what I think the point of your comment. […]
Address was changed by how high I'd put it originally and that's changed now. Also CB:30855 and CB:37154 have since touched the alignment.
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 20: _ROMTOP
If we are going to define this symbol here we should remove it from line 30?
Ack
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/reset16.l... PS1, Line 21: _NEAR_RESET_VECTOR
[…]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... PS3, Line 34: #endif I would drop the C_ENV_BOOTBLOCK_SIZE condition here. The assembly we put in .near_reset_vector has to fit that section (<4 KiB) or otherwise builds attempting to increase the bootblock size can fail.
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... PS3, Line 41: .align 4096 I understand some people are a bit upset having the bootblock size increased, only SIPI_VECTOR_IN_ROM needs the .align here.
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif I think you could do all of the above unconditionally. Some people with very tight flashspace would like to see the entire bootblock top-aligned.
Hello Julius Werner, Arthur Heymans, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36418
to look at the new patch set (#4).
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
cpu/x86: Introduce .near_reset_vector
Add a section to ensure _start16bit is always within 64KB of the reset vector. Otherwise the first jmp may not be reachable in a larger program. Restrict its use only where it becomes necessary.
TEST=Artificially increase google/grunt's C_ENV_BOOTBLOCK_SIZE to 128KB and compare _start16bit at 0xfffe0000 vs. 0xfffff000 before/after.
Change-Id: I6e7888c778e1d1cc426e4160543f4a4662ebf834 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld 2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/36418/4
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/1/src/cpu/x86/16bit/entry16.i... PS1, Line 44: .type _start16bit, @function
Address was changed by how high I'd put it originally and that's changed now. […]
Ack
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... PS3, Line 34: #endif
I would drop the C_ENV_BOOTBLOCK_SIZE condition here. The assembly we put in . […]
I'll give it a try. Have successfully built a number of boards with smaller sizes, so hopefully it'll fly.
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/entry16.i... PS3, Line 41: .align 4096
I understand some people are a bit upset having the bootblock size increased, only SIPI_VECTOR_IN_RO […]
N/A now
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif
I think you could do all of the above unconditionally. […]
Same as in the other comment. Seems to be working ok locally.
Hello Julius Werner, Arthur Heymans, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36418
to look at the new patch set (#5).
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
cpu/x86: Introduce .near_reset_vector
Add a section to ensure _start16bit is always within 64KB of the reset vector. Otherwise the first jmp may not be reachable in a larger program. Restrict its use only where it becomes necessary.
TEST=Artificially increase google/grunt's C_ENV_BOOTBLOCK_SIZE to 128KB and compare _start16bit at 0xfffe0000 vs. 0xfffff000 before/after.
Change-Id: I6e7888c778e1d1cc426e4160543f4a4662ebf834 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld 2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/36418/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif
Same as in the other comment. Seems to be working ok locally.
Builds were section .near_reset_vector (for any reason) grows close to 4 KiB would fail to build if C_ENV_BOOTBLOCK_SIZE increases over 64KiB. It's just cleaner without the conditional to force the 16bit execution environment always in the top (4 KiB) section.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif
Builds were section . […]
I agree it's cleaner without, however there were Apollo Lake boards failing the build. (Sorry if you already noticed that.) I don't know much about that tech, but Kconfig sets C_ENV_BOOTBLOCK_SIZE to 0x8000 and has a comment "32KiB bootblock is all that is mapped in by the CSE at top of 4GiB.". I infer APL cannot be larger than 0x8000 and doesn't have enough room for a 4K section at the top.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif
I agree it's cleaner without, however there were Apollo Lake boards failing the build. […]
I'll have a look at APL and intel/d945gcfl then.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 5:
I have started CB:37895 as an approach to drop static C_ENV_BOOTBLOCK_SIZE and align the required x86 realmode/assembly parts towards the end of the bootblock.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 5:
(1 comment)
I have started CB:37895 as an approach to drop static C_ENV_BOOTBLOCK_SIZE and align the required x86 realmode/assembly parts towards the end of the bootblock.
OK, will spend some time reviewing that.
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/36418/3/src/cpu/x86/16bit/reset16.l... PS3, Line 29: #endif
I'll have a look at APL and intel/d945gcfl then.
Ack
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 6:
I've spent a few minutes replacing this with CB:37895 but am still missing something important.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Patch Set 6: Code-Review-1
This change is no longer needed for Picasso. Only ~32K is my requirement currently.
Although this change provides some value, I think it can be jettisoned for a more creative solution later.
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36418 )
Change subject: cpu/x86: Introduce .near_reset_vector ......................................................................
Abandoned
This is no longer required for any product in development.
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)