Change in coreboot[master]: soc/intel/xeon_sp: Lock down PCI BUSx:00.0 registers

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down PCI BUSx:00.0 registers
......................................................................

soc/intel/xeon_sp: Lock down PCI BUSx:00.0 registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/1

diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
index 39ee8b4..62c7548 100644
--- a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
+++ b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
@@ -132,6 +132,10 @@
 #define PC10_IOAPIC_ID                  0x13
 #define PC11_IOAPIC_ID                  0x14

+// DMI3 B0D0F0 registers
+#define DMIRCBAR		0x50
+#define ERRINJCON		0x1d8
+
 // D7F7 registers
 #define IIO_DFX_LCK_CTL		0x504

diff --git a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
index 8455378..d26c894 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
@@ -176,6 +176,10 @@
 #define PC10_IOAPIC_ID                  0x13
 #define PC11_IOAPIC_ID                  0x14

+// DMI3 B0D0F0 registers
+#define DMIRCBAR		0x50
+#define ERRINJCON		0x1d8
+
 // D7F7 registers
 #define IIO_DFX_LCK_CTL		0x504

diff --git a/src/soc/intel/xeon_sp/uncore.c b/src/soc/intel/xeon_sp/uncore.c
index b01301c..7462ae5 100644
--- a/src/soc/intel/xeon_sp/uncore.c
+++ b/src/soc/intel/xeon_sp/uncore.c
@@ -356,6 +356,31 @@
    .device  = MMAP_VTD_STACK_CFG_REG_DEVID,
 };

+static void dmi3_init(struct device *dev)
+{
+	uint16_t pci_err_inj_dis = pci_read_config16(dev, ERRINJCON);
+	if (!(pci_err_inj_dis & 1))
+		pci_update_config16(dev, ERRINJCON, 0xfe, 1);
+
+	uint32_t dmircbar = pci_read_config32(dev, DMIRCBAR);
+	if (!(dmircbar & 1))
+		pci_update_config32(dev, DMIRCBAR, 0xfffe, 1);
+}
+
+static struct device_operations dmi3_ops = {
+	.read_resources		= pci_dev_read_resources,
+	.set_resources		= pci_dev_set_resources,
+	.enable_resources	= pci_dev_enable_resources,
+	.init			= dmi3_init,
+	.ops_pci		= &soc_pci_ops,
+};
+
+static const struct pci_driver dmi3_driver __pci_driver = {
+	.ops		= &dmi3_ops,
+	.vendor		= PCI_VENDOR_ID_INTEL,
+	.device		= 0x2020,
+};
+
 static void d7_f7_init(struct device *dev)
 {
    uint16_t reg16;

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down PCI BUSx:00.0 registers
......................................................................

soc/intel/xeon_sp: Lock down PCI BUSx:00.0 registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/2

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/mainboard/ocp/deltalake/devicetree.cb
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
4 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/6

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/47448/6/src/mainboard/ocp/deltalake... 
PS6, Line 66: device pci 07.0 on end
move to next patch

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/6/src/soc/intel/xeon_sp/skx/i... 
File src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/47448/6/src/soc/intel/xeon_sp/skx/i... 
PS6, Line 181: #define ERRINJCON		0x1d8
CPX has an extra \n here

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 6: Code-Review+1

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/7

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47448/6/src/mainboard/ocp/deltalake... 
File src/mainboard/ocp/deltalake/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/47448/6/src/mainboard/ocp/deltalake... 
PS6, Line 66: device pci 07.0 on end
> move to next patch

done

https://review.coreboot.org/c/coreboot/+/47448/6/src/soc/intel/xeon_sp/skx/i... 
File src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/47448/6/src/soc/intel/xeon_sp/skx/i... 
PS6, Line 181: #define ERRINJCON		0x1d8
> CPX has an extra \n here

Done

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
PS9, Line 369: 	.device		= 0x2020,
How about defining the DID in a header file?

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
PS9, Line 351: 0xfe
Are these masks correct? They look very odd.

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 35 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/10

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
PS9, Line 351: 0xfe
> Are these masks correct? They look very odd.

Done

https://review.coreboot.org/c/coreboot/+/47448/9/src/soc/intel/xeon_sp/uncor... 
PS9, Line 369: 	.device		= 0x2020,
> How about defining the DID in a header file?

Done

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/10/src/soc/intel/xeon_sp/unco... 
File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/47448/10/src/soc/intel/xeon_sp/unco... 
PS10, Line 349: uint16_t pci_err_inj_dis = pci_read_config16(dev, ERRINJCON);
              : 	if (!(pci_err_inj_dis & 1))
              : 		pci_update_config16(dev, ERRINJCON, 0xfffe, 1);
              : 
              : 	uint32_t dmircbar = pci_read_config32(dev, DMIRCBAR);
              : 	if (!(dmircbar & 1))
              : 		pci_update_config32(dev, DMIRCBAR, 0xfffe, 1);
OK, I think I know what you want to do:

/* Disable error injection */
 pci_or_config16(dev, ERRINJCON, 1 << 0);

/* 
  * DMIRCBAR registers are not TXT lockable, but the BAR enable
  * bit is. TXT requires that DMIRCBAR be disabled for security. 
  */
 pci_and_config32(dev, DMIRCBAR, ~(1 << 0));

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CbNT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 36 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/11

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/10/src/soc/intel/xeon_sp/unco... 
File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/47448/10/src/soc/intel/xeon_sp/unco... 
PS10, Line 349: uint16_t pci_err_inj_dis = pci_read_config16(dev, ERRINJCON);
              : 	if (!(pci_err_inj_dis & 1))
              : 		pci_update_config16(dev, ERRINJCON, 0xfffe, 1);
              : 
              : 	uint32_t dmircbar = pci_read_config32(dev, DMIRCBAR);
              : 	if (!(dmircbar & 1))
              : 		pci_update_config32(dev, DMIRCBAR, 0xfffe, 1);
> OK, I think I know what you want to do: […]

Done

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 11: Code-Review+2

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47448/11//COMMIT_MSG@9 
PS11, Line 9: This is required for CbNT.
It is spelled as CBnT (Converged Bootguard and TXT).

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CBnT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 36 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47448/12

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47448/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47448/11//COMMIT_MSG@9 
PS11, Line 9: This is required for CbNT.
> It is spelled as CBnT (Converged Bootguard and TXT).

Done

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

Patch Set 12: Code-Review+2

Change in coreboot[master]: soc/intel/xeon_sp: Lock down DMI3 PCI registers

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47448 )

