Hello Jason Glenesk,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45050
to review the following change.
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
Previous commit misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. Additionally, the MADT was missing an entry for the second ioapic described in the IVRS.
Remove 8-byte entry alignment and add second ioapic to MADT.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Dump MADT and IVRS tables. Cross check ioapic entries in MADT against IVRS. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c M src/soc/amd/picasso/include/soc/smu.h 3 files changed, 12 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/1
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 1b9c0ca..1aacf5d 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -26,6 +26,7 @@ #include <soc/gpio.h> #include <version.h> #include "chip.h" +#include <soc/smu.h>
unsigned long acpi_fill_mcfg(unsigned long current) { @@ -45,14 +46,23 @@ unsigned int i; uint8_t irq; uint8_t flags; + uint32_t nb_ioapic_base;
/* create all subtables for processors */ current = acpi_create_madt_lapics(current);
- /* Write Kern IOAPIC, only one */ + /* Write FCH IOAPIC */ current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, CONFIG_MAX_CPUS, IO_APIC_ADDR, 0);
+ /* Read base address assigned to gnb ioapic */ + pci_write_config32(SOC_GNB_DEV, SMU_INDEX_ADDR, REG_NB_IOAPIC_BASE); + nb_ioapic_base = pci_read_config32(SOC_GNB_DEV, SMU_DATA_ADDR); + + /* Write GNB IOAPIC */ + current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, + CONFIG_MAX_CPUS + 1, nb_ioapic_base, 32); + /* 0: mean bus 0--->ISA */ /* 0: PIC 0 */ /* 2: APIC 2 */ diff --git a/src/soc/amd/picasso/agesa_acpi.c b/src/soc/amd/picasso/agesa_acpi.c index a651d6e..c76e943 100644 --- a/src/soc/amd/picasso/agesa_acpi.c +++ b/src/soc/amd/picasso/agesa_acpi.c @@ -47,8 +47,6 @@
unsigned long acpi_fill_ivrs_ioapic(acpi_ivrs_t *ivrs, unsigned long current) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_special_t *ivhd_ioapic = (ivrs_ivhd_special_t *)current; memset(ivhd_ioapic, 0, sizeof(*ivhd_ioapic));
@@ -75,8 +73,6 @@
static unsigned long ivhd_describe_hpet(unsigned long current) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_special_t *ivhd_hpet = (ivrs_ivhd_special_t *)current;
ivhd_hpet->type = IVHD_DEV_8_BYTE_EXT_SPECIAL_DEV; @@ -93,8 +89,6 @@ static unsigned long ivhd_describe_f0_device(unsigned long current, uint16_t dev_id, uint8_t datasetting) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_f0_entry_t *ivhd_f0 = (ivrs_ivhd_f0_entry_t *) current;
ivhd_f0->type = IVHD_DEV_VARIABLE; @@ -154,8 +148,6 @@ ivhd_entry->dte_setting = data; *current += sizeof(ivrs_ivhd_generic_t); } else if (type == IVHD_DEV_8_BYTE_ALIAS_SELECT) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - *current = ALIGN_UP(*current, 8); ivrs_ivhd_alias_t *ivhd_entry = (ivrs_ivhd_alias_t *)*current;
ivhd_entry->type = type; diff --git a/src/soc/amd/picasso/include/soc/smu.h b/src/soc/amd/picasso/include/soc/smu.h index 128f4c4..bc9f4f3 100644 --- a/src/soc/amd/picasso/include/soc/smu.h +++ b/src/soc/amd/picasso/include/soc/smu.h @@ -12,6 +12,7 @@ #define REG_ADDR_MESG_ID 0x3b10528 #define REG_ADDR_MESG_RESP 0x3b10564 #define REG_ADDR_MESG_ARGS_BASE 0x0b10998 +#define REG_NB_IOAPIC_BASE 0x13b102f0
/* Argument 0-5 indexed locations are contiguous */ #define SMU_NUM_ARGS 6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45050/1/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45050/1/src/soc/amd/picasso/acpi.c@... PS1, Line 65: trailing whitespace
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 1:
Please help to review this change.
Jason Glenesk has uploaded a new patch set (#2) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
Previous commit misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. Additionally, the MADT was missing an entry for the second ioapic described in the IVRS.
Remove 8-byte entry alignment and add second ioapic to MADT.
Cq-Depend: chrome-internal:3247427, 3247427 BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Dump MADT and IVRS tables. Cross check ioapic entries in MADT against IVRS. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c M src/soc/amd/picasso/include/soc/smu.h 3 files changed, 12 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/2
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45050
to look at the new patch set (#3).
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
Previous commit misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. Additionally, the MADT was missing an entry for the second ioapic described in the IVRS.
Remove 8-byte entry alignment and add second ioapic to MADT.
Cq-Depend: chrome-internal:3247427, 3247427 BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Dump MADT and IVRS tables. Cross check ioapic entries in MADT against IVRS. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c M src/soc/amd/picasso/include/soc/smu.h 3 files changed, 12 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c@... PS3, Line 64: CONFIG_MAX_CPUS + 1, nb_ioapic_base, 32); You should probably use a local variable called apic_id or something and assign it CONFIG_MAX_CPUS + 1 w/ a comment indicating the reasoning.
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c@... PS3, Line 64: 32 Is it always 32? Should we have some defines for how many IRQs are supported in each apic?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45050/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45050/3//COMMIT_MSG@16 PS3, Line 16: 3247427 Think you need a chrome-internal: here as well.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 3:
I'm going to split the MADT change and the IVRS change apart.
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45050
to look at the new patch set (#4).
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
Previous commit misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/agesa_acpi.c M src/soc/amd/picasso/include/soc/smu.h 2 files changed, 1 insertion(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/4
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45050
to look at the new patch set (#5).
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
Previous commit misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/agesa_acpi.c 1 file changed, 0 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 5: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 5: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG@9 PS5, Line 9: Previous commit Please reference the commit by hash and commit message summary.
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG@9 PS5, Line 9: Previous commit misinterpreted spec as requiring size alignment on all : IVHD device entries. Please summarize the issue this causes (hang where?).
Jason Glenesk has uploaded a new patch set (#6) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment.
Previous commit (1916f8969b10e27fe06b3e0eb1caae632bd947f6) misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. The unneeded realignments result in gaps in the table. The kernel hangs in early boot due to the malformed table.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/agesa_acpi.c 1 file changed, 0 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/6
Jason Glenesk has uploaded a new patch set (#7) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment.
Previous CL (1916f8969b10e27fe06b3e0eb1caae632bd947f6) misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. The unneeded realignments result in gaps in the table. The kernel hangs in early boot due to the malformed table.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/agesa_acpi.c 1 file changed, 0 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
Patch Set 7:
comments need to be resolved
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
Patch Set 7:
(4 comments)
Patch Set 7:
comments need to be resolved
https://review.coreboot.org/c/coreboot/+/45050/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45050/3//COMMIT_MSG@16 PS3, Line 16: 3247427
Think you need a chrome-internal: here as well.
Ack
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG@9 PS5, Line 9: Previous commit
Please reference the commit by hash and commit message summary.
Ack
https://review.coreboot.org/c/coreboot/+/45050/5//COMMIT_MSG@9 PS5, Line 9: Previous commit misinterpreted spec as requiring size alignment on all : IVHD device entries.
Please summarize the issue this causes (hang where?).
Ack
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c@... PS3, Line 64: 32
Is it always 32? Should we have some defines for how many IRQs are supported in each apic?
Will use the defs for ioapic irq number in the MADT part i split off. That one requires a bit more work inside FSP.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45050/3/src/soc/amd/picasso/acpi.c@... PS3, Line 64: CONFIG_MAX_CPUS + 1, nb_ioapic_base, 32);
You should probably use a local variable called apic_id or something and assign it CONFIG_MAX_CPUS + […]
Ack
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45050
to look at the new patch set (#8).
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment.
Previous CL (1916f8969b10e27fe06b3e0eb1caae632bd947f6) misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. The unneeded realignments result in gaps in the table. The kernel hangs in early boot due to the malformed table.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/agesa_acpi.c 1 file changed, 0 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/45050/8
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45050 )
Change subject: soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment. ......................................................................
soc/amd/picasso/acpi: Remove padding in IVRS table caused by realignment.
Previous CL (1916f8969b10e27fe06b3e0eb1caae632bd947f6) misinterpreted spec as requiring size alignment on all IVHD device entries. The correct requirement specifies only for 4-byte entries. The unneeded realignments result in gaps in the table. The kernel hangs in early boot due to the malformed table.
Remove 8-byte entry alignment.
BUG=b:166519072 TEST=Boot fully to morphius board with and without amd_iommu kernel parameter. Confirm IVRS contains no alignment gaps/corruption.
Change-Id: Iddcff98279be1d910936b13391dd2448a3bb2d74 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45050 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/soc/amd/picasso/agesa_acpi.c 1 file changed, 0 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/agesa_acpi.c b/src/soc/amd/picasso/agesa_acpi.c index a651d6e..c76e943 100644 --- a/src/soc/amd/picasso/agesa_acpi.c +++ b/src/soc/amd/picasso/agesa_acpi.c @@ -47,8 +47,6 @@
unsigned long acpi_fill_ivrs_ioapic(acpi_ivrs_t *ivrs, unsigned long current) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_special_t *ivhd_ioapic = (ivrs_ivhd_special_t *)current; memset(ivhd_ioapic, 0, sizeof(*ivhd_ioapic));
@@ -75,8 +73,6 @@
static unsigned long ivhd_describe_hpet(unsigned long current) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_special_t *ivhd_hpet = (ivrs_ivhd_special_t *)current;
ivhd_hpet->type = IVHD_DEV_8_BYTE_EXT_SPECIAL_DEV; @@ -93,8 +89,6 @@ static unsigned long ivhd_describe_f0_device(unsigned long current, uint16_t dev_id, uint8_t datasetting) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - current = ALIGN_UP(current, 8); ivrs_ivhd_f0_entry_t *ivhd_f0 = (ivrs_ivhd_f0_entry_t *) current;
ivhd_f0->type = IVHD_DEV_VARIABLE; @@ -154,8 +148,6 @@ ivhd_entry->dte_setting = data; *current += sizeof(ivrs_ivhd_generic_t); } else if (type == IVHD_DEV_8_BYTE_ALIAS_SELECT) { - /* 8-byte IVHD structures must be aligned to the 8-byte boundary. */ - *current = ALIGN_UP(*current, 8); ivrs_ivhd_alias_t *ivhd_entry = (ivrs_ivhd_alias_t *)*current;
ivhd_entry->type = type;
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Jason Glenesk (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)