Re: [PATCH v2 7/7] x86/irq: forward pending interrupts to new destination in fixup_irqs()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jun 11, 2024 at 03:50:42PM +0200, Jan Beulich wrote:
> On 10.06.2024 16:20, Roger Pau Monne wrote:
> > fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
> > the CPU is to become offline, the normal migration logic used by Xen where the
> > vector in the previous target(s) is left configured until the interrupt is
> > received on the new destination is not suitable.
> > 
> > Instead attempt to do as much as possible in order to prevent loosing
> > interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
> > currently the case) attempt to forward pending vectors when interrupts that
> > target the current CPU are migrated to a different destination.
> > 
> > Additionally, for interrupts that have already been moved from the current CPU
> > prior to the call to fixup_irqs() but that haven't been delivered to the new
> > destination (iow: interrupts with move_in_progress set and the current CPU set
> > in ->arch.old_cpu_mask) also check whether the previous vector is pending and
> > forward it to the new destination.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - Rename to apic_irr_read().
> > ---
> >  xen/arch/x86/include/asm/apic.h |  5 +++++
> >  xen/arch/x86/irq.c              | 37 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
> > index d1cb001fb4ab..7bd66dc6e151 100644
> > --- a/xen/arch/x86/include/asm/apic.h
> > +++ b/xen/arch/x86/include/asm/apic.h
> > @@ -132,6 +132,11 @@ static inline bool apic_isr_read(uint8_t vector)
> >              (vector & 0x1f)) & 1;
> >  }
> >  
> > +static inline bool apic_irr_read(unsigned int vector)
> > +{
> > +    return apic_read(APIC_IRR + (vector / 32 * 0x10)) & (1U << (vector % 32));
> > +}
> > +
> >  static inline u32 get_apic_id(void)
> >  {
> >      u32 id = apic_read(APIC_ID);
> > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > index 54eabd23995c..ed262fb55f4a 100644
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -2601,7 +2601,7 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >  
> >      for ( irq = 0; irq < nr_irqs; irq++ )
> >      {
> > -        bool break_affinity = false, set_affinity = true;
> > +        bool break_affinity = false, set_affinity = true, check_irr = false;
> >          unsigned int vector, cpu = smp_processor_id();
> >          cpumask_t *affinity = this_cpu(scratch_cpumask);
> >  
> > @@ -2649,6 +2649,25 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >               !cpumask_test_cpu(cpu, &cpu_online_map) &&
> >               cpumask_test_cpu(cpu, desc->arch.old_cpu_mask) )
> >          {
> > +            /*
> > +             * This to be offlined CPU was the target of an interrupt that's
> > +             * been moved, and the new destination target hasn't yet
> > +             * acknowledged any interrupt from it.
> > +             *
> > +             * We know the interrupt is configured to target the new CPU at
> > +             * this point, so we can check IRR for any pending vectors and
> > +             * forward them to the new destination.
> > +             *
> > +             * Note the difference between move_in_progress or
> > +             * move_cleanup_count being set.  For the later we know the new
> > +             * destination has already acked at least one interrupt from this
> > +             * source, and hence there's no need to forward any stale
> > +             * interrupts.
> > +             */
> 
> I'm a little confused by this last paragraph: It talks about a difference,
> yet ...
> 
> > +            if ( apic_irr_read(desc->arch.old_vector) )
> > +                send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> > +                              desc->arch.vector);
> 
> ... in the code being commented there's no difference visible. Hmm, I guess
> this is related to the enclosing if(). Maybe this could be worded a little
> differently, e.g. starting with "Note that for the other case -
> move_cleanup_count being non-zero - we know ..."?

Hm, I see.  Yes, the difference is that for interrupts that have
move_cleanup_count set we don't forward pending interrupts in IRR on
this CPU.  I put this here because I think it's more naturally
arranged with the rest of the comment.  I can pull the whole comment
ahead if the if() if that's better.

> > @@ -2689,11 +2708,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >          if ( desc->handler->disable )
> >              desc->handler->disable(desc);
> >  
> > +        /*
> > +         * If the current CPU is going offline and is (one of) the target(s) of
> > +         * the interrupt signal to check whether there are any pending vectors
> > +         * to be handled in the local APIC after the interrupt has been moved.
> > +         */
> 
> After reading this a number of times, I think there wants to be a comma between
> "interrupt" and "signal". Or am I getting wrong what is being meant?

Indeed.

> > +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> > +            check_irr = true;
> > +
> >          if ( desc->handler->set_affinity )
> >              desc->handler->set_affinity(desc, affinity);
> >          else if ( !(warned++) )
> >              set_affinity = false;
> >  
> > +        if ( check_irr && apic_irr_read(vector) )
> > +            /*
> > +             * Forward pending interrupt to the new destination, this CPU is
> > +             * going offline and otherwise the interrupt would be lost.
> > +             */
> > +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> > +                          desc->arch.vector);
> > +
> >          if ( desc->handler->enable )
> >              desc->handler->enable(desc);
> >  
> 
> Down from here, after the loop, there's a 1ms window where latched but not
> yet delivered interrupts can be received. How's that playing together with
> the changes you're making? Aren't we then liable to get two interrupts, one
> at the old and one at the new source, in unknown order?

I was mistakenly thinking that clear_local_APIC() would block
interrupt delivery, but that's not the case, so yes, interrupts should
still be delivered in the window below.

Let me test without this last patch.

Thanks, Roger.