Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 12:
(11 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@329 PS10, Line 329: p2sb_dev
Agreed as p2sb_dev was intended to be referred while trying to read ibdf and hbdf. […]
Ack
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; :
P2sbconfigure/P2sbLock/P2sbSaiLock have been executed in silicon initialization. […]
Well, I have to admit silicon initialization ends in coreboot before filling the tables. But the whole thing was designed before Intel started to hide registers. I think things like POSTBOOT_SAI should only be set right before we hand over execution to the OS, it definitely shouldn't be treated as part of the silicon init. If it's done too early, it makes the whole firmware more fragile, and harder to write anyway, slowing the whole process down. Like in this case, we have to manually check that the code parts are in sync. Seems like a pretty bad design choice.
If you agree to my assessment, please file a bug for FSP that it shouldn't hide registers before the `ready to boot` notifi- cation phase.
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 the register value just to disassemble it again, doesn't make the code very readable.
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 what the code does, just remove that part of the comments.
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 soc/intel/cannonlake/ is used, so you have to check on the more specific configs SOC_INTEL_WHISKEYLAKE and SOC_INTEL_ COFFEELAKE (I'm not sure about Comet Lake?).
Also, IS_ENABLED() is deprecated and there is a typo: CONF*I*G_. Anyway, please use the new CONFIG() macro, e.g.:
#if CONFIG(SOC_INTEL_COFFEELAKE) || CONFIG(SOC_INTEL_WHISKEYLAKE)
Note that this negates the condition.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 43: 0xfffffff000ull
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" },
It is updated to differentiate platforms.
Done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
From both of Cannon Lake PCH bios specification and fsp source code. […]
Ack
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.
We should also set `VtdBaseAddress[3]`, right?
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.
Why? Sorry to bother again and again, I just don't see the relation. VMX usually refers to VT-x. Maybe we just talk about different things here. So let's answer this first: * Does the VmxEnable UPD refer to VT-x?
We have Vtd test application running at uefi shell which validate all relevant VT-d configuration along with vmx enable/lock features.
Would that validation fail if VT-d is disabled, no DMAR tables are present but VMX is enabled?
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 documents before anybody @intel did). It says there:
The IPU configuration register should be programmed first
But you say:
Coreboot configures IPUVTDBAR (offset 0x7884) first.
Do you see the contradiction?