Hello,
Quick question for Kconfig lint checking!
For my patch https://review.coreboot.org/c/coreboot/+/71120, the Jenkins build fails with following error:
"Check Kconfig files for errors (lint-stable-008-kconfig): #!!!!! Error: 'CONFIG_SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_2MB' selected at src/soc/intel/meteorlake/Kconfig:101. Symbols created in a choice cannot be selected"
What is the rationale behind enforcing this? As of now, PRMRR Kconfig settings are in common code and i wanted to set the right size from SOC specific code. e.g. the patch i mentioned earlier.
Should we get rid of enforcing "lint-stable-008-kconfig" test? If not what is the best way to achieve the scenario i mentioned above.
One option i think is to simplify PRMRR Kconfig options (get rid of choice menu) and define PRMRR_SIZE config option in SOC specific code. But then all SOC specific code would have to define this Kconfig option. If some SOC does not need PRMRR then also the Kconfig option with default set 0 might be needed.
Thanks for suggestions in advance.
-- Regards, Pratik
Hi Pratik,
On 10.03.23 02:19, Prajapati, Pratikkumar V wrote:
For my patch https://review.coreboot.org/c/coreboot/+/71120, the Jenkins build fails with following error:
"Check Kconfig files for errors (lint-stable-008-kconfig): #!!!!! Error: 'CONFIG_SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_2MB' selected at src/soc/intel/meteorlake/Kconfig:101. Symbols created in a choice cannot be selected"
What is the rationale behind enforcing this? As of now, PRMRR Kconfig settings are in common code and i wanted to set the right size from SOC specific code. e.g. the patch i mentioned earlier.
Quite simple: It doesn't work. Even if the linter didn't complain, a `select` on a choice config wouldn't do anything.
That's actually a good thing, IMO, because it helps us to keep Kconfig prompts sane. A choice always has a prompt, e.g. is visible to the integrator in menuconfig. If you could enforce a single option of a choice, it would show a prompt without meaning.
One option i think is to simplify PRMRR Kconfig options (get rid of choice menu) and define PRMRR_SIZE config option in SOC specific code.
That's the big question. Either it's wrong to have a choice and thereby a prompt or it's wrong to enforce a specific selection. I can't tell what it is in your case. Does it really have to be 2MiB? Why would a bigger PRMRR not work for Keylocker?
The `select INTEL_KEYLOCKER` also looks odd as that config option also has a prompt. Why would the SoC code make that choice if it was already decided that it should have a prompt? Or was that prompt added by accident?
But then all SOC specific code would have to define this Kconfig option.
Not really, we can always set sane defaults at the common/ level.
Nico
Thanks Nico.
PRMRR is used by Intel SGX and Key Locker both. So I wanted to give choice menu option in intel/common/block/cpu/Kconfig, and let integrator choose how much PMRRR s/he wants. Intel Meteor Lake-P (MTL) SOC does not support SGX but it does support Key Locker. 2MB is needed for Key Locker in MTL.
One option I see is to set PRMRR default to 2MB in choice prompt of "PRMRR size" if SOC is MTL. But IMO, putting something SOC specific in common code would be kind of a hack. So ideally I would like give choice menu in common code and select a value in SOC specific code. e.g. choice prompt "PRMRR size" depends on INTEL_KEYLOCKER || SOC_INTEL_COMMON_BLOCK_SGX default SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_MAX if SOC_INTEL_COMMON_BLOCK_SGX_ENABLE default SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_2MB if SOC_INTEL_METEORLAKE default SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_0MB if !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE && !INTEL_KEYLOCKER
Other option I see is to redefine config SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE in src/soc/intel/meteorlake/Kconfig with default value set. But since this is also defined in common Kconfig, I am not sure if this would be considered "clean". Also SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_*MB (bool config options) also liger in generated config file, which my create much confusion.
Any recommendation to achieve this in a clean way?
-- Regards, Pratik
-----Original Message----- From: Nico Huber nico.h@gmx.de Sent: Friday, March 10, 2023 7:21 AM To: Prajapati, Pratikkumar V pratikkumar.v.prajapati@intel.com; coreboot@coreboot.org Subject: Re: [coreboot] Quick question for Kconfig lint checking
Hi Pratik,
On 10.03.23 02:19, Prajapati, Pratikkumar V wrote:
For my patch https://review.coreboot.org/c/coreboot/+/71120, the Jenkins build fails with following error:
"Check Kconfig files for errors (lint-stable-008-kconfig): #!!!!! Error: 'CONFIG_SOC_INTEL_COMMON_BLOCK_PRMRR_SIZE_2MB' selected at src/soc/intel/meteorlake/Kconfig:101. Symbols created in a choice cannot be selected"
What is the rationale behind enforcing this? As of now, PRMRR Kconfig settings are in common code and i wanted to set the right size from SOC specific code. e.g. the patch i mentioned earlier.
Quite simple: It doesn't work. Even if the linter didn't complain, a `select` on a choice config wouldn't do anything.
That's actually a good thing, IMO, because it helps us to keep Kconfig prompts sane. A choice always has a prompt, e.g. is visible to the integrator in menuconfig. If you could enforce a single option of a choice, it would show a prompt without meaning.
One option i think is to simplify PRMRR Kconfig options (get rid of choice menu) and define PRMRR_SIZE config option in SOC specific code.
That's the big question. Either it's wrong to have a choice and thereby a prompt or it's wrong to enforce a specific selection. I can't tell what it is in your case. Does it really have to be 2MiB? Why would a bigger PRMRR not work for Keylocker?
The `select INTEL_KEYLOCKER` also looks odd as that config option also has a prompt. Why would the SoC code make that choice if it was already decided that it should have a prompt? Or was that prompt added by accident?
But then all SOC specific code would have to define this Kconfig option.
Not really, we can always set sane defaults at the common/ level.
Nico
Prajapati, Pratikkumar V wrote:
Any recommendation to achieve this in a clean way?
In case you don't find a good way then please at least ensure a build error with a meaningful error message for invalid configurations.
Thanks
//Peter
-
Nico Huber -
Peter Stuge -
Prajapati, Pratikkumar V