Messages in this thread | | | Date | Wed, 25 Oct 2023 17:11:53 -0500 | Subject | Re: | From | Mario Limonciello <> |
| |
On 10/25/2023 16:04, Thomas Gleixner wrote: > David and a few others reported that on certain newer systems some legacy > interrupts fail to work correctly. > > Debugging revealed that the BIOS of these systems leaves the legacy PIC in > uninitialized state which makes the PIC detection fail and the kernel > switches to a dummy implementation. > > Unfortunately this fallback causes quite some code to fail as it depends on > checks for the number of legacy PIC interrupts or the availability of the > real PIC. > > In theory there is no reason to use the PIC on any modern system when > IO/APIC is available, but the dependencies on the related checks cannot be > resolved trivially and on short notice. This needs lots of analysis and > rework. > > The PIC detection has been added to avoid quirky checks and force selection > of the dummy implementation all over the place, especially in VM guest > scenarios. So it's not an option to revert the relevant commit as that > would break a lot of other scenarios. > > One solution would be to try to initialize the PIC on detection fail and > retry the detection, but that puts the burden on everything which does not > have a PIC. > > Fortunately the ACPI/MADT table header has a flag field, which advertises > in bit 0 that the system is PCAT compatible, which means it has a legacy > 8259 PIC. > > Evaluate that bit and if set avoid the detection routine and keep the real > PIC installed, which then gets initialized (for nothing) and makes the rest > of the code with all the dependencies work again. > > Fixes: e179f6914152 ("x86, irq, pic: Probe for legacy PIC and set legacy_pic appropriately") > Reported-by: David Lazar <dlazar@gmail.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Tested-by: David Lazar <dlazar@gmail.com> > Cc: stable@vger.kernel.org > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218003
s/Link/Closes/
Presumably you will add a proper subject when this is committed?
With adding title and fixing that tag:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> --- > --- > arch/x86/include/asm/i8259.h | 2 ++ > arch/x86/kernel/acpi/boot.c | 3 +++ > arch/x86/kernel/i8259.c | 38 ++++++++++++++++++++++++++++++-------- > 3 files changed, 35 insertions(+), 8 deletions(-) > > --- a/arch/x86/include/asm/i8259.h > +++ b/arch/x86/include/asm/i8259.h > @@ -69,6 +69,8 @@ struct legacy_pic { > void (*make_irq)(unsigned int irq); > }; > > +void legacy_pic_pcat_compat(void); > + > extern struct legacy_pic *legacy_pic; > extern struct legacy_pic null_legacy_pic; > > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -148,6 +148,9 @@ static int __init acpi_parse_madt(struct > pr_debug("Local APIC address 0x%08x\n", madt->address); > } > > + if (madt->flags & ACPI_MADT_PCAT_COMPAT) > + legacy_pic_pcat_compat(); > + > /* ACPI 6.3 and newer support the online capable bit. */ > if (acpi_gbl_FADT.header.revision > 6 || > (acpi_gbl_FADT.header.revision == 6 && > --- a/arch/x86/kernel/i8259.c > +++ b/arch/x86/kernel/i8259.c > @@ -32,6 +32,7 @@ > */ > static void init_8259A(int auto_eoi); > > +static bool pcat_compat __ro_after_init; > static int i8259A_auto_eoi; > DEFINE_RAW_SPINLOCK(i8259A_lock); > > @@ -299,15 +300,32 @@ static void unmask_8259A(void) > > static int probe_8259A(void) > { > + unsigned char new_val, probe_val = ~(1 << PIC_CASCADE_IR); > unsigned long flags; > - unsigned char probe_val = ~(1 << PIC_CASCADE_IR); > - unsigned char new_val; > + > + /* > + * If MADT has the PCAT_COMPAT flag set, then do not bother probing > + * for the PIC. Some BIOSes leave the PIC uninitialized and probing > + * fails. > + * > + * Right now this causes problems as quite some code depends on > + * nr_legacy_irqs() > 0 or has_legacy_pic() == true. This is silly > + * when the system has an IO/APIC because then PIC is not required > + * at all, except for really old machines where the timer interrupt > + * must be routed through the PIC. So just pretend that the PIC is > + * there and let legacy_pic->init() initialize it for nothing. > + * > + * Alternatively this could just try to initialize the PIC and > + * repeat the probe, but for cases where there is no PIC that's > + * just pointless. > + */ > + if (pcat_compat) > + return nr_legacy_irqs(); > + > /* > - * Check to see if we have a PIC. > - * Mask all except the cascade and read > - * back the value we just wrote. If we don't > - * have a PIC, we will read 0xff as opposed to the > - * value we wrote. > + * Check to see if we have a PIC. Mask all except the cascade and > + * read back the value we just wrote. If we don't have a PIC, we > + * will read 0xff as opposed to the value we wrote. > */ > raw_spin_lock_irqsave(&i8259A_lock, flags); > > @@ -429,5 +447,9 @@ static int __init i8259A_init_ops(void) > > return 0; > } > - > device_initcall(i8259A_init_ops); > + > +void __init legacy_pic_pcat_compat(void) > +{ > + pcat_compat = true; > +}
| |