Change in coreboot[master]: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for...
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports ......................................................................
src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports
Currently it is impossible to enable ACS with AGESA by setting the correct bit for AmdInitMid phase. AGESA code path does not call the right function that enables these functionalities. Disabled ACS result in multiple PCIe devices to be assigned to the same IOMMU group. Without IOMMU group separation the devices cannot be passed through independently.
Enable Access Control Services and Advanced Error Reporting for PCI Express bridges in order to have PCIe devices in separate IOMMU groups for correct passthrough.
TEST=run dmesg on Debian Buster and check whether PCIe devices have separate groups
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I10a8eff0ba37196692f9db6519e498fe535ecd15 --- M src/northbridge/amd/pi/00730F01/northbridge.c 1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/35313/1
diff --git a/src/northbridge/amd/pi/00730F01/northbridge.c b/src/northbridge/amd/pi/00730F01/northbridge.c index ba17c61..fede1e9 100644 --- a/src/northbridge/amd/pi/00730F01/northbridge.c +++ b/src/northbridge/amd/pi/00730F01/northbridge.c @@ -783,6 +783,25 @@ pci_write_config32(dev, 0xF8, 0); pci_write_config32(dev, 0xFC, 5); /* TODO: move it to dsdt.asl */
+ /* Select GPP link core IO Link Strap Control register 0xB0 */ + pci_write_config32(dev, 0xE0, 0x014000B0); + value = pci_read_config32(dev, 0xE4); + + /* Enable AER (bit 5) and ACS (bit 6 undocumented) */ + value |= (BIT(5) | BIT(6)); + pci_write_config32(dev, 0xE4, value); + + /* Select GPP link core Wrapper register 0x00 (undocumented) */ + pci_write_config32(dev, 0xE0, 0x01300000); + value = pci_read_config32(dev, 0xE4); + + /* Enable ACS capabilities straps including sub-items. From lspci it + * looks like these bits enable: Source Validation and Translation + * Blocking + */ + value |= (BIT(24) | BIT(25) | BIT(26)); + pci_write_config32(dev, 0xE4, value); + /* disable No Snoop */ dev = pcidev_on_root(1, 1); if (dev != NULL) {
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports ......................................................................
Patch Set 2: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@9 PS2, Line 9: ACS Please spell this out in the beginning (not in the third paragraph).
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@9 PS2, Line 9: Currently it is impossible to enable ACS with AGESA by setting the correct : bit for AmdInitMid phase. AGESA code path does not call the right function : that enables these functionalities. Can a comment be added to the code in question?
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... PS2, Line 781: 0xB0 E or B?
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... PS2, Line 793: /* Enable ACS capabilities straps including sub-items. From lspci it : * looks like these bits enable: Source Validation and Translation : * Blocking : */ Please use one of the listed styles from the coding style.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@20 PS2, Line 20: groups On what board?
Hello Kyösti Mälkki, Patrick Rudolph, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35313
to look at the new patch set (#3).
Change subject: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports ......................................................................
src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports
Currently it is impossible to enable ACS with AGESA by setting the correct bit for AmdInitMid phase. AGESA code path does not call the right function that enables these functionalities. Disabled ACS result in multiple PCIe devices to be assigned to the same IOMMU group. Without IOMMU group separation the devices cannot be passed through independently.
Enable Access Control Services and Advanced Error Reporting for PCI Express bridges in order to have PCIe devices in separate IOMMU groups for correct passthrough.
TEST=run dmesg on Debian Buster on PC Engines apu2 and check whether PCIe devices have separate groups
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I10a8eff0ba37196692f9db6519e498fe535ecd15 --- M src/northbridge/amd/pi/00730F01/northbridge.c 1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/35313/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: src/northbridge/amd/pi/00730F01/northbridge.c: enable ACS and AER for PCIe ports ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... PS2, Line 781: 0xB0
E or B?
0xE0 is an index register that provides access to GPP link core registers 0x0140XXXX and IO Link Strap Control register has an offset of 00B0 which assembles to 0x014000B0
Hello Kyösti Mälkki, Patrick Rudolph, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35313
to look at the new patch set (#4).
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
nb/amd/pi/00730F01: enable ACS and AER for PCIe ports
Enable Access Control Services and Advanced Error Reporting for PCI Express bridges in order to have PCIe devices in separate IOMMU groups for correct passthrough.
TEST=run dmesg on Debian Buster on PC Engines apu2 and check whether PCIe devices have separate groups
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I10a8eff0ba37196692f9db6519e498fe535ecd15 --- M src/northbridge/amd/pi/00730F01/northbridge.c 1 file changed, 29 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/35313/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@9 PS2, Line 9: ACS
Please spell this out in the beginning (not in the third paragraph).
Done
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@9 PS2, Line 9: Currently it is impossible to enable ACS with AGESA by setting the correct : bit for AmdInitMid phase. AGESA code path does not call the right function : that enables these functionalities.
Can a comment be added to the code in question?
Done
https://review.coreboot.org/c/coreboot/+/35313/2//COMMIT_MSG@20 PS2, Line 20: groups
On what board?
Done
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... PS2, Line 793: /* Enable ACS capabilities straps including sub-items. From lspci it : * looks like these bits enable: Source Validation and Translation : * Blocking : */
Please use one of the listed styles from the coding style.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 7: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/35313/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35313/7//COMMIT_MSG@14 PS7, Line 14: devices have separate groups Can you give an example line from before and after?
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/2/src/northbridge/amd/pi/0073... PS2, Line 781: 0xB0
0xE0 is an index register that provides access to GPP link core registers 0x0140XXXX and IO Link Str […]
Done
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 784: result results
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 794: value |= (BIT(5) | BIT(6)); If you add macros for these to, you could remove the comment here.
value |= (PI_…_ENABLE_AER | PI_…_ENABLE_ACS)
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 784: result
results
ACS is Access Control Services, which is plural. Shouldn't it stay `result`?
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 794: value |= (BIT(5) | BIT(6));
If you add macros for these to, you could remove the comment here. […]
AMD BKDG doesn't describe these bits, so I don't know which one is which. This is a 50/50 gamble.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 7:
(1 comment)
IOMMU groups assignment and PCIe capabilities: - before: https://pastebin.com/ZfWWzbJW - after: https://pastebin.com/Ns0uH31z
https://review.coreboot.org/c/coreboot/+/35313/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35313/7//COMMIT_MSG@14 PS7, Line 14: devices have separate groups
Can you give an example line from before and after?
Too many differences to show as proof. Pasted as a message
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 784: result
ACS is Access Control Services, which is plural. […]
You are right. Sorry.
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 794: value |= (BIT(5) | BIT(6));
AMD BKDG doesn't describe these bits, so I don't know which one is which. This is a 50/50 gamble.
But you write `AER (bit 5)` explicitly. What am I missing?
Hello Angel Pons, Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35313
to look at the new patch set (#8).
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
nb/amd/pi/00730F01: enable ACS and AER for PCIe ports
Enable Access Control Services and Advanced Error Reporting for PCI Express bridges in order to have PCIe devices in separate IOMMU groups for correct passthrough.
TEST=run dmesg on Debian Buster on PC Engines apu2 and check whether PCIe devices have separate groups
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I10a8eff0ba37196692f9db6519e498fe535ecd15 --- M src/northbridge/amd/pi/00730F01/northbridge.c 1 file changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/35313/8
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 794: value |= (BIT(5) | BIT(6));
But you write `AER (bit 5)` explicitly. […]
I missed the comment... Looked at BKDG that had these bits undocumented and prematurely answered without noticing my comment... Some of BKDgs have at least AER documented.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/35313/7/src/northbridge/amd/pi/0073... PS7, Line 794: value |= (BIT(5) | BIT(6));
I missed the comment... […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
nb/amd/pi/00730F01: enable ACS and AER for PCIe ports
Enable Access Control Services and Advanced Error Reporting for PCI Express bridges in order to have PCIe devices in separate IOMMU groups for correct passthrough.
TEST=run dmesg on Debian Buster on PC Engines apu2 and check whether PCIe devices have separate groups
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I10a8eff0ba37196692f9db6519e498fe535ecd15 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35313 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/amd/pi/00730F01/northbridge.c 1 file changed, 31 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/amd/pi/00730F01/northbridge.c b/src/northbridge/amd/pi/00730F01/northbridge.c index e5a75e8..cf4d78b 100644 --- a/src/northbridge/amd/pi/00730F01/northbridge.c +++ b/src/northbridge/amd/pi/00730F01/northbridge.c @@ -37,6 +37,8 @@ #include <northbridge/amd/agesa/agesa_helper.h>
#define MAX_NODE_NUMS MAX_NODES +#define PCIE_CAP_AER BIT(5) +#define PCIE_CAP_ACS BIT(6)
typedef struct dram_base_mask { u32 base; //[47:27] at [28:8] @@ -777,6 +779,35 @@ pci_write_config32(dev, 0xF8, 0); pci_write_config32(dev, 0xFC, 5); /* TODO: move it to dsdt.asl */
+ /* + * Currently it is impossible to enable ACS with AGESA by setting the + * correct bit for AmdInitMid phase. AGESA code path does not call the + * right function that enables these functionalities. Disabled ACS + * result in multiple PCIe devices to be assigned to the same IOMMU + * group. Without IOMMU group separation the devices cannot be passed + * through independently. + */ + + /* Select GPP link core IO Link Strap Control register 0xB0 */ + pci_write_config32(dev, 0xE0, 0x014000B0); + value = pci_read_config32(dev, 0xE4); + + /* Enable AER (bit 5) and ACS (bit 6 undocumented) */ + value |= PCIE_CAP_AER | PCIE_CAP_ACS; + pci_write_config32(dev, 0xE4, value); + + /* Select GPP link core Wrapper register 0x00 (undocumented) */ + pci_write_config32(dev, 0xE0, 0x01300000); + value = pci_read_config32(dev, 0xE4); + + /* + * Enable ACS capabilities straps including sub-items. From lspci it + * looks like these bits enable: Source Validation and Translation + * Blocking + */ + value |= (BIT(24) | BIT(25) | BIT(26)); + pci_write_config32(dev, 0xE4, value); + /* disable No Snoop */ dev = pcidev_on_root(1, 1); if (dev != NULL) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35313 )
Change subject: nb/amd/pi/00730F01: enable ACS and AER for PCIe ports ......................................................................
Patch Set 9:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/661 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/660 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/659
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Michał Żygowski (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)