Re: [PATCH v5 06/12] xen/arm: irq: Restore state of local IRQs during system resume

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

On Sat, Aug 23, 2025 at 3:45 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > On ARM, the first 32 interrupts (SGIs and PPIs) are banked per-CPU
> > and not restored by gic_resume (for secondary cpus).
> >
> > This patch introduces restore_local_irqs_on_resume, a function that
> > restores the state of local interrupts on the target CPU during
> > system resume.
> >
> > It iterates over all local IRQs and re-enables those that were not
> > disabled, reprogramming their routing and affinity accordingly.
> >
> > The function is invoked from start_secondary, ensuring that local IRQ
> > state is restored early during CPU bring-up after suspend.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >  xen/arch/arm/irq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 148f184f8b..b3ff38dc27 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -113,6 +113,47 @@ static int init_local_irq_data(unsigned int cpu)
> >      return 0;
> >  }
> >
> > +/*
> > + * The first 32 interrupts (PPIs and SGIs) are per-CPU,
> > + * so call this function on the target CPU to restore them.
> > + *
> > + * SPIs are restored via gic_resume.
> > + */
> > +static void restore_local_irqs_on_resume(void)
> > +{
> > +    int irq;
> > +
> > +    if ( system_state != SYS_STATE_resume )
> > +        return;
>
> Maybe it is better to move this check to restore_local_irqs_on_resume()
> caller? You can put ASSERT(system_state == SYS_STATE_resume) there
> instead.
>
> I am saying this because name of restore_local_irqs_on_resume() implies that 
> it
> should be called only on resume.

Not a problem.
I'll move checking outside the function.

>
> > +
> > +    spin_lock(&local_irqs_type_lock);
> > +
> > +    for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ )
> > +    {
> > +        struct irq_desc *desc = irq_to_desc(irq);
> > +
> > +        spin_lock(&desc->lock);
> > +
> > +        if ( test_bit(_IRQ_DISABLED, &desc->status) )
> > +        {
> > +            spin_unlock(&desc->lock);
> > +            continue;
> > +        }
> > +
> > +        /* it is needed to avoid asserts in below calls */
> > +        set_bit(_IRQ_DISABLED, &desc->status);
>
> Assert in the call below isn't just because. You need to either call
> desc->handler->disable() to properly disable the IRQ or justify why it
> is fine to just set the bit.

Got it. I’ll call the disable handler here instead.

>
> > +
> > +        gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
> > +
> > +        /* _IRQ_DISABLED is cleared by below call */
> > +        desc->handler->startup(desc);
> > +
> > +        spin_unlock(&desc->lock);
> > +    }
> > +
> > +    spin_unlock(&local_irqs_type_lock);
> > +}
> > +
> >  static int cpu_callback(struct notifier_block *nfb, unsigned long action,
> >                          void *hcpu)
> >  {
> > @@ -131,6 +172,9 @@ static int cpu_callback(struct notifier_block *nfb, 
> > unsigned long action,
> >              printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n",
> >                     cpu);
> >          break;
> > +    case CPU_STARTING:
> > +        restore_local_irqs_on_resume();
> > +        break;
> >      }
> >
> >      return notifier_from_errno(rc);
>
> --
> WBR, Volodymyr

Best regards,
Mykola