John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32432
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/1
diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c index 43d91d3..2ee7f7e 100644 --- a/src/soc/intel/cannonlake/acpi.c +++ b/src/soc/intel/cannonlake/acpi.c @@ -358,7 +358,13 @@ return current;
printk(BIOS_DEBUG, "ACPI: * DMAR\n"); - acpi_create_dmar(dmar, DMAR_INTR_REMAP, soc_fill_dmar); + /* Bit 2: DMA_CTRL_PLATFORM_OPT_IN_FLAG. Platform firmware is + * recommended to set this field to report any platform initiated DMA + * is restricted to only reserved memory regions (reported in RMRR + * structures) when transferring control to system software. + */ + acpi_create_dmar(dmar, DMAR_INTR_REMAP | (1 << 2), soc_fill_dmar); + current += dmar->header.length; current = acpi_align_current(current); acpi_add_table(rsdp, dmar); diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 2ad2c93..9fc90cf 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -103,6 +103,7 @@ assert(dev != NULL); const config_t *config = dev->chip_info; FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + FSP_M_TEST_CONFIG *tconfig = &mupd->FspmTestConfig;
soc_memory_init_params(m_cfg, config);
@@ -114,6 +115,9 @@ /* Set debug probe type */ m_cfg->PlatformDebugConsent = config->DebugConsent;
+ /* Configure VT-d */ + tconfig->VtdDisable = config->VtdDisable; + mainboard_memory_init_params(mupd); }
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#2).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 3:
Enable by default will be okay? Or we can still disable that by default and leave mainboard to decide that's enabled or disabled?
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 3:
(1 comment)
Enable by default will be okay? Or we can still disable that by default and leave mainboard to decide that's enabled or disabled?
I would prefer to enable it and not to have an option at all until somebody asks for an option to disable it. Or does anybody already want to disable it?
IMHO, the `VtdDisable` in `chip.h` should be (re)moved anyway: It's not a mainboard question but a question of the coreboot build and, thus, belongs into Kconfig.
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 says "is restricted" and, AFAIUI, it would only be restricted if we actually set up the respective page-tables + configure and enable the IOMMU.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
Enable by default will be okay? Or we can still disable that by default and leave mainboard to decide that's enabled or disabled?
I would prefer to enable it and not to have an option at all until somebody asks for an option to disable it. Or does anybody already want to disable it?
IMHO, the `VtdDisable` in `chip.h` should be (re)moved anyway: It's not a mainboard question but a question of the coreboot build and, thus, belongs into Kconfig.
Not I am aware of, just want to be more conservative here. But if no one is against it, I am okay to cast my vote.
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, Lijian Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#4).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 20 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/4
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 4:
(1 comment)
Updated to refer kconfig for VT-d enable feature.
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 says "is restricted" and, AFAIUI, it would only be restricted […]
The statement was simply copied/pasted from the VT-d specification.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 4: Code-Review+2
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
The statement was simply copied/pasted from the VT-d specification.
Yes, I know. So what does "restricted" mean in this context? That the IOMMU restricts it or that all drivers in the firmware are written to comply with RMRRs?
If it's the latter, how could coreboot know, given its payload model?
https://review.coreboot.org/#/c/32432/4/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/4/src/soc/intel/common/Kconfig@67 PS4, Line 67: default n Thank you for moving it to Kconfig.
Shouldn't this default to y and have a prompt? Without a prompt, it would have similar semantics as the device- tree setting, i.e. you have to edit the code to change it.
Um, but now that I think about it: if it had a prompt, it should be declared in cannonlake/Kconfig and not here. Otherwise, we would show the prompt for all SoCs using the common/ code.
John Zhao 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
Yes, I know. So what does "restricted" mean in this […]
It would be referred to IOMMU. I am not aware that firmware drivers implementation comply with RMRRs.
https://review.coreboot.org/#/c/32432/4/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/4/src/soc/intel/common/Kconfig@67 PS4, Line 67: default n
Thank you for moving it to Kconfig. […]
Yes, it is selected in cannonlake/Kconfig.
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?
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.
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, Lijian Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#5).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 23 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/5
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#6).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig M src/soc/intel/common/block/include/intelblocks/acpi.h 7 files changed, 23 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/6
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 6:
(3 comments)
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. […]
done
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. […]
Updated by simply adding DMA_CTRL_PLATFORM_OPT_IN_FLAG into soc/intel/common/block/include/intelblocks/acpi.h. I feel it should be handled properly in arch/x86/include/arch/acpi.h.
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? […]
done
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?
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 6:
(1 comment)
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
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."
IMHO, that's as ambiguous as the spec. "has made sure" by what means? IOMMU or proper driver code? I think the only chance to know is to ask the author of the spec to clarify it.
NB. To me both wordings read like the firmware has already configured the IOMMU's page tables and turned it on. But then I wonder why wouldn't they just say so?
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#7).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
By default VT-d is disabled by fsp. Enabling VT-d by coreboot through upd VtdDisable. Set bit2 DMA_CTRL_PLATFORM_OPT_IN_FLAG to report any platform initiated DMA is restricted to only reserved memory regions (reported in RMRR structures) when transferring control to system software.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 21 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/7
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 7:
Could we move forward with the setting?
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 7:
Patch Set 7:
Could we move forward with the setting?
Can you please help answer the outstanding question on the BIOS expectation if DMA_CTRL_PLATFORM_OPT_IN_FLAG is set?
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#8).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 19 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/8
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 8:
We are dropping DMA_CTRL_PLATFORM_OPT_IN_FLAG setting and will address it.
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 8: Code-Review+1
(2 comments)
Some final suggestions inline.
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c@345 PS8, Line 345: acpi_dmar_rmrr_fixup(tmp, current); Please mention this fix in the commit message.
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig@65 PS8, Line 65: config SOC_INTEL_COMMON_VTD This does nothing so far. Drop it until somebody needs an option to disable VT-d?
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#9).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable. Update remapping structure types in numerical order as all remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/9
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#10).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable. Update remapping structure types in numerical order as all remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 20 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/32432/10/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/10/src/soc/intel/common/Kconfig@67 PS10, Line 67: bool trailing whitespace
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#11).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable. Update remapping structure types in numerical order as all remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/Kconfig 6 files changed, 20 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/11
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c@345 PS8, Line 345: acpi_dmar_rmrr_fixup(tmp, current);
Please mention this fix in the commit message.
done
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig@65 PS8, Line 65: config SOC_INTEL_COMMON_VTD
This does nothing so far. Drop it until somebody needs an option to disable […]
It appears build failures if simply removing SOC_INTEL_COMMON_VTD here as soc/intel/cannonlake/Kconfig still refers to it.
"Error: Undefined Symbol 'SOC_INTEL_COMMON_VTD' used at src/soc/intel/cannonlake/Kconfig."
I am considering no update.
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 11: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/cannonlake/acpi.c@345 PS8, Line 345: acpi_dmar_rmrr_fixup(tmp, current);
done
Ack
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig@65 PS8, Line 65: config SOC_INTEL_COMMON_VTD
It appears build failures if simply removing SOC_INTEL_COMMON_VTD here as soc/intel/cannonlake/Kconfig still refers to it.
That reference is added in this very same patch. You can just remove it.
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32432
to look at the new patch set (#12).
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable. Update remapping structure types in numerical order as all remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c 4 files changed, 14 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/32432/12
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig File src/soc/intel/common/Kconfig:
https://review.coreboot.org/#/c/32432/8/src/soc/intel/common/Kconfig@65 PS8, Line 65: config SOC_INTEL_COMMON_VTD
It appears build failures if simply removing SOC_INTEL_COMMON_VTD here as soc/intel/cannonlake/Kco […]
done
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 12: Code-Review+2
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 12:
(1 comment)
https://review.coreboot.org/#/c/32432/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/12/src/soc/intel/cannonlake/acpi.c@340 PS12, Line 340: RMRR We should eventually think of doing this to ensure that more entries that are added don't miss the requirement: https://review.coreboot.org/c/coreboot/+/32432/6/src/soc/intel/cannonlake/ac...
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/32432/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/32432/12/src/soc/intel/cannonlake/acpi.c@340 PS12, Line 340: RMRR
We should eventually think of doing this to ensure that more entries that are added don't miss the r […]
sure, thanks.
Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
Patch Set 12: Code-Review+1
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 12:
(1 comment)
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: […]
I'm working on updating earlier platforms. Will look into this.
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32432 )
Change subject: soc/intel/cnl: Enable VT-d ......................................................................
soc/intel/cnl: Enable VT-d
Enable VT-d through fsp upd VtdDisable. Update remapping structure types in numerical order as all remapping structures of type 0 (DRHD) enumerated before remapping structures of type 1 (RMRR), and so forth.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I1d20932e417b9d324edd98c8f2195dc228d2e092 Signed-off-by: John Zhao john.zhao@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32432 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Pratikkumar V Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/systemagent.c 4 files changed, 14 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Pratikkumar V Prajapati: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c index 209e7c5..ff9da45 100644 --- a/src/soc/intel/cannonlake/acpi.c +++ b/src/soc/intel/cannonlake/acpi.c @@ -304,13 +304,6 @@ current += acpi_create_dmar_ds_pci(current, 0, 2, 0);
acpi_dmar_drhd_fixup(tmp, current); - - /* Add RMRR entry */ - tmp = current; - current += acpi_create_dmar_rmrr(current, 0, - sa_get_gsm_base(), sa_get_tolud_base() - 1); - current += acpi_create_dmar_ds_pci(current, 0, 2, 0); - acpi_dmar_rmrr_fixup(tmp, current); }
struct device *const ipu_dev = dev_find_slot(0, SA_DEVFN_IPU); @@ -344,6 +337,13 @@ acpi_dmar_drhd_fixup(tmp, current); }
+ /* Add RMRR entry */ + const unsigned long tmp = current; + current += acpi_create_dmar_rmrr(current, 0, + sa_get_gsm_base(), sa_get_tolud_base() - 1); + current += acpi_create_dmar_ds_pci(current, 0, 2, 0); + acpi_dmar_rmrr_fixup(tmp, current); + return current; }
@@ -361,6 +361,7 @@
printk(BIOS_DEBUG, "ACPI: * DMAR\n"); acpi_create_dmar(dmar, DMAR_INTR_REMAP, soc_fill_dmar); + current += dmar->header.length; current = acpi_align_current(current); acpi_add_table(rsdp, dmar); diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index f34528a..2b2a51f 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -344,9 +344,6 @@ /* Enable Pch iSCLK */ uint8_t pch_isclk;
- /* Intel VT configuration */ - uint8_t VtdDisable; - /* * Acoustic Noise Mitigation * 0b - Disable diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index c71e4b5..6e492bb 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -101,6 +101,7 @@ assert(dev != NULL); const config_t *config = dev->chip_info; FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + FSP_M_TEST_CONFIG *tconfig = &mupd->FspmTestConfig;
soc_memory_init_params(m_cfg, config);
@@ -113,6 +114,10 @@ /* Set debug probe type */ m_cfg->PlatformDebugConsent = CONFIG_SOC_INTEL_CANNONLAKE_DEBUG_CONSENT; + + /* Configure VT-d */ + tconfig->VtdDisable = 0; + mainboard_memory_init_params(mupd); }
diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index d850b15..3f01f14 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -33,8 +33,6 @@ */ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { - const struct soc_intel_cannonlake_config *const config = dev->chip_info; - static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, "PCIEXBAR" }, @@ -63,10 +61,8 @@ if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE)) return;
- if (!(config && config->VtdDisable)) { - sa_add_fixed_mmio_resources(dev, index, soc_vtd_resources, + sa_add_fixed_mmio_resources(dev, index, soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources)); - } }
/*
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
John Zhao (Code Review)
-
Lijian Zhao (Code Review)
-
Nico Huber (Code Review)
-
Pratikkumar V Prajapati (Code Review)