Arthur Heymans 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 5:
(12 comments)
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/mainboard/intel/glkrvp/... PS5, Line 89: config SOC_INTEL_COMMON_BLOCK_SGX_ENABLE : bool : default y Do you want to move the devicetree setting (supposedly mb specific) to a SoC default?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... File src/soc/intel/apollolake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/apollolake/me... PS5, Line 35: CONFIG( not a Kconfig
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 332: valid uninitialized?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 340: valid/MiB) Will always be 0.
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/sgx.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 45: soc_fill_sgx_param The apollolake definition still exists.
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/Kconfig:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 31: SOC_INTEL_COMMON_BLOCK_SGX_PRMRR_SIZE_256 Always be specific about units -> 256*MB* ;)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 39: static const struct sgx_param *get_sgx_param(void) : { : if (sgx_param_valid) : return &g_sgx_param; : : memset(&g_sgx_param, 0, sizeof(g_sgx_param)); : if (soc_fill_sgx_param(&g_sgx_param) < 0) { : printk(BIOS_ERR, "SGX : Failed to get soc sgx param\n"); : return NULL; : } : sgx_param_valid = true; : printk(BIOS_INFO, "SGX : param.enable = %d\n", g_sgx_param.enable); : : return &g_sgx_param; : } : : static int soc_sgx_enabled(void) : { : const struct sgx_param *sgx_param = get_sgx_param(); : return sgx_param ? sgx_param->enable : 0; : } It might be easier to get rid of this crap in a separate patch. I'd +2 that easily ;)
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 182: !CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) Move this up and just return. It's noise in the log to tell the user SGX conditions are not set if it's disabled in Kconfig
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/common/block/... PS5, Line 213: CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_ENABLE) same
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... File src/soc/intel/icelake/chip.h:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/icelake/chip.... PS5, Line 278: get_prmrr_size Move the function declaration to a common place?
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/cpu.c... PS5, Line 498: sgx_configure You are checking for the Kconfig option in the call too
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35799/5/src/soc/intel/skylake/memma... PS5, Line 89: no empty line