Re: [PATCH] x86/ioapic: sanitize IO-APIC pins before enabling the local APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2023 11:53, Roger Pau Monne wrote:
> The current logic to init the local APIC and the IO-APIC does init the
> former first before doing any kind of 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 is enabled periodic injections from such pin cause a storm of
> errors:
> 
> 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.

And I expect no RTE is in ExtInt mode, so one might conclude that
firmware meant to set up RTE 0 that way. Mainly as a remark, albeit
of course there's then the question whether to change the RTE
rather than masking it. What do ACPI tables say?

> Fix this by moving the masking of IO-APIC pins ahead of the enabling
> of the local APIC.  Note that before doing such masking Xen attempts
> to detect the pin where the legacy i8259 is connected, and that logic
> relies on the pin being unmasked, hence the logic is also moved ahead
> of enabling the local APIC.

A comma after "masking" might help readers here.

> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1476,6 +1476,10 @@ int __init APIC_init_uniprocessor (void)
>          return -1;
>      }
>  
> +    if ( smp_found_config && !skip_ioapic_setup && nr_ioapics )
> +        /* Sanitize the IO-APIC pins before enabling the local APIC. */
> +        sanitize_IO_APIC();

I'm a little puzzled by the smp_found_config part of the check here,
but not in smp_prepare_cpus(). What's the reason for (a) the check
and (b) the difference? Isn't checking nr_ioapics sufficient in this
regard? (b) probably could be addressed by moving the code to the
beginning of verify_local_APIC(), immediately ahead of which you
insert both call sites. (At which point the function may want naming
verify_IO_APIC() for consistency, but that's surely minor.)

As to also checking skip_ioapic_setup - wouldn't the unmasked pin be
similarly troublesome in that case? Aiui it would not hurt only if
the LAPIC also isn't (going to be) set up. Then again the flag,
among other things, signals a zero base address found in the ACPI or
MP tables, so I guess this is a (largely) separate issue anyway.

Jan