Change in coreboot[master]: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Vt-d based security platform requires the system firmware to clear bus master enable(BME) bit for all Thunderbolt PCIe root ports, bridges and devices at exit boot service. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/1
diff --git a/src/soc/intel/tigerlake/chip.c b/src/soc/intel/tigerlake/chip.c index 073c8d2..a0007f3 100644 --- a/src/soc/intel/tigerlake/chip.c +++ b/src/soc/intel/tigerlake/chip.c @@ -3,6 +3,7 @@
#include <device/device.h> #include <device/pci.h> +#include <device/pci_ids.h> #include <fsp/api.h> #include <fsp/util.h> #include <intelblocks/acpi.h> @@ -15,6 +16,7 @@ #include <soc/pci_devs.h> #include <soc/ramstage.h> #include <soc/soc_chip.h> +#include <soc/systemagent.h>
#if CONFIG(HAVE_ACPI_TABLES) const char *soc_acpi_name(const struct device *dev) @@ -114,6 +116,48 @@ gpio_pm_configure(value, TOTAL_GPIO_COMM); }
+static void clear_tbt_pcie_rp_bme(struct device *dev, uint16_t device_id) +{ + uint16_t reg16; + + if (dev) { + if ((pci_read_config16(dev, PCI_DEVICE_ID) == device_id)) { + reg16 = pci_read_config16(dev, PCI_COMMAND); + + /* Check if BME bit is enabled before */ + if ((reg16 & PCI_COMMAND_MASTER) == PCI_COMMAND_MASTER) { + reg16 &= ~PCI_COMMAND_MASTER; + pci_write_config16(dev, PCI_COMMAND, reg16); + } + } + } +} + +void platform_fsp_notify_status(enum fsp_notify_phase phase) +{ + if (phase == END_OF_FIRMWARE) { + struct device *dev; + + /* Check if VT-d is enabled */ + dev = pcidev_path_on_root(SA_DEVFN_ROOT); + if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE)) + return; + + /* Clear Thunderbolt PCIe root ports Bus Master Enable(BME) bit */ + dev = pcidev_path_on_root(SA_DEVFN_TBT0); + clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP0); + + dev = pcidev_path_on_root(SA_DEVFN_TBT1); + clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP1); + + dev = pcidev_path_on_root(SA_DEVFN_TBT2); + clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP2); + + dev = pcidev_path_on_root(SA_DEVFN_TBT3); + clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3); + } +} + void soc_init_pre_device(void *chip_info) { /* Snapshot the current GPIO IRQ polarities. FSP is setting a
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/1/src/soc/intel/tigerlake/chi... PS1, Line 124: if ((pci_read_config16(dev, PCI_DEVICE_ID) == device_id)) { Unnecessary parentheses - maybe == should be = ?
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Vt-d based security platform requires the system firmware to clear bus master enable(BME) bit for all Thunderbolt PCIe root ports, bridges and devices at exit boot service. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/1/src/soc/intel/tigerlake/chi... PS1, Line 124: if ((pci_read_config16(dev, PCI_DEVICE_ID) == device_id)) {
Unnecessary parentheses - maybe == should be = ?
Ack
Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Duncan Laurie, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Vt-d based security platform requires the system firmware to clear bus master enable(BME) bit for all Thunderbolt PCIe root ports, bridges and devices at exit boot service. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/3
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 125: reg16 = pci_read_config16(dev, PCI_COMMAND); : : /* Check if BME bit is enabled before */ : if ((reg16 & PCI_COMMAND_MASTER) == PCI_COMMAND_MASTER) { : reg16 &= ~PCI_COMMAND_MASTER; : pci_write_config16(dev, PCI_COMMAND, reg16); : } in terms of performance - no need to do read compare write - just write 0 to the bit.
Prashant Malani has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 123: if (dev) { nit: Kindly use a guard clause ; it will save one level of indentation:
if (!dev) return;
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 124: if (pci_read_config16(dev, PCI_DEVICE_ID) == device_id) { nit: Same here; a guard clause will save another level of indentation:
if (!dev) return;
if (pci_read_config(dev, PCI_DEVICE_ID) != device_id) return;
reg16 = pci_read_config16(...);
....
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 138: if (phase == END_OF_FIRMWARE) { nit: Guard clause here please:
if (phase != END_OF_FIRMWARE) return;
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 149: dev = pcidev_path_on_root(SA_DEVFN_TBT0); Does |dev} need to be checked for an error here? It seems like it's being done on L142, but I'm not sure what the semantics are for this function.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 123: if (dev) {
nit: Kindly use a guard clause ; it will save one level of indentation: […]
Ack
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 124: if (pci_read_config16(dev, PCI_DEVICE_ID) == device_id) {
nit: Same here; a guard clause will save another level of indentation: […]
Ack
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 125: reg16 = pci_read_config16(dev, PCI_COMMAND); : : /* Check if BME bit is enabled before */ : if ((reg16 & PCI_COMMAND_MASTER) == PCI_COMMAND_MASTER) { : reg16 &= ~PCI_COMMAND_MASTER; : pci_write_config16(dev, PCI_COMMAND, reg16); : }
in terms of performance - no need to do read compare write - just write 0 to the bit.
Ack
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 138: if (phase == END_OF_FIRMWARE) {
nit: Guard clause here please: […]
END_OF_FIMWARE is only one of boot stages that could be handled by platform_fsp_notify_status. Note: AFTER_PCI_ENUM(0x20), READY_TO_BOOT(0x40), END_OF_FIRMWARE(0xF0). "if (phase != END_OF_FRIMWARE) return" would not be considered.
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 149: dev = pcidev_path_on_root(SA_DEVFN_TBT0);
Does |dev} need to be checked for an error here? It seems like it's being done on L142, but I'm not […]
L142 checks for device associated with SA_DEVFN_ROOT. Other SA_DEVFN_TBTx devices are checked in the clear_tbt_pcie_rt_bme function.
Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Duncan Laurie, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Vt-d based security platform requires the system firmware to clear bus master enable(BME) bit for all Thunderbolt PCIe root ports, bridges and devices at exit boot service. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/4
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 123: if (dev) {
Ack
I believe that coreboot style actully dictates KnR - https://www.coreboot.org/Coding_Style#Placing_Braces_and_Spaces
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... PS4, Line 123: if (!dev) https://www.coreboot.org/Coding_Style#Placing_Braces_and_Spaces
Sorry for the trouble, can you please revert this to the original form? each if statement should have its own clause.
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... PS4, Line 126: if (pci_read_config16(dev, PCI_DEVICE_ID) != device_id) ditto
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/3/src/soc/intel/tigerlake/chi... PS3, Line 123: if (dev) {
I believe that coreboot style actully dictates KnR - https://www.coreboot. […]
apologies, ignore.
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... PS4, Line 123: if (!dev)
https://www.coreboot.org/Coding_Style#Placing_Braces_and_Spaces […]
Please ignore.
https://review.coreboot.org/c/coreboot/+/40968/4/src/soc/intel/tigerlake/chi... PS4, Line 126: if (pci_read_config16(dev, PCI_DEVICE_ID) != device_id)
ditto
Done
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 4: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG@10 PS4, Line 10: enable(BME) Please add a space before (.
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG@11 PS4, Line 11: and devices at exit boot service. In this state with BME bit cleared, Can you please give a reference, where this is specified?
Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Alex Levin, Duncan Laurie, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at exit boot service on Vt-d based security platform. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/5
Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Alex Levin, Duncan Laurie, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at exit boot service on Vt-d based security platform. In this state with BME bit cleared, the PCI root ports would be considered as trusted to not forward any DMA transaction to download endpoint devices.
BUG=141609884 TEST=built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/6
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG@10 PS4, Line 10: enable(BME)
Please add a space before (.
Ack
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(1 comment)
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/4//COMMIT_MSG@11 PS4, Line 11: and devices at exit boot service. In this state with BME bit cleared,
Can you please give a reference, where this is specified?
It is from Thunderbolt BIOS implementation guide which enumerates firmware development for Vt-d based security requirements. Those requirements are illustrated in the #141609884.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 119: , uint16_t device_id you could probably skip this check and assume coreboot code does the right thing and doesn't pass in the wrong struct device.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 119: clear_tbt_pcie_rp_bme This could go in src/include/device/pci.h as like "pci_disable_bus_master()" or something similar.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 119: , uint16_t device_id
you could probably skip this check and assume coreboot code does the right thing and doesn't pass in […]
Hm unless this was a check to see if the device isn't disabled? if so can you add a comment?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/6//COMMIT_MSG@11 PS6, Line 11: exit boot service What is that in coreboot terms?
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 134: platform_fsp_notify_status Why is it implemted here? Does FSP require BME to be cleared?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40968/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/6//COMMIT_MSG@11 PS6, Line 11: exit boot service
What is that in coreboot terms?
It would be equivalent to coreboot's END_OF_FIRMWARE boot state.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 119: , uint16_t device_id
Hm unless this was a check to see if the device isn't disabled? if so can you add a comment?
coreboot will be updated to check both dev and its device ID match before invoking the above pci_dev_disable_bus_master().
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 134: platform_fsp_notify_status
Why is it implemted here? Does FSP require BME to be cleared?
FSP is typically driven by coreboot. END_OF_FIRMWARE is a later boot stage where it is proper to disable bus master.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3); Why aren't we just adding a pci driver for the root ports so we can schedule work at the right place in the boot state machine? I think it'd be more straight forward. Have you explored this? Or we could just clear BME with our own init or updating of device->command.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 134: platform_fsp_notify_status
FSP is typically driven by coreboot. […]
That doesn't anser the question. Does it need to happen after fspnotify(END_OF_FIRMWARE)? Does it crash or complain?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 119: clear_tbt_pcie_rp_bme
This could go in src/include/device/pci.h as like "pci_disable_bus_master()" or something similar.
ok, CB:41042 with pci_dev_disable_bus_master().
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 134: platform_fsp_notify_status
That doesn't anser the question. […]
No crash or complain. As described in the commit message, it is for security season to avoid forwarding any DMA transaction to download endpoint devices before handing off to OS.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
Why aren't we just adding a pci driver for the root ports so we can schedule work at the right place […]
We only need to disable root ports BME before handing off to OS. It seems proper to implement at this late boot stage after bs_payload_load execution. Adding pci driver for root ports and scheduling seems for complicated scenarios. I did not explore. Please advise alternatively place to clear BME.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE)) I don't understand this logic. I would think the policy should not be dependent on VTD disable. I also am confused why we think it's necessary to not disable bme when VTD is disabled.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
We only need to disable root ports BME before handing off to OS. […]
Why is it ever appropriate to have BME set on these ports in the firmware? A pci driver is straight forward for this: The device operations can use all the defaults except for .enable_resources() where that would turn into the following sequence.
pci_dev_disable_bus_master(dev); dev->command &= ~PCI_COMMAND_MASTER; pci_dev_set_resources(dev);
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
I don't understand this logic. I would think the policy should not be dependent on VTD disable. […]
If Vt-d is enabled with opt_in(bit 2) in DMAR table along with PCIe root ports _DSD property: "ExternalFacingPort", kernel turns IOMMU on. Disabling BME at end of boot stage avoids DMA transaction to download endpoint devices.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
Why is it ever appropriate to have BME set on these ports in the firmware? A pci driver is straight […]
After bus scanning (static and generic), pci drivers resource operations (read/set/enable/assign) are completed at early boot stage before drivers are initialized.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
If Vt-d is enabled with opt_in(bit 2) in DMAR table along with PCIe root ports _DSD property: "ExternalFacingPort", kernel turns IOMMU on. Disabling BME at end of boot stage avoids DMA transaction to download endpoint devices.
My point was that we should be doing this no matter what and deferring to later sw to decide the policy. Keep it restricted until the OS or something else decides to enable it.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
After bus scanning (static and generic), pci drivers resource operations (read/set/enable/assign) ar […]
This didn't answer my question or I didn't understand the response. Why would the solution I proposed not work?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
If Vt-d is enabled with opt_in(bit 2) in DMAR table along with PCIe root ports _DSD property: "Ext […]
Disabling BME before handing off to OS is guided for Thunderbolt Vt-d security based platform. The implication seems not necessary to implement it if Vt-d is disabled.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
This didn't answer my question or I didn't understand the response. […]
As specified in Thunderbolt implementation guide on Vt-d security based platform, the requirement is to disable BME before handing off to OS. Your proposal through pci driver resource operation is unconditionally implemented at early boot stage. The question arises from the policy of disabling BME as to what, when and where the implementation should be done.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
Disabling BME before handing off to OS is guided for Thunderbolt Vt-d security based platform. The implication seems not necessary to implement it if Vt-d is disabled.
We don't lose anything by unconditionally disabling BME. I think it is necessary from a policy perspective. We can err on the conservative side and let the other sw that runs make decisions. I don't think we should be limiting our policy based on VT-d or not.
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
As specified in Thunderbolt implementation guide on Vt-d security based platform, the requirement is […]
Thunderbolt has many security vulnerabilities. VT-d is one way to help plug some of those. That is why we should unconditionally disable BME and let the OS make the policy.
Chiranjeevi Rapolu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
Cherry-picked this. On my DUT, it got stuck:
Starting depthcharge on Deltan... WARNING: can't convert coreboot GPIOs, 'lid' won't be resampled at runtime! WARNING: can't convert coreboot GPIOs, 'power' won't be resampled at runtime! WARNING: can't convert coreboot GPIOs, 'EC in RW' won't be resampled at runtime! BIOS MMAP details: IFD Base Offset : 0x1000000 IFD End Offset : 0x2000000 MMAP Size : 0x1000000 MMAP Start : 0xff000000 Wilco EC [base 0x0940 emi 0x0950] flash 0x00001000-0x00100fff Looking for NVMe Controller 0x32038558 @ 00:1d:00 Wipe memory regions: [0x00000000001000, 0x000000000a0000) [0x00000000100000, 0x00000030000000) [0x0000003263be20, 0x00000076a0f000) [0x00000100000000, 0x00000180400000) xhci_wait_for_command: Command ring still running ===> Stuck here forever!
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
Patch Set 6:
Cherry-picked this. On my DUT, it got stuck:
Starting depthcharge on Deltan... WARNING: can't convert coreboot GPIOs, 'lid' won't be resampled at runtime! WARNING: can't convert coreboot GPIOs, 'power' won't be resampled at runtime! WARNING: can't convert coreboot GPIOs, 'EC in RW' won't be resampled at runtime! BIOS MMAP details: IFD Base Offset : 0x1000000 IFD End Offset : 0x2000000 MMAP Size : 0x1000000 MMAP Start : 0xff000000 Wilco EC [base 0x0940 emi 0x0950] flash 0x00001000-0x00100fff Looking for NVMe Controller 0x32038558 @ 00:1d:00 Wipe memory regions: [0x00000000001000, 0x000000000a0000) [0x00000000100000, 0x00000030000000) [0x0000003263be20, 0x00000076a0f000) [0x00000100000000, 0x00000180400000) xhci_wait_for_command: Command ring still running ===> Stuck here forever!
That sounds irrational as the patch only disables TBT root ports BME. The "xhci_wait_for_command: Command ring still running;" refers to lippayload xhci's CRR check. On our sides, multiple DUT boot reliably to kernel either from nvme or usb after cherry-pick this patch.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Clear TBT PCIe root ports BME at exit boot service ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 142: if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE))
Disabling BME before handing off to OS is guided for Thunderbolt Vt-d security based platform. […]
Will update with src/drivers/intel/tbt driver on the dependency CB:41042
https://review.coreboot.org/c/coreboot/+/40968/6/src/soc/intel/tigerlake/chi... PS6, Line 157: clear_tbt_pcie_rp_bme(dev, PCI_DEVICE_ID_INTEL_TGL_TBT_RP3);
Thunderbolt has many security vulnerabilities. VT-d is one way to help plug some of those. […]
Will update with src/drivers/intel/tbt driver on the dependency CB:41042.
Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/7
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/8
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 8:
There has been lots of discussion here, but nobody asked if ramstage is really the right place to do that: Disabling bus master for TBT is to prevent wild (potentially hostile) DMA accesses early on in the boot cycle, right? Ideally we'd do that in the bootblock, or in romstage before RAM is available or used for anything, at any rate.
If we're deferring this to ramstage, an attacker could just overwrite the ramstage before its executed: blast the entry point with "jmp -1" for 5 seconds, then write whatever code they really want to see executed somewhere and overwrite the jmp again. Since we're typically decompressing the ramstage, and decompression works from low to high addresses, while the entry point is at the beginning, it shouldn't even be hard to win that race.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 8:
There has been lots of discussion here, but nobody asked if ramstage is really the right place to do that: Disabling bus master for TBT is to prevent wild (potentially hostile) DMA accesses early on in the boot cycle, right? Ideally we'd do that in the bootblock, or in romstage before RAM is available or used for anything, at any rate.
If we're deferring this to ramstage, an attacker could just overwrite the ramstage before its executed: blast the entry point with "jmp -1" for 5 seconds, then write whatever code they really want to see executed somewhere and overwrite the jmp again. Since we're typically decompressing the ramstage, and decompression works from low to high addresses, while the entry point is at the beginning, it shouldn't even be hard to win that race.
Is bus mastering enabled early, though? It would be nice to document the reset state and if FSP does anything about it.
Beside that, I doubt that this is TBT specific. We shouldn't enable BM for anything that we don't use. 99% of the BM enable settings in coreboot seem more like a bug than a feature. I do understand that this might not be the right time to clean it up. But I'm with Patrick here, we should prevent that BM gets enabled at all (or disable it as early as possible if it's enabled due to some bug that can't be fixed), not disable it at a random point later just to comply with a document.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#9).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/9
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 9:
Patch Set 8:
There has been lots of discussion here, but nobody asked if ramstage is really the right place to do that: Disabling bus master for TBT is to prevent wild (potentially hostile) DMA accesses early on in the boot cycle, right? Ideally we'd do that in the bootblock, or in romstage before RAM is available or used for anything, at any rate.
If we're deferring this to ramstage, an attacker could just overwrite the ramstage before its executed: blast the entry point with "jmp -1" for 5 seconds, then write whatever code they really want to see executed somewhere and overwrite the jmp again. Since we're typically decompressing the ramstage, and decompression works from low to high addresses, while the entry point is at the beginning, it shouldn't even be hard to win that race.
Is bus mastering enabled early, though? It would be nice to document the reset state and if FSP does anything about it.
Beside that, I doubt that this is TBT specific. We shouldn't enable BM for anything that we don't use. 99% of the BM enable settings in coreboot seem more like a bug than a feature. I do understand that this might not be the right time to clean it up. But I'm with Patrick here, we should prevent that BM gets enabled at all (or disable it as early as possible if it's enabled due to some bug that can't be fixed), not disable it at a random point later just to comply with a document.
Based on Coreboot's TBT setting, FSP configures TBT without enabling TBT's BME at romstage. There is no coreboot code with iommu at pre-boot to block DMA access to critical memory region in the early boot phage, nor DMA remapping in later boot phase to enable BME.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 8:
There has been lots of discussion here, but nobody asked if ramstage is really the right place to do that: Disabling bus master for TBT is to prevent wild (potentially hostile) DMA accesses early on in the boot cycle, right? Ideally we'd do that in the bootblock, or in romstage before RAM is available or used for anything, at any rate.
If we're deferring this to ramstage, an attacker could just overwrite the ramstage before its executed: blast the entry point with "jmp -1" for 5 seconds, then write whatever code they really want to see executed somewhere and overwrite the jmp again. Since we're typically decompressing the ramstage, and decompression works from low to high addresses, while the entry point is at the beginning, it shouldn't even be hard to win that race.
Is bus mastering enabled early, though? It would be nice to document the reset state and if FSP does anything about it.
Beside that, I doubt that this is TBT specific. We shouldn't enable BM for anything that we don't use. 99% of the BM enable settings in coreboot seem more like a bug than a feature. I do understand that this might not be the right time to clean it up. But I'm with Patrick here, we should prevent that BM gets enabled at all (or disable it as early as possible if it's enabled due to some bug that can't be fixed), not disable it at a random point later just to comply with a document.
Based on Coreboot's TBT setting, FSP configures TBT without enabling TBT's BME at romstage. There is no coreboot code with iommu at pre-boot to block DMA access to critical memory region in the early boot phase, nor DMA remapping in later boot phase to enable BME.
It seems no coreboot/fsp manipulating TBT BME at early boot. This patch tries to disable TBT BME at ramstage. Please advise.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 8:
There has been lots of discussion here, but nobody asked if ramstage is really the right place to do that: Disabling bus master for TBT is to prevent wild (potentially hostile) DMA accesses early on in the boot cycle, right? Ideally we'd do that in the bootblock, or in romstage before RAM is available or used for anything, at any rate.
If we're deferring this to ramstage, an attacker could just overwrite the ramstage before its executed: blast the entry point with "jmp -1" for 5 seconds, then write whatever code they really want to see executed somewhere and overwrite the jmp again. Since we're typically decompressing the ramstage, and decompression works from low to high addresses, while the entry point is at the beginning, it shouldn't even be hard to win that race.
Is bus mastering enabled early, though? It would be nice to document the reset state and if FSP does anything about it.
Beside that, I doubt that this is TBT specific. We shouldn't enable BM for anything that we don't use. 99% of the BM enable settings in coreboot seem more like a bug than a feature. I do understand that this might not be the right time to clean it up. But I'm with Patrick here, we should prevent that BM gets enabled at all (or disable it as early as possible if it's enabled due to some bug that can't be fixed), not disable it at a random point later just to comply with a document.
Based on Coreboot's TBT setting, FSP configures TBT without enabling TBT's BME at romstage. There is no coreboot code with iommu at pre-boot to block DMA access to critical memory region in the early boot phase, nor DMA remapping in later boot phase to enable BME.
It seems no coreboot/fsp manipulating TBT BME at early boot. This patch tries to disable TBT BME at ramstage. Please advise.
FWIW, the EDS shows that the BME bit is disabled at reset for all PCIe devices, D29:F0-F7, D28:F0-F7, and D27: F0-F7. Given that FSP updates may happen and its policies may change, I agree it's probably safest to disable BME before handing off control to the payload, as well as in bootblock.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#10).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/10
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/10//COMMIT_MSG@14 PS10, Line 14: 141609884 add b: prefix
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#11).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=b:141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/11
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40968/10//COMMIT_MSG@14 PS10, Line 14: 141609884
add b: prefix
Ack
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#12).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=b:141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/12
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 12:
(3 comments)
In another CL, shouldn't we should also add an early_tbt to disable bus mastering in bootblock?
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/Kcon... File src/drivers/intel/tbt/Kconfig:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/Kcon... PS12, Line 5: suppor support
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 2: /* This file is part of the coreboot project. */ We don't add this line anymore.
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 12: static struct pci_operations tbt_pci_ops = { : .set_subsystem = NULL, : }; Not necessary, there is a NULL-check for set_subsystem before it's used.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 21: .scan_bus = pci_scan_bridge, Will the TBT PCI devices have devices downstream?
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 22: .reset_bus = pci_bus_reset, Why is this required?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 12:
(1 comment)
In another CL, shouldn't we should also add an early_tbt to disable bus mastering in bootblock?
It shouldn't be enabled. We did some tests lately (on CFL mostly) and couldn't find a single BM bit set that wasn't explicitly set by coreboot (or hardwired to 1). There are also some plans to keep record of intentional BM setting and a warning at the end of coreboot if the hardware state mismatches (or optio- nally correction of that state). Taking small steps for now, starting with CB:42459.
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 21: .scan_bus = pci_scan_bridge,
Will the TBT PCI devices have devices downstream?
There are at least fake devices to reserve resources for hot-plugging. I am not sure, though, if the function is needed for those.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/Kcon... File src/drivers/intel/tbt/Kconfig:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/Kcon... PS12, Line 5: suppor
support
Done
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 2: /* This file is part of the coreboot project. */
We don't add this line anymore.
Done
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 12: static struct pci_operations tbt_pci_ops = { : .set_subsystem = NULL, : };
Not necessary, there is a NULL-check for set_subsystem before it's used.
Done
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 21: .scan_bus = pci_scan_bridge,
There are at least fake devices to reserve resources for hot-plugging. I am […]
Done
https://review.coreboot.org/c/coreboot/+/40968/12/src/drivers/intel/tbt/tbt.... PS12, Line 22: .reset_bus = pci_bus_reset,
Why is this required?
reset_bus and scan_bus are default device operations for pci bridges. It seems proper.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#13).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=b:141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/13
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/tbt.... PS13, Line 12: .read_resources = pci_bus_read_resources, : .set_resources = pci_dev_set_resources, : .enable_resources = pci_bus_enable_resources, : .init = tbt_pci_init, : .scan_bus = pci_scan_bridge, : .reset_bus = pci_bus_reset, Please use tabs for aligning the equal sign =.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 13:
(1 comment)
Why
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/Kcon... File src/drivers/intel/tbt/Kconfig:
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/Kcon... PS13, Line 5: When enabled, chip driver/intel/tbt will add support for TBT. Why is a separate driver needed? Isn’t there another way to hook it up automatically?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/Kcon... File src/drivers/intel/tbt/Kconfig:
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/Kcon... PS13, Line 5: When enabled, chip driver/intel/tbt will add support for TBT.
Why is a separate driver needed? Isn’t there another way to hook it up automatically?
This driver can be enabled through platform configuration. i.e, add "select DRIVERS_INTEL_TBT" in mainboard/google/volteer/Kconfig.
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/tbt.... File src/drivers/intel/tbt/tbt.c:
https://review.coreboot.org/c/coreboot/+/40968/13/src/drivers/intel/tbt/tbt.... PS13, Line 12: .read_resources = pci_bus_read_resources, : .set_resources = pci_dev_set_resources, : .enable_resources = pci_bus_enable_resources, : .init = tbt_pci_init, : .scan_bus = pci_scan_bridge, : .reset_bus = pci_bus_reset,
Please use tabs for aligning the equal sign =.
Done
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#14).
Change subject: soc/intel/tigerlake: Disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=b:141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/14
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#15).
Change subject: soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master
Thunderbolt firmware implementation guide requires firmware to clear the bus master enable (BME) bit for all PCIe root ports, bridges and devices at boot stage. Implement disabling TBT root ports BME at TBT driver initialization.
BUG=b:141609884 TEST=Built image and booted to kernel successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- A src/drivers/intel/tbt/Kconfig A src/drivers/intel/tbt/Makefile.inc A src/drivers/intel/tbt/tbt.c 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/15
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master ......................................................................
Patch Set 15: Code-Review-1
Divya S Sasidharan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master ......................................................................
Patch Set 15:
Verified with disabling BME (with Kconfig) at early stage (Patchset 15) disables PCIe resource allocation, which will then affect USB behind TBT dock enumeration at kernel.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master ......................................................................
Patch Set 15:
Patch Set 15:
Verified with disabling BME (with Kconfig) at early stage (Patchset 15) disables PCIe resource allocation, which will then affect USB behind TBT dock enumeration at kernel.
The test results shows initial patchset would be preferable as disabling BME at late stage before handling to payload.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Provide support to disable TBT PCIe root ports bus master ......................................................................
Patch Set 14:
Patch Set 12:
(1 comment)
In another CL, shouldn't we should also add an early_tbt to disable bus mastering in bootblock?
It shouldn't be enabled. We did some tests lately (on CFL mostly) and couldn't find a single BM bit set that wasn't explicitly set by coreboot (or hardwired to 1). There are also some plans to keep record of intentional BM setting and a warning at the end of coreboot if the hardware state mismatches (or optio- nally correction of that state). Taking small steps for now, starting with CB:42459.
Thanks for the pointer Nico
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#16).
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master
This change disables Thunderbolt PCIe root ports bus master before handing over to payload in order to mitigate the threat from the unauthorized external DMA. In this state, the PCIe root ports would be considered as trusted to not forward any DMA transactions to downstream endpoint devices.
BUG=b:141609884 TEST=Verified PCIe resource has been allocated properly and USB behind Thunderbolt dock is enumerated successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/chip.c 1 file changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/16
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 16:
(1 comment)
Patch Set 15:
Patch Set 15:
Verified with disabling BME (with Kconfig) at early stage (Patchset 15) disables PCIe resource allocation, which will then affect USB behind TBT dock enumeration at kernel.
The test results shows initial patchset would be preferable as disabling BME at late stage before handling to payload.
https://review.coreboot.org/c/coreboot/+/40968/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/16/src/soc/intel/tigerlake/ch... PS16, Line 126: : void platform_fsp_notify_status(enum fsp_notify_phase phase) : { : if (phase == END_OF_FIRMWARE) { : struct device *dev; : : /* Disable Thunderbolt PCIe root ports bus master */ : dev = pcidev_path_on_root(SA_DEVFN_TBT0); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT1); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT2); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT3); : if (dev) : pci_dev_disable_bus_master(dev); : } : } Since disabling the bus mastering this doesn't really depend on the FSP notify, maybe this shuld go in a BS_POST_DEVICE callback? That will happen post-FSP-S as well as post-PCI enumeration.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Wonkyu Kim, Caveh Jalali, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Patrick Rudolph, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#17).
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master
This change disables Thunderbolt PCIe root ports bus master before handing over to payload in order to mitigate the threat from the unauthorized external DMA. In this state, the PCIe root ports would be considered as trusted to not forward any DMA transactions to downstream endpoint devices.
BUG=b:141609884 TEST=Verified PCIe resource has been allocated properly and USB behind Thunderbolt dock is enumerated successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/finalize.c 1 file changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/17
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/40968/16/src/soc/intel/tigerlake/ch... PS16, Line 126: : void platform_fsp_notify_status(enum fsp_notify_phase phase) : { : if (phase == END_OF_FIRMWARE) { : struct device *dev; : : /* Disable Thunderbolt PCIe root ports bus master */ : dev = pcidev_path_on_root(SA_DEVFN_TBT0); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT1); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT2); : if (dev) : pci_dev_disable_bus_master(dev); : : dev = pcidev_path_on_root(SA_DEVFN_TBT3); : if (dev) : pci_dev_disable_bus_master(dev); : } : }
Since disabling the bus mastering this doesn't really depend on the FSP notify, maybe this shuld go […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... PS17, Line 90: pci_dev_disable_bus_master(dev); Can’t this be written in a for loop?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 17: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... PS17, Line 90: pci_dev_disable_bus_master(dev);
Can’t this be written in a for loop?
Yes, one can add a few helper #defines in a header:
#define SA_DEVFN_TBT(x) PCI_DEVFN(SA_DEV_SLOT_TBT, (x)) #define NUM_TBT_FUNCTIONS 4
Then write this function as:
static void tbt_finalize(void) { int i;
/* Disable Thunderbolt PCIe root ports bus master */ for (i = 0; i < NUM_TBT_FUNCTIONS; i++) { struct device *const dev = pcidev_path_on_root(SA_DEVFN_TBT(i) if (dev) pci_dev_disable_bus_master(dev); } }
Hello build bot (Jenkins), Patrick Georgi, Wonkyu Kim, Duncan Laurie, Alex Levin, Shamile Khan, Prashant Malani, Angel Pons, Patrick Rudolph, Martin Roth, Caveh Jalali, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40968
to look at the new patch set (#18).
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master
This change disables Thunderbolt PCIe root ports bus master before handing over to payload in order to mitigate the threat from the unauthorized external DMA. In this state, the PCIe root ports would be considered as trusted to not forward any DMA transactions to downstream endpoint devices.
BUG=b:141609884 TEST=Verified PCIe resource has been allocated properly and USB behind Thunderbolt dock is enumerated successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/tigerlake/finalize.c M src/soc/intel/tigerlake/include/soc/pci_devs.h 2 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/40968/18
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/40968/17/src/soc/intel/tigerlake/fi... PS17, Line 90: pci_dev_disable_bus_master(dev);
Yes, one can add a few helper #defines in a header: […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
Patch Set 18: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40968 )
Change subject: soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master ......................................................................
soc/intel/tigerlake: Disable Thunderbolt PCIe root ports bus master
This change disables Thunderbolt PCIe root ports bus master before handing over to payload in order to mitigate the threat from the unauthorized external DMA. In this state, the PCIe root ports would be considered as trusted to not forward any DMA transactions to downstream endpoint devices.
BUG=b:141609884 TEST=Verified PCIe resource has been allocated properly and USB behind Thunderbolt dock is enumerated successfully.
Change-Id: I9650b9dd4df1f9bee53ae3737b7bf60b2ef8017b Signed-off-by: John Zhao john.zhao@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40968 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/tigerlake/finalize.c M src/soc/intel/tigerlake/include/soc/pci_devs.h 2 files changed, 16 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/finalize.c b/src/soc/intel/tigerlake/finalize.c index 534abd9..5bf01de 100644 --- a/src/soc/intel/tigerlake/finalize.c +++ b/src/soc/intel/tigerlake/finalize.c @@ -67,12 +67,26 @@ pmc_clear_pmcon_sts(); }
+static void tbt_finalize(void) +{ + int i; + const struct device *dev; + + /* Disable Thunderbolt PCIe root ports bus master */ + for (i = 0; i < NUM_TBT_FUNCTIONS; i++) { + dev = pcidev_path_on_root(SA_DEVFN_TBT(i)); + if (dev) + pci_dev_disable_bus_master(dev); + } +} + static void soc_finalize(void *unused) { printk(BIOS_DEBUG, "Finalizing chipset.\n");
pch_finalize(); apm_control(APM_CNT_FINALIZE); + tbt_finalize();
/* Indicate finalize step with post code */ post_code(POST_OS_BOOT); diff --git a/src/soc/intel/tigerlake/include/soc/pci_devs.h b/src/soc/intel/tigerlake/include/soc/pci_devs.h index ee3e894..82d8360 100644 --- a/src/soc/intel/tigerlake/include/soc/pci_devs.h +++ b/src/soc/intel/tigerlake/include/soc/pci_devs.h @@ -35,6 +35,8 @@ #define SA_DEV_IPU PCI_DEV(0, SA_DEV_SLOT_IPU, 0)
#define SA_DEV_SLOT_TBT 0x07 +#define SA_DEVFN_TBT(x) PCI_DEVFN(SA_DEV_SLOT_TBT, (x)) +#define NUM_TBT_FUNCTIONS 4 #define SA_DEVFN_TBT0 PCI_DEVFN(SA_DEV_SLOT_TBT, 0) #define SA_DEVFN_TBT1 PCI_DEVFN(SA_DEV_SLOT_TBT, 1) #define SA_DEVFN_TBT2 PCI_DEVFN(SA_DEV_SLOT_TBT, 2)
-
Aaron Durbin (Code Review) -
Alex Levin (Code Review)
-
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Caveh Jalali (Code Review)
-
Chiranjeevi Rapolu (Code Review)
-
Divya S Sasidharan (Code Review)
-
Duncan Laurie (Code Review)
-
John Zhao (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Prashant Malani (Code Review)
-
Tim Wawrzynczak (Code Review)