Hello Jason Glenesk,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45056
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
MADT does not contain an entry for the second ioapic described in the IVRS.
Add second ioapic to MADT.
Cq-Depend: chrome-internal:3247427, chrome-internal: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.
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/include/soc/smu.h 2 files changed, 20 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/1
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 1b9c0ca..dc4a167 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,13 +46,29 @@ unsigned int i; uint8_t irq; uint8_t flags; + uint32_t nb_ioapic_base; + uint8_t ioapic_id;
/* create all subtables for processors */ current = acpi_create_madt_lapics(current);
- /* Write Kern IOAPIC, only one */ + /* Lapics use IDs 0 - max logical cpus - 1. FCH ioapic ID uses the next ID*/ + ioapic_id = CONFIG_MAX_CPUS; + + /* Write FCH IOAPIC */ current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, - CONFIG_MAX_CPUS, IO_APIC_ADDR, 0); + ioapic_id, 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); + + /* GNB uses the next ID*/ + ioapic_id++; + + /* Write GNB IOAPIC */ + current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, + ioapic_id, nb_ioapic_base, IO_APIC_INTERRUPTS);
/* 0: mean bus 0--->ISA */ /* 0: PIC 0 */ 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/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/1/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/1/src/soc/amd/picasso/acpi.c@... PS1, Line 57: trailing whitespace
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 1:
Please help to review this change.
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/+/45056
to look at the new patch set (#2).
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
soc/amd/picasso/acpi: Fix hang caused by IVRS
MADT does not contain an entry for the second ioapic described in the IVRS.
Add second ioapic to MADT.
Cq-Depend: chrome-internal:3247427, chrome-internal: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.
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/include/soc/smu.h 2 files changed, 20 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 55: 0 - max logical cpus - 1 This reads as 0 - max logical cpus. i.e., a negative number. Maybe better written as [0, max logical cpus - 1], or maybe [0, max logical cpus)
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 64: nb_ioapic_base If this is zero can we skip writing the nb ioapic? This way we don't write an invalid value if FSP is out of date.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG@7 PS2, Line 7: Fix hang caused by IVRS Also update the commit message to describe what the patch does.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG@21 PS2, Line 21: Signed-off-by you could ditch one of these lines.
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 52: Kern Thanks for removing.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Fix hang caused by IVRS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 64: nb_ioapic_base
If this is zero can we skip writing the nb ioapic? This way we don't write an invalid value if FSP i […]
you'll need to mask off some of the lower bits
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/+/45056
to look at the new patch set (#3).
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized.
MADT does not contain an entry for the second ioapic described in the IVRS. Check hob produced by FSP to detect if fsp has initialized second ioapic and use base address assigned to create MADT ioapic entry.
Cq-Depend: chrome-internal:3247427, chrome-internal: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.
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c M src/vendorcode/amd/fsp/picasso/FspGuids.h 2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/3/src/soc/amd/picasso/acpi.c@... PS3, Line 70: if (nb_ioapic_base->addr){ space required before the open brace '{'
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/+/45056
to look at the new patch set (#4).
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized.
MADT does not contain an entry for the second ioapic described in the IVRS. Check hob produced by FSP to detect if fsp has initialized second ioapic and use base address assigned to create MADT ioapic entry.
Cq-Depend: chrome-internal:3247427, chrome-internal: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.
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c M src/vendorcode/amd/fsp/picasso/FspGuids.h 2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/4
Jason Glenesk has uploaded a new patch set (#5) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized.
MADT does not contain an entry for the second ioapic described in the IVRS. Check hob produced by FSP to detect if fsp has initialized second ioapic and use base address assigned to create MADT ioapic entry.
Cq-Depend: chrome-internal:3247427, chrome-internal: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.
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c M src/vendorcode/amd/fsp/picasso/FspGuids.h 2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/5
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
Patch Set 5: Code-Review-1
Moving this ti WIP as more work needs to be done, but i have to step out.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c@... PS5, Line 59: CONFIG_MAX_CPUS I think we should do one of two things:
1. FSP has definies for io apic ids use here through fsp headers. 2. Add Kconfigs for the ids.
We shouldn't make these ids dependent on CONFIG_MAX_CPUS because we know things are being hard coded in FSP at the moment.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG@21 PS2, Line 21: Signed-off-by
you could ditch one of these lines.
Ack
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 55: 0 - max logical cpus - 1
This reads as 0 - max logical cpus. i.e., a negative number. […]
Ack
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 64: nb_ioapic_base
you'll need to mask off some of the lower bits
Ack
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c@... PS5, Line 59: CONFIG_MAX_CPUS
I think we should do one of two things: […]
Agree. We are planning a separate change to add a upd so coreboot can dictate what ids it wants to use.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/5//COMMIT_MSG@7 PS5, Line 7: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized. Please remove the dot/period at the end of the git commit message summary.
Marshall Dawson has uploaded a new patch set (#6) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Check for an FSP-produced HOB describing the address of the GNB IOAPIC. If found, add an entry to MADT describing it. FSP does not generate the HOB if the GNB IOAPIC is not enabled.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 29 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/6
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG@7 PS2, Line 7: Fix hang caused by IVRS
Also update the commit message to describe what the patch does.
Done
https://review.coreboot.org/c/coreboot/+/45056/2//COMMIT_MSG@21 PS2, Line 21: Signed-off-by
Ack
Done
https://review.coreboot.org/c/coreboot/+/45056/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45056/5//COMMIT_MSG@7 PS5, Line 7: soc/amd/picasso/acpi: Add MADT entry for NB ioapic, if initialized.
Please remove the dot/period at the end of the git commit message summary.
Done
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 55: 0 - max logical cpus - 1
Ack
Done
https://review.coreboot.org/c/coreboot/+/45056/2/src/soc/amd/picasso/acpi.c@... PS2, Line 64: nb_ioapic_base
Ack
Done
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/5/src/soc/amd/picasso/acpi.c@... PS5, Line 59: CONFIG_MAX_CPUS
Agree. […]
Added in CB:45115
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC What are the conditions under which the second IOAPIC won't be initialized?
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 54: nit: space not required.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: populates this wording: FSP generates this HOB
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 54: BIOS_ERR Is this an error or just informational? We don't "need" it right?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC
What are the conditions under which the second IOAPIC won't be initialized?
With the current patches in flight, it'll be enabled all the time. However I can't think of any reason to not enable it. I'd be fine with calling it a standard behavior of the FSP. Then we'll treat it more like an error if it's not found.
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 54:
nit: space not required.
I suspect it was meant to be indented, e.g.
ACPI: * MADT NB_IOAPIC_BASE HOB was not found.
Hmm, well some of our other messages aren't indented. I'd kind of prefer they be somewhat consistent. I'd suggest a follow-on change for formatting, unless there's a new PS for l.50.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC
With the current patches in flight, it'll be enabled all the time. […]
I ask because I am wondering if we should pass in the IOAPIC address for the NB from coreboot to FSP as a UPD instead of adding another HOB for it. That way you don't really need to check for the HOB in all places where it has to be used and instead rely on using the macro/Kconfig.
Also, probably we want to also declare it as reserved resource here: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/common/block/...
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 54:
I suspect it was meant to be indented, e.g. […]
I see. Sounds good.
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: populates this
wording: FSP generates this HOB
To Marshall's comment - is there any power implication by keeping it on all the time? I'm assuming not.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: populates this
To Marshall's comment - is there any power implication by keeping it on all the time? I'm assuming n […]
Eric, I have a question in re. power. I assume it's negligible.
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC I'd be fine with passing the address in and forcing FSP to obey the UPD. In that case, I'd probably take FSP's default back to disabled and make coreboot enable it intentionally.
Also, probably we want to also declare it as reserved resource here: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/common/block/...
Hmm, I thought we were already reserving a big chunk of space that should've covered it. Maybe I'm thinking of how Stoney Ridge though. You wouldn't put it in picasso/root_complex.c?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC
I'd be fine with passing the address in and forcing FSP to obey the UPD. In that case, I'd probably take FSP's default back to disabled and make coreboot enable it intentionally.
I think that would be fine. Just selecting an address and providing it to FSP will allow us to control the placement in coreboot.
Hmm, I thought we were already reserving a big chunk of space that should've covered it.
I don't think that's true. From coreboot logs, I see that 0x1000 bytes at IO_APIC_ADDR are marked as reserved: ``` update_constraints: PCI: 00:14.3 03 base fec00000 limit fec00fff mem (fixed) ```
And the space following that is available for MMIO allocation: ``` * Base: fec01000, Size: f000, Tag: 200 ```
So, depending upon what address is chosen we will have to update the reservations accordingly.
You wouldn't put it in picasso/root_complex.c?
Right now the IO_APIC_ADDR reservation is marked as part of lpc.c. But I see your point about root_complex.c since this IOAPIC is the one in NB. So, root_complex.c should be fine.
Marshall Dawson has uploaded a new patch set (#7) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/7
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: populates this
Eric, I have a question in re. power. I assume it's negligible.
Confirmed negligible.
Wording is gone now.
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 50: FSP populates this generates this HOB if it initializes second IOAPIC
I'd be fine with passing the address in and forcing FSP to obey the UPD. […]
Done. Passing address to FSP in CB:45115
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/7/src/soc/amd/picasso/acpi.c@... PS7, Line 52: /* Write FCH IOAPIC */ nit: Either this should say write FCH and GNB IOAPICs or drop the comment altogether?
Marshall Dawson has uploaded a new patch set (#8) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/8
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/7/src/soc/amd/picasso/acpi.c@... PS7, Line 52: /* Write FCH IOAPIC */
nit: Either this should say write FCH and GNB IOAPICs or drop the comment altogether?
Agree. Had meant to remove it.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 8: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 8: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 8: Code-Review+2
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/+/45056
to look at the new patch set (#9).
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/9
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 9: Code-Review+1
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/+/45056
to look at the new patch set (#10).
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry using new Kconfig symbol. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/10
Jason Glenesk has uploaded a new patch set (#11) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry using new Kconfig symbol for IOAPIC ID. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e --- M src/soc/amd/picasso/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45056/11
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 11: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 11: Code-Review+2
Ancestry was broken then restored. Commit message was increased, so reapplying +2 from earlier today.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 11: Code-Review+1
This will probably need a retrigger because Jenkins failed. Also, there's unresolved comments. Other than that, LGTM.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45056/6/src/soc/amd/picasso/acpi.c@... PS6, Line 54: BIOS_ERR
Is this an error or just informational? We don't "need" it right?
Ack
Marshall Dawson has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45056 )
Change subject: soc/amd/picasso: Add MADT entry for GNB IOAPIC ......................................................................
soc/amd/picasso: Add MADT entry for GNB IOAPIC
Add the missing entry using new Kconfig symbol for IOAPIC ID. coreboot will always enable the GNB IOAPIC.
Cq-Depend: chrome-internal:3247431, chrome-internal:3253044 BUG=b:167421913, 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. BRANCH=Zork
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Change-Id: Ic4a2e9b71dba948e8a4907e5f97131426d8a4a3e Reviewed-on: https://review.coreboot.org/c/coreboot/+/45056 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Marshall Dawson: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 84be4bd..6f6383d 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -49,10 +49,12 @@ /* create all subtables for processors */ current = acpi_create_madt_lapics(current);
- /* Write IOAPIC, only one */ current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, CONFIG_PICASSO_FCH_IOAPIC_ID, IO_APIC_ADDR, 0);
+ current += acpi_create_madt_ioapic((acpi_madt_ioapic_t *)current, + CONFIG_PICASSO_GNB_IOAPIC_ID, GNB_IO_APIC_ADDR, IO_APIC_INTERRUPTS); + /* 0: mean bus 0--->ISA */ /* 0: PIC 0 */ /* 2: APIC 2 */
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Eric Peers (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Jason Glenesk (Code Review)
-
Marshall Dawson (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)