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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Mykola,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> From: Mykola Kvach <mykola_kvach@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_kvach@epam.com>
> ---
>  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.

> +
> +    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.

> +
> +        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