Re: x86/apic: Dead code in setup_local_APIC()

[Maciej W. Rozycki]

On Fri, 13 Mar 2020, Andrew Cooper wrote:

> c/s 2640da4ccc "x86/apic: Soft disable APIC before initializing it" had
> a (perhaps unintended) consequence for the setup of LVT0.
> 
> Later, LVT0's mask bit is sampled to determine whether the BSP should be
> configured to accept ExtINT messages.
> 
> Because soft reset unconditionally masks the LVT registers, the
> following patch could be taken to drop dead code:
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 5f973fed3c9f..b80032d2dfeb 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1723,8 +1723,7 @@ static void setup_local_APIC(void)
> ÂÂÂÂÂÂÂ /*
> ÂÂÂÂÂÂÂÂ * TODO: set up through-local-APIC from through-I/O-APIC? --macro
> ÂÂÂÂÂÂÂÂ */
> -ÂÂÂÂÂÂ value = apic_read(APIC_LVT0) & APIC_LVT_MASKED;
> -ÂÂÂÂÂÂ if (!cpu && (pic_mode || !value || skip_ioapic_setup)) {
> +ÂÂÂÂÂÂ if (!cpu && (pic_mode || skip_ioapic_setup)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ value = APIC_DM_EXTINT;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ apic_printk(APIC_VERBOSE, "enabled ExtINT on CPU#%d\n",
> cpu);
> ÂÂÂÂÂÂÂ } else {
> 
> 
> However, the comment just out of context above says that ExtINT is
> deliberately configured even symmetric-IO mode, in case some interrupts
> are using the PIC. If that is the intended behaviour, then 2640da4ccc
> regressed it.

 FYI, it's been a very long while since I last poke at this code, however 
the context along with my comment indicates the LVT0 mask check is there 
so as to handle virtual-wire setups correctly whether the ExtINT has been 
routed via the local or the I/O APIC.  In the latter case LVT0 will have 
been masked by the bootstrap firmware.

 This only matters for systems that do not wire the 8254 PIT IRQ natively 
(nominally to I/O APIC #0, input #2) and a suitable ExtINT interrupt has 
to be retained for the PIT IRQ to work.  Different hardware has the 8259A 
combo wired to either or both I/O APIC #0, input #0 and local APIC, input 
#0.  If only a single route is available, it has to be used according to 
how the bootstrap firmware has set the hardware up.

 As I understand it this will only matter for older systems that comply 
with the MPS rather than ACPI.  It may be difficult to verify with such a 
system these days (I do retain such a live specimen, but it's clean on the 
hardware side in that it has all the possible IRQ routings concerned here 
implemented, so all this hackery does not really matter and it can use the 
native 8254 PIT IRQ routing).  Whether we need the PIT nowadays in the 
first place is another matter.

 HTH,

  Maciej