John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 13:
(7 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
Well, I have to admit silicon initialization ends in coreboot […]
Literally it was referred to fsp_silicon_init. P2sb access blocked was a known issue and a HSD had been filed a while ago.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@345 PS10, Line 345: ibdf >> 8
Please get rid of the `ibdf` and `hbdf` variables. Forging […]
ibdf and hbdf usage was intended to mirror fsp settings for both of them in the scenario they can be directly retrieved from fsp. Updated.
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c@298 PS12, Line 298: and in 32-bit space
This makes no sense either. If you don't have the time to read […]
done
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... PS12, Line 53: #if IS_ENABLED(CONFG_SOC_INTEL_CANNONLAKE)
SOC_INTEL_CANNONLAKE is always selected when the code in […]
done
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... PS12, Line 119: tconfig->VtdDisable = config->VtdDisable;
This is part of FSP-S configuration for CFL/WHL. […]
VtdDisable and VtdBaseAddress[3] had been deprecated from FSP-S. They are in FSP-M. There had been intention to address both of VtDisable and VtdBaseAddress in fsp_memory_init. But it causes build failure because of corresponding upd change has not been landed in https://github.com/intelfsp/fsp. Issue had been raised a while ago. fsp refers default base address values which match coreboot setting. VtdBaseAddress and VtdDisable can be optionally configured when github intelfsp upd has been updated.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
It appears properly to associate vmx with VT-d setting. […]
VmxEnable(VT-x) and VtdDisable (VT-d) are in FSP-M. The Vtd test validation fails if VT-d is enabled, but vmx is not enabled/locked.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
Thanks for the quote (I'm bound by NDA, so I can't quote these […]
If SA device IPU is not fused off, coreboot updates SaIpuEnable flag and fsp configures IPU. Besides the configurable SaIpuConfiguration, I did not realize other settings needed by coreboot. Please share your finding.