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 6:
(2 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
so this bit was added to tell the OS that the firmware actually does it.
I just read this part of the spec again and I agree with your assessment. This bit should be set only if firmware is already restricting access to DMA ranges in RMRR. Since we are not doing that in coreboot, does it really make sense to set the flag?
IMHO, this bit should only be set in coreboot if the payload is already an OS kernel.
Rather than payload already being an OS kernel, I think it should be set only if coreboot is actually restricting DMA access outside of RMRR ranges? From the Linux patch you linked: "Returns true if the platform has %DMA_CTRL_PLATFORM_OPT_IN_FLAG set in the ACPI DMAR table. This means that the platform boot firmware has made sure no device can issue DMA outside of RMRR regions."
https://review.coreboot.org/#/c/32432/6/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/6/src/soc/intel/cannonlake/acpi.c@340 PS6, Line 340: RMRR I was thinking more on the lines of: 1. Make acpi_create_dmar call into soc_fill_dmar with a param (id) which is basically the type of table that is being filled.
2. soc_fill_dmar can then check the id and make a call into soc_fill_drhd/soc_fill_rmrr/soc_fill_xxxx, etc.
That ensures we don't miss out these issues in future reviews. Thoughts?