Change in coreboot[master]: drivers/intel/fsp1_0: Fake microcode update to make FSP happy
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_0: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_0: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/cache_as_ram.S 1 file changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/36255/1
diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index 007b974..b7e21f7 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -235,17 +235,39 @@ * esp is set to this location so that the call into and return from the FSP * in find_fsp will work. */ + +/* Put something sensible in here */ +#define MCU_DATA_SIZE 0x10 +#define MCU_HEADER_SIZE 0x30 +#define MCU_TOTAL_SIZE (MCU_HEADER_SIZE + MCU_DATA_SIZE) + .align 4 fake_fsp_stack: .long find_fsp_ret .long CONFIG_FSP_LOC /* FSP base address */
CAR_init_params: - .long CONFIG_CPU_MICROCODE_CBFS_LOC /* Microcode Location */ - .long CONFIG_CPU_MICROCODE_CBFS_LEN /* Microcode Length */ + .long fake_microcode /* Microcode Location */ + .long MCU_TOTAL_SIZE /* Microcode Length */ .long 0xFFFFFFFF - CONFIG_ROM_SIZE + 1 /* Firmware Location */ .long CONFIG_ROM_SIZE /* Firmware Length */
CAR_init_stack: .long CAR_init_done .long CAR_init_params + +fake_microcode: +fake_microcode_header_start: + .long 1 /* Header Version */ + .long 1 /* Microcode revision */ + .long 0x10232019 /* Date: Time of writing 23-10-2019 */ + .long 0x00010ff0 /* Sig: (non existing) Family: 0xf, Model: 0x1f, stepping: 0 */ + .long 0 /* Checksum: not checked by FSP, so won't care */ + .long 1 /* Loader Revision */ + .long 1 /* Processor Flags */ + .long MCU_DATA_SIZE /* Data Size */ + .long MCU_TOTAL_SIZE /* Total Size */ + .space 12 /* Reserved */ +fake_microcode_header_end: + .space MCU_DATA_SIZE +fake_microcode_end:
Hello Patrick Rudolph, Huang Jin, Lee Leahy,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36255
to look at the new patch set (#2).
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_1: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/cache_as_ram.S 1 file changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/36255/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 2:
Tested how, and how much time is saved?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 2:
Patch Set 2:
Tested how, and how much time is saved?
I suspect very little changes in execution time. Typically microcode for 2-4 stepping is included for these SOC and only the header is check. From coreboot doing the updates + fake microcode for FSP to FSP only, I suspect coreboot being faster as WB-ROM caching is set up to speed up things.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 2:
FWIW, this is actually consistently 6.5ms slower on google/cyan than without the patch
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
FWIW, this is actually consistently 6.5ms slower on google/cyan than without the patch
Hmm maybe the fake microcode can be optimized to branch off earlier in the comparison process. I'm not so eager to dive back into that code :(
https://review.coreboot.org/c/coreboot/+/36255/2/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36255/2/src/drivers/intel/fsp1_1/ca... PS2, Line 259: fake_microcode: : fake_microcode_header_start: : .long 1 /* Header Version */ : .long 1 /* Microcode revision */ : .long 0x10232019 /* Date: Time of writing 23-10-2019 */ : .long 0x00010ff0 /* Sig: (non existing) Family: 0xf, Model: 0x1f, stepping: 0 */ : .long 0 /* Checksum: not checked by FSP, so won't care */ : .long 1 /* Loader Revision */ : .long 1 /* Processor Flags */ : .long MCU_DATA_SIZE /* Data Size */ : .long MCU_TOTAL_SIZE /* Total Size */ : .space 12 /* Reserved */ : fake_microcode_header_end: : .space MCU_DATA_SIZE : fake_microcode_end: This should probably be done in C code.
Hello Patrick Rudolph, Huang Jin, Lee Leahy, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36255
to look at the new patch set (#3).
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_1: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/cache_as_ram.S 1 file changed, 19 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/36255/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 3:
(1 comment)
Patch Set 2:
FWIW, this is actually consistently 6.5ms slower on google/cyan than without the patch
Is that an acceptable delay for not having to specify where the real microcode is in Kconfig?
https://review.coreboot.org/c/coreboot/+/36255/2/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36255/2/src/drivers/intel/fsp1_1/ca... PS2, Line 259: fake_microcode: : fake_microcode_header_start: : .long 1 /* Header Version */ : .long 1 /* Microcode revision */ : .long 0x10232019 /* Date: Time of writing 23-10-2019 */ : .long 0x00010ff0 /* Sig: (non existing) Family: 0xf, Model: 0x1f, stepping: 0 */ : .long 0 /* Checksum: not checked by FSP, so won't care */ : .long 1 /* Loader Revision */ : .long 1 /* Processor Flags */ : .long MCU_DATA_SIZE /* Data Size */ : .long MCU_TOTAL_SIZE /* Total Size */ : .space 12 /* Reserved */ : fake_microcode_header_end: : .space MCU_DATA_SIZE : fake_microcode_end:
This should probably be done in C code.
scratch that. It's hard to use sizeof to assembly.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 3:
FWIW, this is actually consistently 6.5ms slower on google/cyan than without the patch
Is that an acceptable delay for not having to specify where the real microcode is in Kconfig?
yes, I'm perfectly fine with that tradeoff
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36255/3/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36255/3/src/drivers/intel/fsp1_1/ca... PS3, Line 243: Please add a comment, why we fake the MCU.
Hello Patrick Rudolph, Huang Jin, Lee Leahy, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36255
to look at the new patch set (#4).
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_1: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
The advantage is that the Kconfig symbols to specify the location and the size of the microcode blob can be dropped.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Kconfig M src/drivers/intel/fsp1_1/cache_as_ram.S M src/mainboard/facebook/fbg1701/Kconfig M src/mainboard/portwell/m107/Kconfig 4 files changed, 21 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/36255/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36255/3/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36255/3/src/drivers/intel/fsp1_1/ca... PS3, Line 243:
Please add a comment, why we fake the MCU.
Done
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36255
to look at the new patch set (#5).
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_1: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
The advantage is that the Kconfig symbols to specify the location and the size of the microcode blob can be dropped.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Kconfig M src/drivers/intel/fsp1_1/cache_as_ram.S M src/mainboard/facebook/fbg1701/Kconfig M src/mainboard/portwell/m107/Kconfig 4 files changed, 21 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/36255/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36255 )
Change subject: drivers/intel/fsp1_1: Fake microcode update to make FSP happy ......................................................................
drivers/intel/fsp1_1: Fake microcode update to make FSP happy
The FSP loops through microcode updates and at the end checks if the microcode revision is not zero. Since we update the microcode before loading FSP, this is the case and a fake microcode can be passed to the FSP.
The advantage is that the Kconfig symbols to specify the location and the size of the microcode blob can be dropped.
Change-Id: I63cfb7b19e9795da85566733fb4c1ff989e85d03 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36255 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/Kconfig M src/drivers/intel/fsp1_1/cache_as_ram.S M src/mainboard/facebook/fbg1701/Kconfig M src/mainboard/portwell/m107/Kconfig 4 files changed, 21 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/Kconfig b/src/drivers/intel/fsp1_1/Kconfig index 1d22952..5f8f5b5 100644 --- a/src/drivers/intel/fsp1_1/Kconfig +++ b/src/drivers/intel/fsp1_1/Kconfig @@ -56,19 +56,6 @@ value that is set in the FSP binary. If the FSP needs to be moved, rebase the FSP with Intel's BCT (tool).
-config CPU_MICROCODE_CBFS_LEN - hex "Microcode update region length in bytes" - default 0x0 - help - The length in bytes of the microcode update region. - -config CPU_MICROCODE_CBFS_LOC - hex "Microcode update base address in CBFS" - default 0x0 - help - The location (base address) in CBFS that contains the microcode update - binary. - config DISPLAY_HOBS bool "Display hand-off-blocks (HOBs)" default n diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index f4638d9..fea7acb 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -239,11 +239,30 @@ .long CONFIG_FSP_LOC /* FSP base address */
CAR_init_params: - .long CONFIG_CPU_MICROCODE_CBFS_LOC /* Microcode Location */ - .long CONFIG_CPU_MICROCODE_CBFS_LEN /* Microcode Length */ + .long fake_microcode /* Microcode Location */ + .long fake_microcode_end - fake_microcode /* Microcode Length */ .long 0xFFFFFFFF - CONFIG_ROM_SIZE + 1 /* Firmware Location */ .long CONFIG_ROM_SIZE /* Firmware Length */
CAR_init_stack: .long CAR_init_done .long CAR_init_params + + /* coreboot updates microcode itself. FSP still needs a pointer + to something that looks like microcode, so provide it with fake + microcode. */ +fake_microcode: +fake_microcode_header_start: + .long 1 /* Header Version */ + .long 1 /* Microcode revision */ + .long 0x10232019 /* Date: Time of writing 23-10-2019 */ + .long 0x00010ff0 /* Sig: (non existing) Family: 0xf, Model: 0x1f, stepping: 0 */ + .long 0 /* Checksum: not checked by FSP, so won't care */ + .long 1 /* Loader Revision */ + .long 1 /* Processor Flags */ + .long fake_microcode_end - fake_microcode_header_end /* Data Size */ + .long fake_microcode_end - fake_microcode /* Total Size */ + .space 12 /* Reserved */ +fake_microcode_header_end: + .space 0x10 /* 16 bytes of empty data */ +fake_microcode_end: diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig index 3ade727..41d59ff 100644 --- a/src/mainboard/facebook/fbg1701/Kconfig +++ b/src/mainboard/facebook/fbg1701/Kconfig @@ -50,16 +50,6 @@ hex default 0x00600000
-config CPU_MICROCODE_CBFS_LEN - hex - default 0x10C00 - help - This should be updated when the microcode patch changes. - -config CPU_MICROCODE_CBFS_LOC - hex - default 0xFFFE9400 - config MRC_SETTINGS_CACHE_SIZE hex default 0x08000 diff --git a/src/mainboard/portwell/m107/Kconfig b/src/mainboard/portwell/m107/Kconfig index 3ab20f0..e5e3ff5 100644 --- a/src/mainboard/portwell/m107/Kconfig +++ b/src/mainboard/portwell/m107/Kconfig @@ -61,16 +61,6 @@ hex default 0x00800000
-config CPU_MICROCODE_CBFS_LEN - hex - default 0x10C00 - help - This should be updated when the microcode patch changes. - -config CPU_MICROCODE_CBFS_LOC - hex - default 0xFFFE9400 - config MRC_SETTINGS_CACHE_SIZE hex default 0x08000
-
Arthur Heymans (Code Review) -
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)