Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35799 )
Change subject: soc/intel/sgx: convert SGX and PRMRR devicetree options to Kconfig ......................................................................
Patch Set 35:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/apollolake/m... PS29, Line 39: #ifdef SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE
Well, we could return 0 for limit=0... […]
But the way you are constraining with Kconfig 'no limit' is impossible. This sequence of #ifdef is pretty ugly to accommodate properly. I would hope we wouldn't have to touch all this code.
One could even take it so far as to not have a get_max_prmrr_size() that takes a parameter because you ensuring you are providing the size through a macro based on Kconfig. In such a scenario when would we ever need a get_max_prmrr_size() with a parameter when it could just be get_prmrr_size() ?
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 40: depends on SOC_INTEL_CANNONLAKE_BASE || SOC_INTEL_ICELAKE
what do you mean with "and in multiple places"?
Well, it looks like we keeping the dep list here and above on line 24. But regardless someone needs to append more conditions for every SoC, correct? Maybe I'm over thinking it, but it looks fairly brittle and another touch point for adding a new SoC.
https://review.coreboot.org/c/coreboot/+/35799/29/src/soc/intel/common/block... PS29, Line 53: depends on !SOC_INTEL_COMMON_BLOCK_SGX_ENABLE
Because SGX depends on PRMRR >= 32 MiB
Please add a comment to that effect.