[PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Ashok Raj]

When offlining CPU's, fixup_irqs() migrates all interrupts away from the
outgoing CPU to an online CPU. Its always possible the device sent an
interrupt to the previous CPU destination. Pending interrupt bit in IRR in
lapic identifies such interrupts. apic_soft_disable() will not capture any
new interrupts in IRR. This causes interrupts from device to be lost during
cpu offline. The issue was found when explicitly setting MSI affinity to a
CPU and immediately offlining it. It was simple to recreate with a USB
ethernet device and doing I/O to it while the CPU is offlined. Lost
interrupts happen even when Interrupt Remapping is enabled.

Current code does apic_soft_disable() before migrating interrupts.

native_cpu_disable()
{
	...
	apic_soft_disable();
	cpu_disable_common();
	  --> fixup_irqs(); // Too late to capture anything in IRR.
}

Just fliping the above call sequence seems to hit the IRR checks
and the lost interrupt is fixed for both legacy MSI and when
interrupt remapping is enabled.


Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
Signed-off-by: Ashok Raj <ashok.raj@intel.com>

To: linux-kernel@vger.kernel.org
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
Cc: Evan Green <evgreen@chromium.org>
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/smpboot.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ffbd9a3d78d8..278cc9f92f2f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
 	if (ret)
 		return ret;
 
+	cpu_disable_common();
 	/*
 	 * Disable the local APIC. Otherwise IPI broadcasts will reach
 	 * it. It still responds normally to INIT, NMI, SMI, and SIPI
-	 * messages.
+	 * messages. Its important to do apic_soft_disable() after
+	 * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
+	 * depends on IRR being set. After apic_soft_disable() CPU preserves
+	 * currently set IRR/ISR but new interrupts will not set IRR.
+	 * This causes interrupts sent to outgoing cpu before completion
+	 * of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
+	 * APIC State after It Has been Software Disabled" section for more
+	 * details.
 	 */
 	apic_soft_disable();
-	cpu_disable_common();
 
 	return 0;
 }
-- 
2.13.6

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Randy Dunlap]

On 8/14/20 2:38 PM, Ashok Raj wrote:
> When offlining CPU's, fixup_irqs() migrates all interrupts away from the

                 CPUs,

> outgoing CPU to an online CPU. Its always possible the device sent an

                                 It's

> interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> lapic identifies such interrupts. apic_soft_disable() will not capture any

  LAPIC

> new interrupts in IRR. This causes interrupts from device to be lost during
> cpu offline. The issue was found when explicitly setting MSI affinity to a

  CPU

> CPU and immediately offlining it. It was simple to recreate with a USB
> ethernet device and doing I/O to it while the CPU is offlined. Lost
> interrupts happen even when Interrupt Remapping is enabled.
> 
> Current code does apic_soft_disable() before migrating interrupts.
> 
> native_cpu_disable()
> {
> 	...
> 	apic_soft_disable();
> 	cpu_disable_common();
> 	  --> fixup_irqs(); // Too late to capture anything in IRR.
> }
> 
> Just fliping the above call sequence seems to hit the IRR checks

       flipping

> and the lost interrupt is fixed for both legacy MSI and when
> interrupt remapping is enabled.
> 
> 
> Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> 
> To: linux-kernel@vger.kernel.org
> To: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/smpboot.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index ffbd9a3d78d8..278cc9f92f2f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
>  	if (ret)
>  		return ret;
>  
> +	cpu_disable_common();
>  	/*
>  	 * Disable the local APIC. Otherwise IPI broadcasts will reach
>  	 * it. It still responds normally to INIT, NMI, SMI, and SIPI
> -	 * messages.
> +	 * messages. Its important to do apic_soft_disable() after

	             It's

> +	 * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
> +	 * depends on IRR being set. After apic_soft_disable() CPU preserves
> +	 * currently set IRR/ISR but new interrupts will not set IRR.
> +	 * This causes interrupts sent to outgoing cpu before completion

	                                           CPU

> +	 * of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local

	      IRQ

> +	 * APIC State after It Has been Software Disabled" section for more
> +	 * details.
>  	 */
>  	apic_soft_disable();
> -	cpu_disable_common();
>  
>  	return 0;
>  }
> 