Change subject: soc/intel/xeon_sp: Lock down DMI3 PCI registers
......................................................................

soc/intel/xeon_sp: Lock down DMI3 PCI registers

This is required for CBnT.

Change-Id: If5637eb8dd7de406b24b92100b68c5fa11c16854
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47448
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
M src/soc/intel/xeon_sp/uncore.c
3 files changed, 36 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
index 198d385..6ddcce4 100644
--- a/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
+++ b/src/soc/intel/xeon_sp/cpx/include/soc/pci_devs.h
@@ -122,4 +122,9 @@
 // ========== IOAPIC Definitions for DMAR/ACPI ========
 #define PCH_IOAPIC_ID                   0x08

+// DMI3 B0D0F0 registers
+#define DMI3_DEVID		0x2020
+#define DMIRCBAR		0x50
+#define ERRINJCON		0x1d8
+
 #endif /* _SOC_PCI_DEVS_H_ */
diff --git a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
index ce223cc..5fa2a38 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/pci_devs.h
@@ -167,4 +167,9 @@
 // ========== IOAPIC Definitions for DMAR/ACPI ========
 #define PCH_IOAPIC_ID                   0x08

+// DMI3 B0D0F0 registers
+#define DMI3_DEVID		0x2020
+#define DMIRCBAR		0x50
+#define ERRINJCON		0x1d8
+
 #endif /* _SOC_PCI_DEVS_H_ */
diff --git a/src/soc/intel/xeon_sp/uncore.c b/src/soc/intel/xeon_sp/uncore.c
index 00623a8..2663023 100644
--- a/src/soc/intel/xeon_sp/uncore.c
+++ b/src/soc/intel/xeon_sp/uncore.c
@@ -348,3 +348,29 @@
    .vendor   = PCI_VENDOR_ID_INTEL,
    .device   = MMAP_VTD_STACK_CFG_REG_DEVID,
 };
+
+static void dmi3_init(struct device *dev)
+{
+	/* Disable error injection */
+	pci_or_config16(dev, ERRINJCON, 1 << 0);
+
+	/*
+	 * DMIRCBAR registers are not TXT lockable, but the BAR enable
+	 * bit is. TXT requires that DMIRCBAR be disabled for security.
+	 */
+	pci_and_config32(dev, DMIRCBAR, ~(1 << 0));
+}
+
+static struct device_operations dmi3_ops = {
+	.read_resources		= pci_dev_read_resources,
+	.set_resources		= pci_dev_set_resources,
+	.enable_resources	= pci_dev_enable_resources,
+	.init			= dmi3_init,
+	.ops_pci		= &soc_pci_ops,
+};
+
+static const struct pci_driver dmi3_driver __pci_driver = {
+	.ops		= &dmi3_ops,
+	.vendor		= PCI_VENDOR_ID_INTEL,
+	.device		= DMI3_DEVID,
+};