x86/apic: Dead code in setup_local_APIC()

x86/apic: Dead code in setup_local_APIC()

[Andrew Cooper]

Hello,

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.

One option would be to sample LVT0.MASK before clearing SPIV.EN, but if
the intention is to allow ExtINT in symmetric-IO mode, then its
configuration shouldn't be based on its previous value.

Thoughts?

~Andrew

(I'm actually debugging why Xen can't find a timer IRQ on this platform,
but its not my system and I'm playing spot-the-difference with Linux
based on some photos of a boot log.  I don't think this difference is
relevant to my bug, but it also doesn't appear to be intentional on the
Linux side either.)

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