thanks.
-- 
~Randy

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Raj, Ashok]

Hi Randy,


On Fri, Aug 14, 2020 at 04:25:32PM -0700, Randy Dunlap wrote:
> On 8/14/20 2:38 PM, Ashok Raj wrote:
> > When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> 
>                  CPUs,

Thanks for catching these. I'll fix all these suggested changes in my next rev
Once i get additional feedback from Thomas.


> 
> > outgoing CPU to an online CPU. Its always possible the device sent an
> 
>                                  It's
> 
> > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> > lapic identifies such interrupts. apic_soft_disable() will not capture any
> 
>   LAPIC
> 
> > new interrupts in IRR. This causes interrupts from device to be lost during
> > cpu offline. The issue was found when explicitly setting MSI affinity to a
> 
>   CPU
> 
> > CPU and immediately offlining it. It was simple to recreate with a USB
> > ethernet device and doing I/O to it while the CPU is offlined. Lost
> > interrupts happen even when Interrupt Remapping is enabled.
> > 
> > Current code does apic_soft_disable() before migrating interrupts.
> > 
> > native_cpu_disable()
> > {
> > 	...
> > 	apic_soft_disable();
> > 	cpu_disable_common();
> > 	  --> fixup_irqs(); // Too late to capture anything in IRR.
> > }
> > 
> > Just fliping the above call sequence seems to hit the IRR checks
> 
>        flipping
> 
> > and the lost interrupt is fixed for both legacy MSI and when
> > interrupt remapping is enabled.
> > 
> > 
> > Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> > Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > 
> > To: linux-kernel@vger.kernel.org
> > To: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> > Cc: Evan Green <evgreen@chromium.org>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/x86/kernel/smpboot.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index ffbd9a3d78d8..278cc9f92f2f 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> >  	if (ret)
> >  		return ret;
> >  
> > +	cpu_disable_common();
> >  	/*
> >  	 * Disable the local APIC. Otherwise IPI broadcasts will reach
> >  	 * it. It still responds normally to INIT, NMI, SMI, and SIPI
> > -	 * messages.
> > +	 * messages. Its important to do apic_soft_disable() after
> 
> 	             It's
> 
> > +	 * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
> > +	 * depends on IRR being set. After apic_soft_disable() CPU preserves
> > +	 * currently set IRR/ISR but new interrupts will not set IRR.
> > +	 * This causes interrupts sent to outgoing cpu before completion
> 
> 	                                           CPU
> 
> > +	 * of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
> 
> 	      IRQ
> 
> > +	 * APIC State after It Has been Software Disabled" section for more
> > +	 * details.
> >  	 */
> >  	apic_soft_disable();
> > -	cpu_disable_common();
> >  
> >  	return 0;
> >  }
> > 
> 
> thanks.
> -- 
> ~Randy
> 

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Raj, Ashok]

Hi Randy

For some unknown reason my previous response said its taiting to be 
delivered. 

On Fri, Aug 14, 2020 at 04:25:32PM -0700, Randy Dunlap wrote:
> On 8/14/20 2:38 PM, Ashok Raj wrote:
> > When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> 
>                  CPUs,

I'll fix all these in the next rev. 

Just waiting to hear back from Thomas if he has additional ones I can fix
and resend v2.

