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 13:
(9 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.
Because that’s in the documentation [1]. […]
Well, it says 75 is the maximum, not that it's the minimum... Narrower columns are generally easier to read for the eyes, so if somebody already formatted things nicely, please don't make them change it.
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
done.
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space.
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@345 PS10, Line 345: ibdf >> 8
ibdf and hbdf usage was intended to mirror fsp settings for both of them in the scenario they can be […]
Ack
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
done
Ack
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)
done
Ack
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; :
VmxEnable(VT-x) and VtdDisable (VT-d) are in FSP-M. […]
Well, IIRC, the current logic ensures the opposite: disabling VT-x when VT-d is disabled. What we'd need to ensure that this test always passes is to disable VT-d if VT-x is disabled.
But, I'm questioning the test anyway. Where does that requirement come from?
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));
If SA device IPU is not fused off, coreboot updates SaIpuEnable flag and fsp configures IPU. […]
I fear, we are talking past each other. I'm merely concerned about the order of the writes to B0.D5.F0+0xf0 and MCHBAR+0x7884.
The BWG says write B0.D5.F0+0xf0 first, but we are writing to MCHBAR+0x7884 first and call FSP then to write the other (and MCHBAR+0x7884 again).
Also, now that I typed this a few times. Is 0x7884 the correct offset? shouldn't it be 8-byte aligned?