Apu2 iommu fix #592
Open
Apu2 iommu fix #592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Michał Żygowski <[email protected]>
…r IOMMU Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Michał Żygowski <[email protected]>
Move the GNB IOAPIC enabling to northbridge init where the IOAPIC is initialized. Remove the comment about IoapicSbFeatureEn, this bit is not touched here. Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Michał Żygowski <[email protected]>
… register Signed-off-by: Michał Żygowski <[email protected]>
This was referenced Nov 23, 2024
miczyg1
commented
Nov 23, 2024
10 tasks
Introduce PCI_DEVID macro which allows to construct PCI device addresses or range with bus numbers. Signed-off-by: Michał Żygowski <[email protected]>
miczyg1
force-pushed
the
apu2_iommu_fix
branch
2 times, most recently
from
2b58e23 to
aed43dc
Compare
Signed-off-by: Michał Żygowski <[email protected]>
…e area Use the IORESOURCE_SOFT_RESERVE attribute to reserve CC6 save state DRAM. Using regualr reserved memory makes it hard on EDK2 to detect if it is MMIO or reserved DRAM, as TOM2 is equal to the base of the CC6 save state area, not its limit. Signed-off-by: Michał Żygowski <[email protected]>
Signed-off-by: Michał Żygowski <[email protected]>
Per family 16h models 30h-3fh BKDG the IoapicSbFeatureEn must be configured according to the interrupt routing mode selecte by OS. If OS chose APIC mode, the IoapicSbFeatureEn must be cleared. Otherwise, it must be set, meaning PIC mode is used. Add a hook to _PIC method to call SoC/northbridge specific code to set/clear the bit to configure GNB IOAPIC properly. ACPI specification says that _PIC method is optional and can be called by OSPM to provide the interrupt routing mode information to the firmware. However, if the method is not called, the firmware must assume PIC mode is used. AGESA sets the IoapicSbFeatureEn already to be compliant with ACPI. Previously, coreboot cleared the bit unconditionally and left a comment to move that part to DSDT. The hook allows to clear the IoapicSbFeatureEn bit if OS chooses APIC mode for interrupt routing. Signed-off-by: Michał Żygowski <[email protected]>
miczyg1
force-pushed
the
apu2_iommu_fix
branch
from
aed43dc to
e4edeca
Compare
|
I get a panic from Xen when booting with coreboot+EDK2 that mentions IOMMU and thought that this PR might fix it as well, but it doesn't. Any idea if this is even related? Also wondering whether @krystian-hebel has seen this panic. Xen boot log
P.S. This is MultiBoot2 boot but EFI boot behaves the same at least when booting from GRUB. |
Looks like it's not. I don't get the error on |
It may be because of these lines being removed: The IoapicSbFeatureEn controls the routing of masked GNB IOAPIC interrupts to SB IOAPIC. When the software intends to use PIC mode for interrupt routing, this bit should remain set. When IOAPIC mode is to be used, then it should be cleared. ACPI specification requires the pre-OS environment (i.e. firmware) to assume PIC mode as default if I have talked with @andyhhp about it and he said that Xen does not talk ACPI and simply assumes IOAPIC mode always. This is not compliant with the ACPI specification, however he suggested to make it compliant with the spec. That of course probably causes the panic you see @SergiiDmytruk . If all Xen versions behave like that, then we will have to make it an runtime option? maybe @andyhhp can say more. |
|
@miczyg1, you didn't break anything! The issue happens without this PR as well, I just thought the PR could fix it. The problem must be somewhere in Xen staging because https://github.com/TrenchBoot/xen/tree/aem_intel_fixes_v0.4.1 boots fine in EFI mode. |
|
This is Xen falling over a NULL pointer while configuring an IO-APIC pin. @SergiiDmytruk Can you try reverting https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=86001b3970fea4536048607ea6e12541736c48e1 ? or at least put in a NULL pointer check for |
|
@andyhhp, reverting xen/arch/x86/io_apic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d44d2c9a41..509b032502 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -304,7 +304,7 @@ void __ioapic_write_entry(
* something different than the CPU vector, and hence need to be cached
* for performing EOI.
*/
- if ( io_apic_pin_eoi )
+ if ( io_apic_pin_eoi && io_apic_pin_eoi[apic] )
io_apic_pin_eoi[apic][pin] = e.vector;
}
elseThanks! |
|
@SergiiDmytruk Thanks for confirming. I'll take this report over to Xen and we'll try to figure out what to do. |
|
@SergiiDmytruk Could you provide a boot log of the crash without any out of tree patches applied to Xen? So that the base commit is unmodified from Xen upstream repository. |
|
@royger, here is a boot log for Xen at https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=58ba55959ae1bca0651396d0752c2076a45b5ee6 Boot log
|
|
Could you provide the boot log with the following instrumentation patch applied? diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d9db2efc4f58..2b4df4388ad6 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -304,6 +304,7 @@ void __ioapic_write_entry(
* something different than the CPU vector, and hence need to be cached
* for performing EOI.
*/
+ printk("IOAPIC%u pin %u\n", apic, pin);
if ( io_apic_pin_eoi )
io_apic_pin_eoi[apic][pin] = e.vector;
}
@@ -1373,6 +1374,7 @@ void __init enable_IO_APIC(void)
if ( io_apic_pin_eoi )
{
+ printk("IOAPIC%u EOI alloc nr: %u\n", apic, nr_ioapic_entries[apic]);
io_apic_pin_eoi[apic] = xvmalloc_array(typeof(**io_apic_pin_eoi),
nr_ioapic_entries[apic]);
BUG_ON(!io_apic_pin_eoi[apic]); |
|
Log with those debug prints: Details
|
|
Huh, so we really do allocate only one EOI bitmap, but there are two IO-APICs |
|
Can you try the following patch: diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d9db2efc4f58..0614e71c0667 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1389,14 +1389,15 @@ void __init enable_IO_APIC(void)
/* If the interrupt line is enabled and in ExtInt mode
* I have found the pin where the i8259 is connected.
*/
- if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT)) {
+ if ( entry.mask == 0 && entry.delivery_mode == dest_ExtINT &&
+ ioapic_i8259.apic == -1 )
+ {
+ ASSERT(ioapic_i8259.pin == -1);
ioapic_i8259.apic = apic;
ioapic_i8259.pin = pin;
- goto found_i8259;
}
}
}
- found_i8259:
/* Look to see what if the MP table has reported the ExtINT */
/* If we could not find the appropriate pin by looking at the ioapic
* the i8259 probably is not connected the ioapic but give the
|
|
Added that patch and Xen booted. In case you want to see the log (keeping debug prints made it rather long and GitHub has 64KiB limit): xen.log |
|
Thanks for testing, I would like to add |
No problem, the bug was getting in the way of my work. Yes, feel free to add Reported-by. |
|
Fixed in Xen staging as of this morning. https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f38fd27c4ceadf7ec4e82e82d0731b6ea415c51e |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.