Cheers,
Ashok
> 
> > outgoing CPU to an online CPU. Its always possible the device sent an
> 
>                                  It's
> 
> > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> > lapic identifies such interrupts. apic_soft_disable() will not capture any
> 
>   LAPIC
> 
> > new interrupts in IRR. This causes interrupts from device to be lost during
> > cpu offline. The issue was found when explicitly setting MSI affinity to a
> 
>   CPU
> 
> > CPU and immediately offlining it. It was simple to recreate with a USB
> > ethernet device and doing I/O to it while the CPU is offlined. Lost
> > interrupts happen even when Interrupt Remapping is enabled.
> > 
> > Current code does apic_soft_disable() before migrating interrupts.
> > 
> > native_cpu_disable()
> > {
> > 	...
> > 	apic_soft_disable();
> > 	cpu_disable_common();
> > 	  --> fixup_irqs(); // Too late to capture anything in IRR.
> > }
> > 
> > Just fliping the above call sequence seems to hit the IRR checks
> 
>        flipping
> 
> > and the lost interrupt is fixed for both legacy MSI and when
> > interrupt remapping is enabled.
> > 
> > 
> > Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> > Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > 
> > To: linux-kernel@vger.kernel.org
> > To: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> > Cc: Evan Green <evgreen@chromium.org>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/x86/kernel/smpboot.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index ffbd9a3d78d8..278cc9f92f2f 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> >  	if (ret)
> >  		return ret;
> >  
> > +	cpu_disable_common();
> >  	/*
> >  	 * Disable the local APIC. Otherwise IPI broadcasts will reach
> >  	 * it. It still responds normally to INIT, NMI, SMI, and SIPI
> > -	 * messages.
> > +	 * messages. Its important to do apic_soft_disable() after
> 
> 	             It's
> 
> > +	 * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
> > +	 * depends on IRR being set. After apic_soft_disable() CPU preserves
> > +	 * currently set IRR/ISR but new interrupts will not set IRR.
> > +	 * This causes interrupts sent to outgoing cpu before completion
> 
> 	                                           CPU
> 
> > +	 * of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
> 
> 	      IRQ
> 
> > +	 * APIC State after It Has been Software Disabled" section for more
> > +	 * details.
> >  	 */
> >  	apic_soft_disable();
> > -	cpu_disable_common();
> >  
> >  	return 0;
> >  }
> > 
> 
> thanks.
> -- 
> ~Randy
> 

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Evan Green]

Hi Ashok,
Thank you, Srikanth, and Sukumar for some very impressive debugging here.

On Fri, Aug 14, 2020 at 2:38 PM Ashok Raj <ashok.raj@intel.com> wrote:
>
> When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> outgoing CPU to an online CPU. Its always possible the device sent an
> interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> lapic identifies such interrupts. apic_soft_disable() will not capture any
> new interrupts in IRR. This causes interrupts from device to be lost during
> cpu offline. The issue was found when explicitly setting MSI affinity to a
> CPU and immediately offlining it. It was simple to recreate with a USB
> ethernet device and doing I/O to it while the CPU is offlined. Lost
> interrupts happen even when Interrupt Remapping is enabled.
>
> Current code does apic_soft_disable() before migrating interrupts.
>
> native_cpu_disable()
> {
>         ...
>         apic_soft_disable();
>         cpu_disable_common();
>           --> fixup_irqs(); // Too late to capture anything in IRR.
> }
>
> Just fliping the above call sequence seems to hit the IRR checks
> and the lost interrupt is fixed for both legacy MSI and when
> interrupt remapping is enabled.
>
>
> Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>
> To: linux-kernel@vger.kernel.org
> To: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/smpboot.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index ffbd9a3d78d8..278cc9f92f2f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
>         if (ret)
>                 return ret;
>
> +       cpu_disable_common();
>         /*
>          * Disable the local APIC. Otherwise IPI broadcasts will reach
>          * it. It still responds normally to INIT, NMI, SMI, and SIPI
> -        * messages.

I'm slightly unclear about whether interrupts are disabled at the core
by this point or not. I followed native_cpu_disable() up to
__cpu_disable(), up to take_cpu_down(). This is passed into a call to
stop_machine_cpuslocked(), where interrupts get disabled at the core.
So unless there's another path, it seems like interrupts are always
disabled at the core by this point.

If interrupts are always disabled, then the comment above is a little
obsolete, since we're not expecting to receive broadcast IPIs from
here on out anyway. We could clean up that comment in this change.

If there is a path to here where interrupts are still enabled at the
core, then we'd need to watch out, because this change now allows
broadcast IPIs to come in during cpu_disable_common(). That could be
bad. But I think that's not this case, so this should be ok.

> +        * messages. Its important to do apic_soft_disable() after
> +        * fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
> +        * depends on IRR being set. After apic_soft_disable() CPU preserves
> +        * currently set IRR/ISR but new interrupts will not set IRR.
> +        * This causes interrupts sent to outgoing cpu before completion
> +        * of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
> +        * APIC State after It Has been Software Disabled" section for more
> +        * details.
>          */
>         apic_soft_disable();
> -       cpu_disable_common();
>
>         return 0;
>  }
> --
> 2.13.6
>

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Raj, Ashok]

