Change in coreboot[master]: soc/intel/skylake: Add option to control microcode inclusion
Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode inclusion ......................................................................
soc/intel/skylake: Add option to control microcode inclusion
On embedded boards the cpu mounted on the board is known. So it is not required to include microcode for all possible SkyLake and KabyLake cpus. This patch provides the possibility to only support the versions required.
By the default all microcode will be included and the versions not required can be removed using Kconfig.
BUG=N/A TEST=build
Change-Id: Iaa36c2846b2279a2eb2b61e6c97d6c89d0736f55 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37514/1
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 528fd4a..c95d209 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -288,4 +288,28 @@ hex default 0x40000 # 256KB
+config HAVE_SKYLAKE_CPU + bool "Board can contain SkyLake CPU" + default y + +if SKYLAKE_SOC_PCH_H + +config HAVE_KABYLAKE_CPU + bool "Board can contain KabyLake CPU" + default y if SOC_INTEL_KABYLAKE + +endif + +if !SKYLAKE_SOC_PCH_H + +config HAVE_KABYLAKE_DUAL + bool "Board can contain KabyLake DUAL core" + default y + +config HAVE_KABYLAKE_QUAD + bool "Board can contain KabyLake QUAD core" + default y + +endif + endif diff --git a/src/soc/intel/skylake/Makefile.inc b/src/soc/intel/skylake/Makefile.inc index c093738..1c351b8 100644 --- a/src/soc/intel/skylake/Makefile.inc +++ b/src/soc/intel/skylake/Makefile.inc @@ -79,18 +79,28 @@ postcar-y += uart.c
ifeq ($(CONFIG_SKYLAKE_SOC_PCH_H),y) +ifeq ($(CONFIG_HAVE_SKYLAKE_CPU),y) # Skylake H Q0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-5e-03 +endif +ifeq ($(CONFIG_HAVE_KABYLAKE_CPU),y) # Kabylake H B0 S0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-9e-09 +endif else +ifeq ($(CONFIG_HAVE_SKYLAKE_CPU),y) # Skylake D0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-4e-03 +endif +ifeq ($(CONFIG_HAVE_KABYLAKE_DUAL),y) # Kabylake H0, J0, J1 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-8e-09 +endif +ifeq ($(CONFIG_HAVE_KABYLAKE_QUAD),y) # Kabylake Y0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-8e-0a endif +endif # Missing for Skylake C0 (0x406e2), Kabylake G0 (0x406e8), Kabylake HA0 (0x506e8) # since those are probably pre-release samples.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode inclusion ......................................................................
Patch Set 1:
(11 comments)
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@7 PS1, Line 7: microcode … microcode update inclusion
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: KabyLake Kaby Lake
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: SkyLake Sky Lake
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: required to include microcode for all possible SkyLake and KabyLake By default
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@14 PS1, Line 14: By the default By default
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@14 PS1, Line 14: microcode microcode updates
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 291: HAVE_SKYLAKE_CPU Maybe we can find a better name?
1. `SUPPORTS_SKYLAKE_CPU` 2. `MAINBOARD_SUPPORTS_SKYLAKE_CPU` 3. `MAINBOARD_HAS_SKYLAKE_CPU_SUPPORT`
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 292: SkyLake Sky Lake
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 298: KabyLake Kaby Lake
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 306: KabyLake Ditto.
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 310: bool "Board can contain KabyLake QUAD core" Ditto.
Hello Patrick Rudolph, Frans Hendriks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37514
to look at the new patch set (#2).
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
soc/intel/skylake: Add option to control microcode update inclusion
On embedded boards the cpu mounted on the board is known. So it is not required to include microcode for all possible Sky Lake and Kaby Lake cpus. This patch provides the possibility to only support the versions required.
By default all microcode updates will be included and the versions not required can be removed using Kconfig.
BUG=N/A TEST=build
Change-Id: Iaa36c2846b2279a2eb2b61e6c97d6c89d0736f55 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37514/2
Hello Patrick Rudolph, Frans Hendriks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37514
to look at the new patch set (#3).
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
soc/intel/skylake: Add option to control microcode update inclusion
On embedded boards the cpu mounted on the board is known. So it is not required to include microcode for all possible Sky Lake and Kaby Lake cpus. This patch provides the possibility to only support the versions required.
By default all microcode updates will be included and the versions not required can be removed using Kconfig.
BUG=N/A TEST=build
Change-Id: Iaa36c2846b2279a2eb2b61e6c97d6c89d0736f55 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37514/3
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@7 PS1, Line 7: microcode
… microcode update inclusion
Done
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: required to include microcode for all possible SkyLake and KabyLake
By default
Done
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: KabyLake
Kaby Lake
Done
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@10 PS1, Line 10: SkyLake
Sky Lake
Done
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@14 PS1, Line 14: By the default
By default
Done
https://review.coreboot.org/c/coreboot/+/37514/1//COMMIT_MSG@14 PS1, Line 14: microcode
microcode updates
Done
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 291: HAVE_SKYLAKE_CPU
Maybe we can find a better name? […]
Done
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 292: SkyLake
Sky Lake
Done
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 298: KabyLake
Kaby Lake
Done
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 306: KabyLake
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 310: bool "Board can contain KabyLake QUAD core"
Ditto.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
The current approach implies that users can't change that behavior without editing Kconfig files. I'd rather have an option in menuconfig to only include microcode updates for a specific CPUID, that can be used with any mainboard. Those who squeeze Linux kernels into flash chips next to coreboot like extra space, so even desktop boards with socketed CPUs could use this idea.
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 292: SkyLake
Done
It's actually "Skylake"
https://ark.intel.com/content/www/us/en/ark/products/codename/37572/skylake....
Hello Patrick Rudolph, Angel Pons, Frans Hendriks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37514
to look at the new patch set (#4).
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
soc/intel/skylake: Add option to control microcode update inclusion
On embedded boards the cpu mounted on the board is known. So it is not required to include microcode for all possible Sky Lake and Kaby Lake cpus. This patch provides the possibility to only support the versions required.
By default all microcode updates will be included and the versions not required can be removed using Kconfig.
BUG=N/A TEST=build
Change-Id: Iaa36c2846b2279a2eb2b61e6c97d6c89d0736f55 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37514/4
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 292: SkyLake
It's actually "Skylake" […]
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4:
Patch Set 3: Code-Review-1
(1 comment)
The current approach implies that users can't change that behavior without editing Kconfig files. I'd rather have an option in menuconfig to only include microcode updates for a specific CPUID, that can be used with any mainboard. Those who squeeze Linux kernels into flash chips next to coreboot like extra space, so even desktop boards with socketed CPUs could use this idea.
There is no need to edit Kconfig to change the behavior. The items show up in menuconfig. If the board contains a PCH_H the only options that show are Skylake and Kabylake as those are the only two possibilities.
In the other case (with the LP) there are 3 possibilities: * Skylake * Kabylake with 2 or 4 cores.
I didn't use the CPUID but the descriptions ("Skylake", "Kaby Lake DUAL" and "Kaby Lake QUAD") as this is obvious immediately and doesn't require to dig into the datasheet to find the CPUID.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/1/src/soc/intel/skylake/Kconf... PS1, Line 292: SkyLake
Done
Sorry about that. You are right, and I forgot.
Skylake and Kaby Lake. Very confusing.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4: Code-Review+2
Patch Set 4:
Patch Set 3: Code-Review-1
(1 comment)
The current approach implies that users can't change that behavior without editing Kconfig files. I'd rather have an option in menuconfig to only include microcode updates for a specific CPUID, that can be used with any mainboard. Those who squeeze Linux kernels into flash chips next to coreboot like extra space, so even desktop boards with socketed CPUs could use this idea.
There is no need to edit Kconfig to change the behavior. The items show up in menuconfig. If the board contains a PCH_H the only options that show are Skylake and Kabylake as those are the only two possibilities.
In the other case (with the LP) there are 3 possibilities:
- Skylake
- Kabylake with 2 or 4 cores.
I didn't use the CPUID but the descriptions ("Skylake", "Kaby Lake DUAL" and "Kaby Lake QUAD") as this is obvious immediately and doesn't require to dig into the datasheet to find the CPUID.
Oh, missed that the Kconfig options you added have a prompt. My bad.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37514/4/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/4/src/soc/intel/skylake/Kconf... PS4, Line 301: endif : : if !SKYLAKE_SOC_PCH_H I was wondering why this isn't an "else". It seems like there's no such thing in Kconfig
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37514/4/src/soc/intel/skylake/Kconf... File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37514/4/src/soc/intel/skylake/Kconf... PS4, Line 301: endif : : if !SKYLAKE_SOC_PCH_H
I was wondering why this isn't an "else". […]
As far as I know you are right. We initially tried to use the else here but it didn't work out. If there is a better solution I would like to know.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 4: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
soc/intel/skylake: Add option to control microcode update inclusion
On embedded boards the cpu mounted on the board is known. So it is not required to include microcode for all possible Sky Lake and Kaby Lake cpus. This patch provides the possibility to only support the versions required.
By default all microcode updates will be included and the versions not required can be removed using Kconfig.
BUG=N/A TEST=build
Change-Id: Iaa36c2846b2279a2eb2b61e6c97d6c89d0736f55 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37514 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc 2 files changed, 34 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Patrick Rudolph: Looks good to me, approved Frans Hendriks: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 528fd4a..31f809a 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -288,4 +288,28 @@ hex default 0x40000 # 256KB
+config MAINBOARD_SUPPORTS_SKYLAKE_CPU + bool "Board can contain Skylake CPU" + default y + +if SKYLAKE_SOC_PCH_H + +config MAINBOARD_SUPPORTS_KABYLAKE_CPU + bool "Board can contain Kaby Lake CPU" + default y if SOC_INTEL_KABYLAKE + +endif + +if !SKYLAKE_SOC_PCH_H + +config MAINBOARD_SUPPORTS_KABYLAKE_DUAL + bool "Board can contain Kaby Lake DUAL core" + default y + +config MAINBOARD_SUPPORTS_KABYLAKE_QUAD + bool "Board can contain Kaby Lake QUAD core" + default y + +endif + endif diff --git a/src/soc/intel/skylake/Makefile.inc b/src/soc/intel/skylake/Makefile.inc index c093738..75121ab 100644 --- a/src/soc/intel/skylake/Makefile.inc +++ b/src/soc/intel/skylake/Makefile.inc @@ -79,18 +79,28 @@ postcar-y += uart.c
ifeq ($(CONFIG_SKYLAKE_SOC_PCH_H),y) +ifeq ($(CONFIG_MAINBOARD_SUPPORTS_SKYLAKE_CPU),y) # Skylake H Q0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-5e-03 +endif +ifeq ($(CONFIG_MAINBOARD_SUPPORTS_KABYLAKE_CPU),y) # Kabylake H B0 S0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-9e-09 +endif else +ifeq ($(CONFIG_MAINBOARD_SUPPORTS_SKYLAKE_CPU),y) # Skylake D0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-4e-03 +endif +ifeq ($(CONFIG_MAINBOARD_SUPPORTS_KABYLAKE_DUAL),y) # Kabylake H0, J0, J1 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-8e-09 +endif +ifeq ($(CONFIG_MAINBOARD_SUPPORTS_KABYLAKE_QUAD),y) # Kabylake Y0 cpu_microcode_bins += 3rdparty/intel-microcode/intel-ucode/06-8e-0a endif +endif # Missing for Skylake C0 (0x406e2), Kabylake G0 (0x406e8), Kabylake HA0 (0x506e8) # since those are probably pre-release samples.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 5:
Sorry, I'm late. I'm not very happy about the prompts. "Board can ..." doesn't sound like a user decision. Also, some prompts are even for soldered down CPUs. This is nothing a user should be prompted for.
Also, we already had two means to override the default decision: * `config CPU_MICROCODE_CBFS_EXTERNAL_BINS`, and * mainboard makefiles can overwrite `cpu_microcode_bins`.
Now, if a board maintainer would choose to do the latter, it would override the prompted Kconfig settings :-/
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 5:
Patch Set 5:
Sorry, I'm late. I'm not very happy about the prompts. "Board can ..." doesn't sound like a user decision. Also, some prompts are even for soldered down CPUs. This is nothing a user should be prompted for.
Also, we already had two means to override the default decision:
- `config CPU_MICROCODE_CBFS_EXTERNAL_BINS`, and
- mainboard makefiles can overwrite `cpu_microcode_bins`.
Now, if a board maintainer would choose to do the latter, it would override the prompted Kconfig settings :-/
Basically this is an attempt to make this process more user-friendly and remove the requirement to edit Kconfig (or other) files. In fact, modifying these settings is a user decision. It is only required to do this when you run into trouble because of a large payload e.g. I can understand your position as well but this hugely depends on the definition of what a user is. If you look at the feedback from Angel Pons you can see the request here was to have this as a menu item. So this is already in a conflict with your view on how this should work.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Sorry, I'm late. I'm not very happy about the prompts. "Board can ..." doesn't sound like a user decision. Also, some prompts are even for soldered down CPUs. This is nothing a user should be prompted for.
Also, we already had two means to override the default decision:
- `config CPU_MICROCODE_CBFS_EXTERNAL_BINS`, and
- mainboard makefiles can overwrite `cpu_microcode_bins`.
Now, if a board maintainer would choose to do the latter, it would override the prompted Kconfig settings :-/
Basically this is an attempt to make this process more user-friendly and remove the requirement to edit Kconfig (or other) files. In fact, modifying these settings is a user decision. It is only required to do this when you run into trouble because of a large payload e.g. I can understand your position as well but this hugely depends on the definition of what a user is. If you look at the feedback from Angel Pons you can see the request here was to have this as a menu item. So this is already in a conflict with your view on how this should work.
There was no need to edit files, we already have config CPU_MICROCODE_CBFS_EXTERNAL_BINS. What this patch adds is a mapping from CPU names/descriptions to CPUID signatures that was missing before. It's a nice addition, but I don't see how the added Kconfig options make it more user friendly. They don't even have a help text, no sign that they are related to the added microcode updates. Also, they only affect config CPU_MICROCODE_CBFS_DEFAULT_BINS, but show up no matter if that is selected.
And for 90% of the boards the added Kconfig prompts are not necessary because we know what is soldered to the board. We try to avoid Kconfig prompts that can result in bricks, you just added a bunch.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
Sorry, I'm late. I'm not very happy about the prompts. "Board can ..." doesn't sound like a user decision. Also, some prompts are even for soldered down CPUs. This is nothing a user should be prompted for.
Also, we already had two means to override the default decision:
- `config CPU_MICROCODE_CBFS_EXTERNAL_BINS`, and
- mainboard makefiles can overwrite `cpu_microcode_bins`.
Now, if a board maintainer would choose to do the latter, it would override the prompted Kconfig settings :-/
Basically this is an attempt to make this process more user-friendly and remove the requirement to edit Kconfig (or other) files. In fact, modifying these settings is a user decision. It is only required to do this when you run into trouble because of a large payload e.g. I can understand your position as well but this hugely depends on the definition of what a user is. If you look at the feedback from Angel Pons you can see the request here was to have this as a menu item. So this is already in a conflict with your view on how this should work.
There was no need to edit files, we already have config CPU_MICROCODE_CBFS_EXTERNAL_BINS. What this patch adds is a mapping from CPU names/descriptions to CPUID signatures that was missing before. It's a nice addition, but I don't see how the added Kconfig options make it more user friendly. They don't even have a help text, no sign that they are related to the added microcode updates. Also, they only affect config CPU_MICROCODE_CBFS_DEFAULT_BINS, but show up no matter if that is selected.
And for 90% of the boards the added Kconfig prompts are not necessary because we know what is soldered to the board. We try to avoid Kconfig prompts that can result in bricks, you just added a bunch.
As you can imagine, it is not my intention to turn as many boards as possible into bricks. The intention is to make it easier for the user to select a subset of the microcode blobs available for this platform and easily remove the ones that don't apply.
You are absolutely right about the missing help and the missing dependency on CPU_MICROCODE_CBFS_DEFAULT_BINS this is absolutely confusing. The issue with CPU_MICROCODE_CBFS_EXTERNAL_BINS is that it is quite easy to pick the wrong one and brick the system that way.
Thinking about I was wondering if it would make more sense to reverse the logic. So if you have the CPU_MICROCODE_CBFS_DEFAULT_BINS you will be provided the option to remove SKYLAKE microcode e.g. This would make it easier to control from the mainboard level using a select and I think it would be more obvious to a user to understand what happens.
Please let me know what you think.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37514 )
Change subject: soc/intel/skylake: Add option to control microcode update inclusion ......................................................................
Patch Set 5:
Thinking about I was wondering if it would make more sense to reverse the logic. So if you have the CPU_MICROCODE_CBFS_DEFAULT_BINS you will be provided the option to remove SKYLAKE microcode e.g. This would make it easier to control from the mainboard level using a select and I think it would be more obvious to a user to understand what happens.
Please let me know what you think.
Sounds reasonable. But I wonder what would be better: a) try to add Kconfig options for all possible cases, or b) let the mainboard's Makefile override the paths.
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Wim Vervoorn (Code Review)