Re: [PATCH RFC 1/1] x86/irq/i8259: Ack PIC vectors in the LAPIC ISR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/irq/i8259: unconditional APIC access in apic_eoi_8259A() crashes on boot
--

commit da80aa20e6beda6754faf006bd9626316c6d5e1d
Author: Dongli Zhang <dongli.zhang@oracle.com>

x86/irq/i8259: Ack PIC vectors in the LAPIC ISR

This commit adds apic_eoi_8259A() to check the local APIC ISR bit for the
handled ISA IRQ vector and issue an APIC EOI if set, covering both normal
and auto-EOI 8259A modes to prevent LAPIC ISR stalls.

> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
> index f7a86b94a0dd5..73939edb8c2b3 100644
> --- a/arch/x86/kernel/i8259.c
> +++ b/arch/x86/kernel/i8259.c
> @@ -144,6 +144,16 @@ static inline int i8259A_irq_real(unsigned int irq)
>  	return value;
>  }
>  
> +static void apic_eoi_8259A(unsigned int irq)
> +{
> +	unsigned int vector = ISA_IRQ_VECTOR(irq);
> +	u32 isr;
> +
> +	isr = apic_read(APIC_ISR + APIC_VECTOR_TO_REG_OFFSET(vector));
> +	if (isr & BIT(APIC_VECTOR_TO_BIT_NUMBER(vector)))
> +		apic_eoi();
> +}

[Severity: High]
Is it safe for apic_eoi_8259A() in arch/x86/kernel/i8259.c to call
apic_read() and apic_eoi() unconditionally on every 8259A PIC interrupt
without verifying that the local APIC is enabled, mapped, and initialized?

There appear to be several boot paths where an 8259A interrupt (such as
IRQ0 unmasked in hpet_time_init() -> setup_default_timer_irq(), or a
delayed kexec/kdump interrupt) can fire while apic_read() points to an
unmapped fixmap address, an invalid MSR, or an unallocated per-CPU pointer:

1. Booting with nolapic (or acpi_mps_check() disabling the LAPIC) when
x2APIC was pre-enabled by firmware or left enabled in MSR_IA32_APICBASE by
the crashing kernel before kexec/kdump:

setup_arch() -> apic_setup_apic_calls() initializes the APIC read callback
to apic_physflat.read (native_apic_mem_read), which dereferences
APIC_BASE (fix_to_virt(FIX_APIC_BASE)).

setup_nolapic() sets apic_is_disabled = true and clears X86_FEATURE_APIC,
but does not clear X86_FEATURE_X2APIC. check_x2apic() in
arch/x86/kernel/apic/apic.c then sees x2apic_enabled() and sets
x2apic_mode = 1:

