The current logic to init the local APIC and the IO-APIC does init the
local APIC LVTERR/ESR before doing any sanitation on the IO-APIC pin
configuration.  It's already noted on enable_IO_APIC() that Xen
shouldn't trust the IO-APIC being empty at bootup.

At XenServer we have a system where the IO-APIC 0 is handed to Xen
with pin 0 unmasked, set to Fixed delivery mode, edge triggered and
with a vector of 0 (all fields of the RTE are zeroed).  Once the local
APIC LVTERR/ESR is enabled periodic injections from such pin cause the
local APIC to in turn inject periodic error vectors:

APIC error on CPU0: 00(40), Received illegal vector
APIC error on CPU0: 40(40), Received illegal vector
APIC error on CPU0: 40(40), Received illegal vector
APIC error on CPU0: 40(40), Received illegal vector
APIC error on CPU0: 40(40), Received illegal vector
APIC error on CPU0: 40(40), Received illegal vector

That prevents Xen from booting.

Move the masking of the IO-APIC pins ahead of the setup of the local
APIC.  This has the side effect of also moving the detection of the
pin where the i8259 is connected, as such detection must be done
before masking any pins.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Remove the smp_found_config from APIC_init_uniprocessor().
 - Adjust commit message.
 - Move the IO-APIC sanitize call just ahead of setup_local_APIC().
 - Keep the enable_IO_APIC() function name.
---
 xen/arch/x86/apic.c            | 4 ++++
 xen/arch/x86/include/asm/irq.h | 1 +
 xen/arch/x86/io_apic.c         | 4 +---
 xen/arch/x86/smpboot.c         | 5 +++++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index f71474d47dd1..cf1d012841e6 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1491,6 +1491,10 @@ int __init APIC_init_uniprocessor (void)
     physids_clear(phys_cpu_present_map);
     physid_set(boot_cpu_physical_apicid, phys_cpu_present_map);
 
+    if ( !skip_ioapic_setup && nr_ioapics )
+        /* Sanitize the IO-APIC pins before enabling the lapic LVTERR/ESR. */
+        enable_IO_APIC();
+
     setup_local_APIC(true);
 
     if (nmi_watchdog == NMI_LOCAL_APIC)
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 424b0e1af8f4..3f95dd39b7b9 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -118,6 +118,7 @@ bool bogus_8259A_irq(unsigned int irq);
 int i8259A_suspend(void);
 int i8259A_resume(void);
 
+void enable_IO_APIC(void);
 void setup_IO_APIC(void);
 void disable_IO_APIC(void);
 void setup_ioapic_dest(void);
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 9b8a972cf570..25a08b1ea6c6 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1273,7 +1273,7 @@ static void cf_check _print_IO_APIC_keyhandler(unsigned char key)
     __print_IO_APIC(0);
 }
 
-static void __init enable_IO_APIC(void)
+void __init enable_IO_APIC(void)
 {
     int i8259_apic, i8259_pin;
     int i, apic;
@@ -2067,8 +2067,6 @@ static void __init ioapic_pm_state_alloc(void)
 
 void __init setup_IO_APIC(void)
 {
-    enable_IO_APIC();
-
     if (acpi_ioapic)
         io_apic_irqs = ~0;	/* all IRQs go through IOAPIC */
     else
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index cf9bb220f90d..3a1a659082c6 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1224,6 +1224,11 @@ void __init smp_prepare_cpus(void)
     verify_local_APIC();
 
     connect_bsp_APIC();
+
+    if ( !skip_ioapic_setup && nr_ioapics )
+        /* Sanitize the IO-APIC pins before enabling the lapic LVTERR/ESR. */
+        enable_IO_APIC();
+
     setup_local_APIC(true);
 
     if ( !skip_ioapic_setup && nr_ioapics )
-- 
2.41.0

On 14.07.2023 18:03, Roger Pau Monne wrote:
> The current logic to init the local APIC and the IO-APIC does init the
> local APIC LVTERR/ESR before doing any sanitation on the IO-APIC pin

Nit: I guess I'll take the liberty of making this read "sanitization"
while committing.

> configuration.  It's already noted on enable_IO_APIC() that Xen
> shouldn't trust the IO-APIC being empty at bootup.
> 
> At XenServer we have a system where the IO-APIC 0 is handed to Xen
> with pin 0 unmasked, set to Fixed delivery mode, edge triggered and
> with a vector of 0 (all fields of the RTE are zeroed).  Once the local
> APIC LVTERR/ESR is enabled periodic injections from such pin cause the
> local APIC to in turn inject periodic error vectors:
> 
> APIC error on CPU0: 00(40), Received illegal vector
> APIC error on CPU0: 40(40), Received illegal vector
> APIC error on CPU0: 40(40), Received illegal vector
> APIC error on CPU0: 40(40), Received illegal vector
> APIC error on CPU0: 40(40), Received illegal vector
> APIC error on CPU0: 40(40), Received illegal vector
> 
> That prevents Xen from booting.
> 
> Move the masking of the IO-APIC pins ahead of the setup of the local
> APIC.  This has the side effect of also moving the detection of the
> pin where the i8259 is connected, as such detection must be done
> before masking any pins.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>