Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 4:
(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
It would be referred to IOMMU. I am not aware that firmware drivers implementation comply with RMRRs.
But they should. According to the description this is about the point "when transferring control to system software". This is exactly what RMRRs are there for, to report DMA ranges that are still used by firmware drivers beyond this point.
However, it doesn't matter what firmware drivers do before this point.
IIUC, it is completely upto the system software i.e. OS to enable this restriction when setting up the DMA remapping structures.
This is my understanding now, too. And I fear the worst: The spec says that RMRR "may" be used to report DMA ranges that are still used by firmware drivers. But it didn't enforce it, so this bit was added to tell the OS that the firmware actually does it. If we'd set this bit unconditionally now (without knowing with what payload drivers the coreboot build is going to be combined with) we would defeat its purpose. After the first few firmwares that report it wrong, OS deve- lopers would likely cease to trust the bit.
IMHO, this bit should only be set in coreboot if the payload is already an OS kernel.
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?
There is this patch for Linux: https://patchwork.kernel.org/patch/10678945/
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. […]
Nice catch, I'll try to fix this for earlier platforms.