arch/x86/kernel/apic/apic.c:check_x2apic() {
	if (x2apic_enabled()) {
		pr_info("x2apic: enabled by BIOS, switching to x2apic ops\n");
		x2apic_mode = 1;

Because X86_FEATURE_APIC is clear, acpi_parse_madt() in
arch/x86/kernel/acpi/boot.c returns -EINVAL before
default_acpi_madt_oem_check() can switch the APIC driver from apic_physflat
to an x2APIC driver:

arch/x86/kernel/acpi/boot.c:acpi_parse_madt() {
	if (!boot_cpu_has(X86_FEATURE_APIC))
		return -EINVAL;

Later, both register_lapic_address() and init_apic_mappings() in
arch/x86/kernel/apic/apic.c check x2apic_mode, skipping both
apic_set_fixmap() and apic_disable() (which would otherwise install
apic_noop):

arch/x86/kernel/apic/apic.c:init_apic_mappings() {
	if (x2apic_mode)
		return;

	if (!smp_found_config) {
		if (!detect_init_APIC()) {
			pr_info("APIC: disable apic facility\n");
			apic_disable();
		}
	}
}

apic_intr_mode_init() in arch/x86/kernel/apic/apic.c then returns early on
APIC_PIC without calling x86_64_probe_apic(). On the first 8259A interrupt,
apic_eoi_8259A() calls native_apic_mem_read(), causing an unhandled page
fault on the unmapped FIX_APIC_BASE address.

2. Early boot with pre-enabled x2APIC before x86_64_probe_apic() runs:

When x2APIC is pre-enabled and ACPI MADT is disabled or absent (such as
with acpi=off or acpi=noirq), apic_call_read remains native_apic_mem_read
with FIX_APIC_BASE unmapped until x86_64_probe_apic() runs inside
apic_intr_mode_init(). However, start_kernel() calls local_irq_enable() and
x86_late_time_init() -> hpet_time_init() (which unmasks PIC IRQ0) before
apic_intr_mode_init(), so a PIC interrupt in this window will page fault on
APIC_BASE.

Conversely, if BIOS pre-enabled x2APIC is disabled in
enable_IR_x2apic() -> try_to_enable_x2apic() -> x2apic_disable() (for
example, when interrupt remapping is unavailable or disabled),
enable_IR_x2apic() in arch/x86/kernel/apic/apic.c unmasks the PIC and
restores interrupts before x86_64_probe_apic() switches the driver back
from x2APIC MSR ops to apic_physflat:

arch/x86/kernel/apic/apic.c:enable_IR_x2apic() {
	try_to_enable_x2apic(ir_stat);

	if (ir_stat < 0)
		restore_ioapic_entries();
	legacy_pic->restore_mask();
	local_irq_restore(flags);
}

Any pending PIC interrupt firing upon local_irq_restore(flags) will invoke
native_apic_msr_read() while x2APIC is disabled in MSR_IA32_APICBASE,
triggering a #GP fault on rdmsr.

3. Hyper-V with nolapic in xAPIC mode:

init_hypervisor_platform() -> hyperv_init() -> hv_apic_init() registers
hv_apic_read in __x86_apic_override.read. With nolapic, FIX_APIC_BASE is
not mapped during MADT parsing, and when init_apic_mappings() calls
apic_disable() -> apic_install_driver(&apic_noop) ->
restore_override_callbacks(), apic_noop.read is overwritten with
hv_apic_read(). For APIC_ISR, hv_apic_read() falls back to
native_apic_mem_read(), faulting on the unmapped APIC_BASE address on the
first PIC interrupt.

4. AMD SEV-SNP Secure AVIC before savic_probe():

During early_acpi_boot_init(), default_acpi_madt_oem_check() installs
apic_x2apic_savic, whose read callback is savic_read() in
arch/x86/kernel/apic/x2apic_savic.c:

arch/x86/kernel/apic/x2apic_savic.c:savic_read() {
	void *ap = this_cpu_ptr(savic_page);

savic_page is not allocated until x86_late_time_init() ->
apic_intr_mode_init() -> x86_64_probe_apic() -> savic_probe(). A PIC
interrupt arriving after local_irq_enable() / hpet_time_init() and before
savic_probe() will evaluate this_cpu_ptr(savic_page) with savic_page == NULL
and read from unrelated per-CPU memory.

5. Disabled LAPIC with an MP table present or on 32-bit UP systems:

When mpparse_find_mptable() sets smp_found_config = 1 while the LAPIC is
disabled (via nolapic, acpi_mps_check(), or hardware-disabled APIC),
init_apic_mappings() skips detect_init_APIC() and apic_disable(), leaving
native_apic_mem_read active. apic_eoi_8259A() will then perform MMIO reads
from disabled LAPIC physical space (and if 0xffffffff is returned, issue
spurious MMIO EOI writes via apic_eoi()) on every PIC interrupt. In
addition, on 32-bit UP systems where detect_init_APIC() jumps to no_apic
and installs apic_noop while X86_FEATURE_APIC is still set and
apic_is_disabled is false, apic_eoi_8259A() will trigger the WARN_ON_ONCE()
in noop_apic_read().

Should apic_eoi_8259A() guard the APIC access to ensure the local APIC is
actually enabled and initialized before calling apic_read()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029063112.203893-1-dongli.zhang@oracle.com?part=1