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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Tue, Sep 2, 2025 at 7:49 PM Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
>
>
>
> On 02.09.25 01:10, Mykola Kvach wrote:
>
> Hello Mykola
>
> > 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>
> > ---
> > Changes in V6:
> > - Call handler->disable() instead of just setting the _IRQ_DISABLED flag
> > - Move the system state check outside of restore_local_irqs_on_resume()
> > ---
> >   xen/arch/arm/irq.c | 39 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 39 insertions(+)
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 6c899347ca..ddd2940554 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -116,6 +116,41 @@ 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;
>
> NIT: Please, use "unsigned int" if irq cannot be negative
>
> > +
> > +    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;
> > +        }
> > +
> > +        /* Disable the IRQ to avoid assertions in the following calls */
> > +        desc->handler->disable(desc);
> > +        gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
>
> Shouldn't we use GIC_PRI_IPI for SGIs?

I'll update the priority value in the next version.

Initially, I assumed gic_route_irq_to_xen() was used for all
interrupts with the same priority. But looking more closely, it
doesn't appear to be called for SGIs at all.

In fact, SGI configuration, including priority, is handled during CPU
initialization in gic_init_secondary_cpu(), which is called before
the CPU_STARTING notifier.

Given that, it's probably better to avoid updating SGI priorities here
entirely and rely on their boot-time configuration instead.

>
>
> > +        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)
> >   {
> > @@ -134,6 +169,10 @@ 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:
> > +        if ( system_state == SYS_STATE_resume )
> > +            restore_local_irqs_on_resume();
> > +        break;
>
> May I please ask, why all this new code (i.e.
> restore_local_irqs_on_resume()) is not covered by #ifdef
> CONFIG_SYSTEM_SUSPEND?
>
> >       }
> >
> >       return notifier_from_errno(rc);
>