Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/#/c/32432/3/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/3/src/soc/intel/cannonlake/acpi.c@364 PS3, Line 364: any platform initiated DMA : * is restricted
It would be referred to IOMMU. […]
IIUC, it is completely upto the system software i.e. OS to enable this restriction when setting up the DMA remapping structures.
BTW, John - I am curious to understand the motivation behind setting of this bit. I don't see anything in Linux kernel actually using this. How are you testing this?
https://review.coreboot.org/#/c/32432/4/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/4/src/soc/intel/cannonlake/acpi.c@308 PS4, Line 308: /* Add RMRR entry */ This should have been caught in the previous CL. As per ACPI Spec for DMAR, it states that "BIOS implementations must report these remapping structure types in numerical order. i.e., All remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth."
As per this, adding of RMRR entry should be done after all DRHD structures have been added in this function.
https://review.coreboot.org/#/c/32432/4/src/soc/intel/cannonlake/acpi.c@368 PS4, Line 368: (1 << 2) Can you please fix DMA_CTRL_PLATFORM_OPT_IN_FLAG in acpi.h to be (1 << 2) and use that here?
https://review.coreboot.org/#/c/32432/4/src/soc/intel/cannonlake/romstage/fs... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/32432/4/src/soc/intel/cannonlake/romstage/fs... PS4, Line 119: #if CONFIG(SOC_INTEL_COMMON_VTD) Can this not be a runtime check?
if (CONFIG(SOC_INTEL_COMMON_VTD)) tconfig->VtdDisable = 0;
In fact, why is this check required at all? SOC_INTEL_COMMON_VTD is set for cannonlake in Kconfig. So, why not set VtdDisable to 0 always?