Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
[WIP] arch/x86: Clean up bootblock assembly
We hae identical gdtptr16 and gdtptr. The reference in gdtptr_offset calculation is not accounted for when considering --gc-sections, so to support linking gdt_init.S separately add dummy use of gdtptr symbol.
Realmode execution already accessed gdt that was located outside [_start16bit,_estart16bit] region. Remove latter symbol as the former was not really a start of region, but entry point symbol.
With the romcc bootblock solution, entry32.inc may have been linked into romstage once.
Change-Id: I0a3f6aeb217ca4e38b936b8c9ec8b0b69732cbb9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock.ld M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/32bit/entry32.inc M src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld 4 files changed, 6 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/47964/1
diff --git a/src/arch/x86/bootblock.ld b/src/arch/x86/bootblock.ld index ff7bb62..4a3dbb9 100644 --- a/src/arch/x86/bootblock.ld +++ b/src/arch/x86/bootblock.ld @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-gdtptr16_offset = gdtptr16 & 0xffff; +gdtptr_offset = gdtptr & 0xffff; nullidt_offset = nullidt & 0xffff;
/* Symbol _start16bit must be aligned to 4kB to start AP CPUs with diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 13d12be..2665cc6 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -108,7 +108,7 @@ movw $nullidt_offset, %bx subw %ax, %bx lidt %cs:(%bx) - movw $gdtptr16_offset, %bx + movw $gdtptr_offset, %bx subw %ax, %bx lgdtl %cs:(%bx)
@@ -127,11 +127,8 @@ * The gdt is defined in entry32.inc, it has a 4 Gb code segment * at 0x08, and a 4 GB data segment at 0x10; */ -.align 4 -.globl gdtptr16 -gdtptr16: - .word gdt_end - gdt -1 /* compute the table limit */ - .long gdt /* we know the offset */ +__gdtptr: + .long gdtptr
.align 4 .globl nullidt @@ -139,7 +136,3 @@ .word 0 /* limit */ .long 0 .word 0 - -.globl _estart16bit -_estart16bit: - .code32 diff --git a/src/cpu/x86/32bit/entry32.inc b/src/cpu/x86/32bit/entry32.inc index be67b53..8509448 100644 --- a/src/cpu/x86/32bit/entry32.inc +++ b/src/cpu/x86/32bit/entry32.inc @@ -8,29 +8,11 @@
.code32 /* - * When we come here we are in protected mode. We expand - * the stack and copies the data segment from ROM to the - * memory. - * - * After that, we call the chipset bootstrap routine that - * does what is left of the chipset initialization. - * + * When we come here we are in protected mode. * NOTE aligned to 4 so that we are sure that the prefetch * cache will be reloaded. - * - * In the bootblock there is already a ljmp to __protected_start and - * the reset vector jumps to symbol _start16bit in entry16.inc from - * the reset vectors's symbol which is _start. Therefore, don't - * expose the _start symbol for bootblock. */ .align 4 -#if !ENV_BOOTBLOCK -.globl _start -_start: -#endif - - lgdt %cs:gdtptr - ljmp $ROM_CODE_SEG, $__protected_start
__protected_start: /* Save the BIST value */ diff --git a/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld b/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld index 369d431..c2b37f8 100644 --- a/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld +++ b/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld @@ -98,7 +98,7 @@
#if ENV_BOOTBLOCK
-gdtptr16_offset = gdtptr16 & 0xffff; +gdtptr_offset = gdtptr & 0xffff; nullidt_offset = nullidt & 0xffff;
SECTIONS {
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47964/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47964/1//COMMIT_MSG@9 PS1, Line 9: hae have
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47964
to look at the new patch set (#2).
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
[WIP] arch/x86: Clean up bootblock assembly
We have identical gdtptr16 and gdtptr. The reference in gdtptr_offset calculation is not accounted for when considering --gc-sections, so to support linking gdt_init.S separately add dummy use of gdtptr symbol.
Realmode execution already accessed gdt that was located outside [_start16bit,_estart16bit] region. Remove latter symbol as the former was not really a start of region, but entry point symbol.
With the romcc bootblock solution, entry32.inc may have been linked into romstage once.
Change-Id: I0a3f6aeb217ca4e38b936b8c9ec8b0b69732cbb9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock.ld M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/32bit/entry32.inc M src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld 4 files changed, 6 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/47964/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 2: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 2:
i'll try to test this on Mandolin later today
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 2:
Patch Set 2:
i'll try to test this on Mandolin later today
I decided it is not worth removing the duplicate gdtptr16 in entry16.inc, and I will update these commits soonish with some other small fixes.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: [WIP] arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
i'll try to test this on Mandolin later today
I decided it is not worth removing the duplicate gdtptr16 in entry16.inc, and I will update these commits soonish with some other small fixes.
Actually, gdt and gdt_end must appear in the same compilation unit with gdtptr16, should we want to keep it.
Hello build bot (Jenkins), Jason Glenesk, Patrick Georgi, Martin Roth, Marshall Dawson, Julius Werner, Arthur Heymans, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47964
to look at the new patch set (#5).
Change subject: arch/x86: Clean up bootblock assembly ......................................................................
arch/x86: Clean up bootblock assembly
We have identical gdtptr16 and gdtptr. The reference in gdtptr_offset calculation is not accounted for when considering --gc-sections, so to support linking gdt_init.S separately add dummy use of gdtptr symbol.
Realmode execution already accessed gdt that was located outside [_start16bit,_estart16bit] region. Remove latter symbol as the former was not really a start of region, but entry point symbol.
With the romcc bootblock solution, entry32.inc may have been linked into romstage before, but the !ENV_BOOTBLOCK case seems obsolete now.
Change-Id: I0a3f6aeb217ca4e38b936b8c9ec8b0b69732cbb9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock.ld M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/32bit/entry32.inc M src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld 4 files changed, 6 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/47964/5
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 5: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: arch/x86: Clean up bootblock assembly ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47964/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47964/1//COMMIT_MSG@9 PS1, Line 9: hae
have
Done
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47964 )
Change subject: arch/x86: Clean up bootblock assembly ......................................................................
arch/x86: Clean up bootblock assembly
We have identical gdtptr16 and gdtptr. The reference in gdtptr_offset calculation is not accounted for when considering --gc-sections, so to support linking gdt_init.S separately add dummy use of gdtptr symbol.
Realmode execution already accessed gdt that was located outside [_start16bit,_estart16bit] region. Remove latter symbol as the former was not really a start of region, but entry point symbol.
With the romcc bootblock solution, entry32.inc may have been linked into romstage before, but the !ENV_BOOTBLOCK case seems obsolete now.
Change-Id: I0a3f6aeb217ca4e38b936b8c9ec8b0b69732cbb9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47964 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/bootblock.ld M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/32bit/entry32.inc M src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld 4 files changed, 6 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/arch/x86/bootblock.ld b/src/arch/x86/bootblock.ld index 12f932c..849addd 100644 --- a/src/arch/x86/bootblock.ld +++ b/src/arch/x86/bootblock.ld @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-gdtptr16_offset = gdtptr16 & 0xffff; +gdtptr_offset = gdtptr & 0xffff; nullidt_offset = nullidt & 0xffff;
/* Symbol _start16bit must be aligned to 4kB to start AP CPUs with diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 13d12be..2665cc6 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -108,7 +108,7 @@ movw $nullidt_offset, %bx subw %ax, %bx lidt %cs:(%bx) - movw $gdtptr16_offset, %bx + movw $gdtptr_offset, %bx subw %ax, %bx lgdtl %cs:(%bx)
@@ -127,11 +127,8 @@ * The gdt is defined in entry32.inc, it has a 4 Gb code segment * at 0x08, and a 4 GB data segment at 0x10; */ -.align 4 -.globl gdtptr16 -gdtptr16: - .word gdt_end - gdt -1 /* compute the table limit */ - .long gdt /* we know the offset */ +__gdtptr: + .long gdtptr
.align 4 .globl nullidt @@ -139,7 +136,3 @@ .word 0 /* limit */ .long 0 .word 0 - -.globl _estart16bit -_estart16bit: - .code32 diff --git a/src/cpu/x86/32bit/entry32.inc b/src/cpu/x86/32bit/entry32.inc index be67b53..8509448 100644 --- a/src/cpu/x86/32bit/entry32.inc +++ b/src/cpu/x86/32bit/entry32.inc @@ -8,29 +8,11 @@
.code32 /* - * When we come here we are in protected mode. We expand - * the stack and copies the data segment from ROM to the - * memory. - * - * After that, we call the chipset bootstrap routine that - * does what is left of the chipset initialization. - * + * When we come here we are in protected mode. * NOTE aligned to 4 so that we are sure that the prefetch * cache will be reloaded. - * - * In the bootblock there is already a ljmp to __protected_start and - * the reset vector jumps to symbol _start16bit in entry16.inc from - * the reset vectors's symbol which is _start. Therefore, don't - * expose the _start symbol for bootblock. */ .align 4 -#if !ENV_BOOTBLOCK -.globl _start -_start: -#endif - - lgdt %cs:gdtptr - ljmp $ROM_CODE_SEG, $__protected_start
__protected_start: /* Save the BIST value */ diff --git a/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld b/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld index f702b4b..352472e 100644 --- a/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld +++ b/src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld @@ -102,7 +102,7 @@
#if ENV_BOOTBLOCK
-gdtptr16_offset = gdtptr16 & 0xffff; +gdtptr_offset = gdtptr & 0xffff; nullidt_offset = nullidt & 0xffff;
SECTIONS {
-
Arthur Heymans (Code Review) -
Felix Held (Code Review)
-
Hung-Te Lin (Code Review)
-
Kyösti Mälkki (Code Review)