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 10:
(12 comments)
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG@8 PS3, Line 8: : SoC is detected Vt-d capable. ACPI DMAR table is : created and DMA-remapping hardware units entries : are programmed. Device scope like IOAPIC and HPET : will be added once cnl fsp upd update is available.
done
Paul, why? also, the usual width for Git commit messages is 72 chars.
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@298 PS10, Line 298: and in 64-bit space It's a 64-bit register and the mask is even narrower, so it's always in 64-bit space.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space. see above
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@329 PS10, Line 329: p2sb_dev This doesn't seem to be used?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space see above
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; : Why not just read the registers instead? That would save us from any doubts if the constants are in sync with them.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@362 PS10, Line 362: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED) Why this check? are there dependencies between the vtd units?
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 The mask seems one bit too long. I guess it should be
0x7ffffff000ull
i.e. bits 38:12 set.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, I guess it won't hurt but this resource doesn't seem to exist on CFL.
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 Where do these numbers come from? When are the respective registers set?
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; : Why have a `VtdDisable` option at all? Nobody seems to use it and what could be the reason?
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)); The BWG mentions another register to be programmed first in the IPU's PCI configuration space. Is this already handled?