Change in coreboot[master]: device: Add a disbling PCIe device bus master function

[John Zhao (Code Review)]

John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disbling PCIe device bus master function
......................................................................

device: Add a disbling PCIe device bus master function

A function pci_dev_disable_bus_master() is created. This function
can be used to disable Thunderbolt PCIe root ports, bridges and
devices for Vt-d based security platform at end of boot service.

BUG=None
TEST=verified PCIe device bus master enable bit is cleared.

Signed-off-by: John Zhao 
Change-Id: Ie92a15bf2c66fdc311098acb81019d4fb7f68313
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/41042/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 6fce761..7c1b9d8 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -1568,4 +1568,20 @@
 #endif
    }
 }
+
+/**
+ * Disable device bus master.
+ *
+ * This function is to disable device bus master.
+ *
+ * @param dev Pointer to the device.
+ */
+void pci_dev_disable_bus_master(struct device *dev)
+{
+	u16 reg;
+
+	reg = pci_read_config16(dev, PCI_COMMAND);
+	reg &= ~PCI_COMMAND_MASTER;
+	pci_write_config16(dev, PCI_COMMAND, reg);
+}
 #endif
diff --git a/src/include/device/pci.h b/src/include/device/pci.h
index f091105..2283359 100644
--- a/src/include/device/pci.h
+++ b/src/include/device/pci.h
@@ -127,6 +127,7 @@
    return (attr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY;
 }

+void pci_dev_disable_bus_master(struct device *dev);
 #endif /* CONFIG_PCI */

void pci_early_bridge_init(void);

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disbling PCIe device bus master function
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG@7 
PS1, Line 7: disbling
disabling

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG@14 
PS1, Line 14: verified
Verified

https://review.coreboot.org/c/coreboot/+/41042/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/1/src/device/pci_device.c@158... 
PS1, Line 1585: 	pci_write_config16(dev, PCI_COMMAND, reg);
`pci_update_config16()` allows you to do this in one line.

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disbling PCIe device bus master function
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG@7 
PS1, Line 7: disbling
> disabling

Ack

https://review.coreboot.org/c/coreboot/+/41042/1//COMMIT_MSG@14 
PS1, Line 14: verified
> Verified

Ack

https://review.coreboot.org/c/coreboot/+/41042/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/1/src/device/pci_device.c@158... 
PS1, Line 1585: 	pci_write_config16(dev, PCI_COMMAND, reg);
> `pci_update_config16()` allows you to do this in one line.

pci_update_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MASTER, 0x0)

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

Hello build bot (Jenkins), Nico Huber, Duncan Laurie,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41042

to look at the new patch set (#2).

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

device: Add a disabling PCIe device bus master function

A function pci_dev_disable_bus_master() is created. This function
can be used to disable Thunderbolt PCIe root ports, bridges and
devices for Vt-d based security platform at end of boot service.

BUG=None
TEST=Verified PCIe device bus master enable bit is cleared.

Signed-off-by: John Zhao 
Change-Id: Ie92a15bf2c66fdc311098acb81019d4fb7f68313
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/41042/2

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41042/2/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/2/src/device/pci_device.c@157... 
PS2, Line 1572: /**
              :  * Disable device bus master.
              :  *
              :  * This function is to disable device bus master.
              :  *
              :  * @param dev Pointer to the device.
              :  */
Please don't write boiler-plate comments.

Sorry for being picky, but IMHO this comment is just a waste of space. It repeats the
function name two times and then the declaration of `dev` one time. Why? All this
information is (even easier to read, IMHO) in the next line. So I don't see the need
for a comment.

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41042/2/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/2/src/device/pci_device.c@157... 
PS2, Line 1572: /**
              :  * Disable device bus master.
              :  *
              :  * This function is to disable device bus master.
              :  *
              :  * @param dev Pointer to the device.
              :  */
> Please don't write boiler-plate comments. […]

Ack

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

Hello build bot (Jenkins), Nico Huber, Duncan Laurie,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41042

to look at the new patch set (#3).

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

device: Add a disabling PCIe device bus master function

A function pci_dev_disable_bus_master() is created. This function
can be used to disable Thunderbolt PCIe root ports, bridges and
devices for Vt-d based security platform at end of boot service.

BUG=None
TEST=Verified PCIe device bus master enable bit is cleared.

Signed-off-by: John Zhao 
Change-Id: Ie92a15bf2c66fdc311098acb81019d4fb7f68313
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/41042/3

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c@157... 
PS3, Line 1572: void pci_dev_disable_bus_master(struct device *dev)
const struct

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c@157... 
PS3, Line 1572: void pci_dev_disable_bus_master(struct device *dev)
> const struct

sorry, you meant from "struct device *dev" to "const struct device *dev"? it seems not necessary.

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c@157... 
PS3, Line 1572: void pci_dev_disable_bus_master(struct device *dev)
> sorry, you meant from "struct device *dev" to "const struct device *dev"? it seems not necessary.

it just shows the compiler that you're not modifying the contents of what's pointed at.

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

Hello build bot (Jenkins), Nico Huber, Duncan Laurie,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41042

to look at the new patch set (#4).

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

device: Add a disabling PCIe device bus master function

A function pci_dev_disable_bus_master() is created. This function
can be used to disable Thunderbolt PCIe root ports, bridges and
devices for Vt-d based security platform at end of boot service.

BUG=None
TEST=Verified PCIe device bus master enable bit is cleared.

Signed-off-by: John Zhao 
Change-Id: Ie92a15bf2c66fdc311098acb81019d4fb7f68313
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/41042/4

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 4:

(1 comment)
> Patch Set 3:
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41042/3/src/device/pci_device.c@157... 
PS3, Line 1572: void pci_dev_disable_bus_master(struct device *dev)
> it just shows the compiler that you're not modifying the contents of what's pointed at.

Ack

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

device: Add a disabling PCIe device bus master function

A function pci_dev_disable_bus_master() is created. This function
can be used to disable Thunderbolt PCIe root ports, bridges and
devices for Vt-d based security platform at end of boot service.

BUG=None
TEST=Verified PCIe device bus master enable bit is cleared.

Signed-off-by: John Zhao 
Change-Id: Ie92a15bf2c66fdc311098acb81019d4fb7f68313
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41042
Tested-by: build bot (Jenkins) 
Reviewed-by: Tim Wawrzynczak 
---
M src/device/pci_device.c
M src/include/device/pci.h
2 files changed, 6 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index f83520e..032e15c 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -1643,4 +1643,9 @@
 #endif
    }
 }
+
+void pci_dev_disable_bus_master(const struct device *dev)
+{
+	pci_update_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MASTER, 0x0);
+}
 #endif
diff --git a/src/include/device/pci.h b/src/include/device/pci.h
index f091105..4529074 100644
--- a/src/include/device/pci.h
+++ b/src/include/device/pci.h
@@ -127,6 +127,7 @@
    return (attr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY;
 }

+void pci_dev_disable_bus_master(const struct device *dev);
 #endif /* CONFIG_PCI */

void pci_early_bridge_init(void);

Change in coreboot[master]: device: Add a disabling PCIe device bus master function

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41042 )

Change subject: device: Add a disabling PCIe device bus master function
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/5293
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5292
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5291
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/5290

Please note: This test is under development and might not be accurate at all!