Hi Evan

Some details below, 

On Mon, Aug 17, 2020 at 11:12:17AM -0700, Evan Green wrote:
> Hi Ashok,
> Thank you, Srikanth, and Sukumar for some very impressive debugging here.
> 
> On Fri, Aug 14, 2020 at 2:38 PM Ashok Raj <ashok.raj@intel.com> wrote:
> >
> > When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> > outgoing CPU to an online CPU. Its always possible the device sent an
> > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> > lapic identifies such interrupts. apic_soft_disable() will not capture any
> > new interrupts in IRR. This causes interrupts from device to be lost during
> > cpu offline. The issue was found when explicitly setting MSI affinity to a
> > CPU and immediately offlining it. It was simple to recreate with a USB
> > ethernet device and doing I/O to it while the CPU is offlined. Lost
> > interrupts happen even when Interrupt Remapping is enabled.
> >
> > Current code does apic_soft_disable() before migrating interrupts.
> >
> > native_cpu_disable()
> > {
> >         ...
> >         apic_soft_disable();
> >         cpu_disable_common();
> >           --> fixup_irqs(); // Too late to capture anything in IRR.
> > }
> >
> > Just fliping the above call sequence seems to hit the IRR checks
> > and the lost interrupt is fixed for both legacy MSI and when
> > interrupt remapping is enabled.
> >
> >
> > Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> > Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> >
> > To: linux-kernel@vger.kernel.org
> > To: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> > Cc: Evan Green <evgreen@chromium.org>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/x86/kernel/smpboot.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index ffbd9a3d78d8..278cc9f92f2f 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> >         if (ret)
> >                 return ret;
> >
> > +       cpu_disable_common();
> >         /*
> >          * Disable the local APIC. Otherwise IPI broadcasts will reach
> >          * it. It still responds normally to INIT, NMI, SMI, and SIPI
> > -        * messages.
> 
> I'm slightly unclear about whether interrupts are disabled at the core
> by this point or not. I followed native_cpu_disable() up to
> __cpu_disable(), up to take_cpu_down(). This is passed into a call to
> stop_machine_cpuslocked(), where interrupts get disabled at the core.
> So unless there's another path, it seems like interrupts are always
> disabled at the core by this point.

local_irq_disable() just does cli() which allows interrupts to trickle
in to the IRR bits, and once you do sti() things would flow back for
normal interrupt processing. 


> 
> If interrupts are always disabled, then the comment above is a little

Disable interrupts is different from disabling LAPIC. Once you do the
apic_soft_disable(), there is nothing flowing into the LAPIC except
for INIT, NMI, SMI and SIPI messages. 

This turns off the pipe for all other interrupts to enter LAPIC. Which
is different from doing a cli().


> obsolete, since we're not expecting to receive broadcast IPIs from
> here on out anyway. We could clean up that comment in this change.
> 
> If there is a path to here where interrupts are still enabled at the
> core, then we'd need to watch out, because this change now allows
> broadcast IPIs to come in during cpu_disable_common(). That could be
> bad. But I think that's not this case, so this should be ok.

Section SDM Vol3.b 10.4.7.2 says.

* The reception of any interrupt or transmission of any IPIs that are in 
  progress when the local APIC is disabled are completed before the local 
  APIC enters the software-disabled state.

It doesn't actually say much about broadcast IPI's, except broadcast 
NMI for instance, which is still permitted when cli() is set.

Hope this helps.

Cheers,
Ashok

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Evan Green]

On Mon, Aug 17, 2020 at 11:33 AM Raj, Ashok <ashok.raj@intel.com> wrote:
>
> Hi Evan
>
> Some details below,
>
> On Mon, Aug 17, 2020 at 11:12:17AM -0700, Evan Green wrote:
> > Hi Ashok,
> > Thank you, Srikanth, and Sukumar for some very impressive debugging here.
> >
> > On Fri, Aug 14, 2020 at 2:38 PM Ashok Raj <ashok.raj@intel.com> wrote:
> > >
> > > When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> > > outgoing CPU to an online CPU. Its always possible the device sent an
> > > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> > > lapic identifies such interrupts. apic_soft_disable() will not capture any
> > > new interrupts in IRR. This causes interrupts from device to be lost during
> > > cpu offline. The issue was found when explicitly setting MSI affinity to a
> > > CPU and immediately offlining it. It was simple to recreate with a USB
> > > ethernet device and doing I/O to it while the CPU is offlined. Lost
> > > interrupts happen even when Interrupt Remapping is enabled.
> > >
> > > Current code does apic_soft_disable() before migrating interrupts.
> > >
> > > native_cpu_disable()
> > > {
> > >         ...
> > >         apic_soft_disable();
> > >         cpu_disable_common();
> > >           --> fixup_irqs(); // Too late to capture anything in IRR.
> > > }
> > >
> > > Just fliping the above call sequence seems to hit the IRR checks
> > > and the lost interrupt is fixed for both legacy MSI and when
> > > interrupt remapping is enabled.
> > >
> > >
> > > Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> > > Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> > > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > >
> > > To: linux-kernel@vger.kernel.org
> > > To: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > > Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> > > Cc: Evan Green <evgreen@chromium.org>
> > > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  arch/x86/kernel/smpboot.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > > index ffbd9a3d78d8..278cc9f92f2f 100644
> > > --- a/arch/x86/kernel/smpboot.c
> > > +++ b/arch/x86/kernel/smpboot.c
> > > @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       cpu_disable_common();
> > >         /*
> > >          * Disable the local APIC. Otherwise IPI broadcasts will reach
> > >          * it. It still responds normally to INIT, NMI, SMI, and SIPI
> > > -        * messages.
> >
> > I'm slightly unclear about whether interrupts are disabled at the core
> > by this point or not. I followed native_cpu_disable() up to
> > __cpu_disable(), up to take_cpu_down(). This is passed into a call to
> > stop_machine_cpuslocked(), where interrupts get disabled at the core.
> > So unless there's another path, it seems like interrupts are always
> > disabled at the core by this point.
>
> local_irq_disable() just does cli() which allows interrupts to trickle
> in to the IRR bits, and once you do sti() things would flow back for
> normal interrupt processing.
>
>
> >
> > If interrupts are always disabled, then the comment above is a little
>
> Disable interrupts is different from disabling LAPIC. Once you do the
> apic_soft_disable(), there is nothing flowing into the LAPIC except
> for INIT, NMI, SMI and SIPI messages.
>
> This turns off the pipe for all other interrupts to enter LAPIC. Which
> is different from doing a cli().

I understand the distinction. I was mostly musing on the difference in
behavior your change causes if this function is entered with
interrupts enabled at the core (ie sti()). But I think it never is, so
that thought is moot.

I could never repro the issue reliably on comet lake after Thomas'
original fix. But I can still repro it easily on jasper lake. And this
patch fixes the issue for me on that platform. Thanks for the fix.

Reviewed-by: Evan Green <evgreen@chromium.org>
Tested-by: Evan Green <evgreen@chromium.org>

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Raj, Ashok]

On Thu, Aug 20, 2020 at 11:21:24AM -0700, Evan Green wrote:
> > >
> > > I'm slightly unclear about whether interrupts are disabled at the core
> > > by this point or not. I followed native_cpu_disable() up to
> > > __cpu_disable(), up to take_cpu_down(). This is passed into a call to
> > > stop_machine_cpuslocked(), where interrupts get disabled at the core.
> > > So unless there's another path, it seems like interrupts are always
> > > disabled at the core by this point.
> >
> > local_irq_disable() just does cli() which allows interrupts to trickle
> > in to the IRR bits, and once you do sti() things would flow back for
> > normal interrupt processing.
> >
> >
> > >
> > > If interrupts are always disabled, then the comment above is a little
> >
> > Disable interrupts is different from disabling LAPIC. Once you do the
> > apic_soft_disable(), there is nothing flowing into the LAPIC except
> > for INIT, NMI, SMI and SIPI messages.
> >
> > This turns off the pipe for all other interrupts to enter LAPIC. Which
> > is different from doing a cli().
> 
> I understand the distinction. I was mostly musing on the difference in
> behavior your change causes if this function is entered with
> interrupts enabled at the core (ie sti()). But I think it never is, so
> that thought is moot.
> 
> I could never repro the issue reliably on comet lake after Thomas'
> original fix. But I can still repro it easily on jasper lake. And this
> patch fixes the issue for me on that platform. Thanks for the fix.
> 
> Reviewed-by: Evan Green <evgreen@chromium.org>
> Tested-by: Evan Green <evgreen@chromium.org>

Thanks Evan for testing. I'll wait for thomas if he finds anything else
that needs to be fixed and send a final v2 after fixing the typos and
others identified by Randy. 

Cheers,
Ashok

Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated

[Mathias Nyman]

On 15.8.2020 0.38, Ashok Raj wrote:
> When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> outgoing CPU to an online CPU. Its always possible the device sent an
> interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> lapic identifies such interrupts. apic_soft_disable() will not capture any
> new interrupts in IRR. This causes interrupts from device to be lost during
> cpu offline. The issue was found when explicitly setting MSI affinity to a
> CPU and immediately offlining it. It was simple to recreate with a USB
> ethernet device and doing I/O to it while the CPU is offlined. Lost
> interrupts happen even when Interrupt Remapping is enabled.
> 
> Current code does apic_soft_disable() before migrating interrupts.
> 
> native_cpu_disable()
> {
> 	...
> 	apic_soft_disable();
> 	cpu_disable_common();
> 	  --> fixup_irqs(); // Too late to capture anything in IRR.
> }
> 
> Just fliping the above call sequence seems to hit the IRR checks
> and the lost interrupt is fixed for both legacy MSI and when
> interrupt remapping is enabled.
> 
> 
> Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> Link: https://lore.kernel.org/lkml/875zdarr4h.fsf@nanos.tec.linutronix.de/
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> 
> To: linux-kernel@vger.kernel.org
> To: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sukumar Ghorai <sukumar.ghorai@intel.com>
> Cc: Srikanth Nandamuri <srikanth.nandamuri@intel.com>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable@vger.kernel.org
> ---

This fixes the lost xhci interrupt for me.
Before this patch a msi interupt was lost after ~200 cycles of toggling CPUs
offline/online under heavy usb traffic.

With this patch I ran 3x2000 cycles without any issues (Comet lake, patch on top of 5.8)
Tried both with and without CONFIG_IRQ_REMAP. No issues seen.

Tested-by: Mathias Nyman <mathias.nyman@linux.intel.com>