Re: [PATCH v2 07/13] x86/irq: Factor out calling ISR from common_interrupt

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Kevin,

On Fri, 12 Apr 2024 09:21:45 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Saturday, April 6, 2024 6:31 AM
> > 
> > Prepare for calling external IRQ handlers directly from the posted MSI
> > demultiplexing loop. Extract the common code with common interrupt to
> > avoid code duplication.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  arch/x86/kernel/irq.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> > index f39f6147104c..c54de9378943 100644
> > --- a/arch/x86/kernel/irq.c
> > +++ b/arch/x86/kernel/irq.c
> > @@ -242,18 +242,10 @@ static __always_inline void handle_irq(struct
> > irq_desc *desc,
> >  		__handle_irq(desc, regs);
> >  }
> > 
> > -/*
> > - * common_interrupt() handles all normal device IRQ's (the special SMP
> > - * cross-CPU interrupts have their own entry points).
> > - */
> > -DEFINE_IDTENTRY_IRQ(common_interrupt)
> > +static __always_inline void call_irq_handler(int vector, struct
> > pt_regs *regs) {
> > -	struct pt_regs *old_regs = set_irq_regs(regs);
> >  	struct irq_desc *desc;
> > 
> > -	/* entry code tells RCU that we're not quiescent.  Check it. */
> > -	RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up
> > RCU");
> > -
> >  	desc = __this_cpu_read(vector_irq[vector]);
> >  	if (likely(!IS_ERR_OR_NULL(desc))) {
> >  		handle_irq(desc, regs);  
> 
> the hidden lines has one problem:
> 
> 	} else {
> 		apic_eoi();
> 
> 		if (desc == VECTOR_UNUSED) {
> 			...
> 
> there will be two EOI's for unused vectors, adding the one
> in sysvec_posted_msi_notification().

Indeed this unlikely case could cause lost interrupt. Imagine we have:

- IDT vector N (MSI notification), O, and P (other high-priority
system vectors).
- Device MSI vector A which triggers N.

Action 			APIC IRR		APIC ISR
---------------------------------------------------------
Device MSI A		N
APIC accepts N		-			N
New IRQs arrive		O,P			N
handle_irq(A)
eoi() due to A's fault	-			O,P
eoi in post_msi		-			P
----------------------------------------------------------
The second EOI clears ISR for vector O but missed processing it.


Intel SDM 11.8.4 for background.
"The IRR contains the active interrupt requests that have been accepted,
but not yet dispatched to the processor for servicing. When the local APIC
accepts an interrupt, it sets the bit in the IRR that corresponds the
vector of the accepted interrupt. When the processor core is ready to
handle the next interrupt, the local APIC clears the highest priority IRR
bit that is set and sets the corresponding ISR bit. The vector for the
highest priority bit set in the ISR is then dispatched to the processor
core for servicing.

While the processor is servicing the highest priority interrupt, the local
APIC can send additional fixed interrupts by setting bits in the IRR. When
the interrupt service routine issues a write to the EOI register (see
Section 11.8.5, Signaling Interrupt Servicing Completion), the local APIC
responds by clearing the highest priority ISR bit that is set. It then
repeats the process of clearing the highest priority bit in the IRR and
setting the corresponding bit in the ISR. The processor core then begins
executing the service routing for the highest priority bit set in the ISR
"

I need to avoid the duplicated EOI in this case and at minimum cost for the
hot path.

Thanks,

Jacob