Hi Oleksandr,
On Tue, Sep 2, 2025 at 7:49 PM Oleksandr Tyshchenko <olekst...@gmail.com> wrote:
>
>
>
> On 02.09.25 01:10, Mykola Kvach wrote:
>
> Hello Mykola
>
> > From: Mykola Kvach <mykola_kv...@epam.com>
> >
> > 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_kv...@epam.com>
> > ---
> > 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
ok
>
> > +
> > + 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?
Yes, we should. But currently I am restoring the same value
as it was before suspend...
I definitely agree that this needs to be fixed at the original
place where the issue was introduced, but I was planning to
address it in a future patch.
>
>
> > + 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?
I don’t see a reason to introduce such "macaron-style" code. On ARM, the
system suspend state is only set when CONFIG_SYSTEM_SUSPEND is defined
anyway.
If you would prefer me to wrap all relevant code with this define, please
let me know and I’ll make the change. In this case, I think the current
approach is cleaner, but I’m open to your opinion.
>
> > }
> >
> > return notifier_from_errno(rc);
>
Best regards,
Mykola
