Re: [PATCH v3 1/5] arm/time: Use static irqaction

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Mykyta,

On Fri, Oct 10, 2025 at 12:22 PM Mykyta Poturai <Mykyta_Poturai@xxxxxxxx> wrote:
>
> When stopping a core deinit_timer_interrupt is called in non-alloc
> context, which causes xfree in release_irq to fail an assert.
>
> To fix this, switch to a statically allocated irqaction that does not
> need to be freed in release_irq.
>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
>
> v2->v3:
> * no changes
>
> v1->v2:
> * Use percpu actions
> ---
>  xen/arch/arm/time.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index e74d30d258..59349467de 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -303,9 +303,15 @@ static void check_timer_irq_cfg(unsigned int irq, const 
> char *which)
>             "WARNING: %s-timer IRQ%u is not level triggered.\n", which, irq);
>  }
>
> +DEFINE_PER_CPU_READ_MOSTLY(struct irqaction, irq_hyp);
> +DEFINE_PER_CPU_READ_MOSTLY(struct irqaction, irq_virt);

Scope nit (MISRA C:2012 R8.8):
if irq_hyp and irq_virt are only used in this TU, give it internal linkage.

> +
>  /* Set up the timer interrupt on this CPU */
>  void init_timer_interrupt(void)
>  {
> +    struct irqaction *hyp_action = &this_cpu(irq_hyp);
> +    struct irqaction *virt_action = &this_cpu(irq_virt);
> +
>      /* Sensible defaults */
>      WRITE_SYSREG64(0, CNTVOFF_EL2);     /* No VM-specific offset */
>      /* Do not let the VMs program the physical timer, only read the physical 
> counter */
> @@ -314,10 +320,17 @@ void init_timer_interrupt(void)
>      WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
>      isb();
>
> -    request_irq(timer_irq[TIMER_HYP_PPI], 0, htimer_interrupt,
> -                "hyptimer", NULL);
> -    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
> -                   "virtimer", NULL);
> +    hyp_action->name = "hyptimer";
> +    hyp_action->handler = htimer_interrupt;
> +    hyp_action->dev_id = NULL;
> +    hyp_action->free_on_release = 0;
> +    setup_irq(timer_irq[TIMER_HYP_PPI], 0, hyp_action);
> +
> +    virt_action->name = "virtimer";
> +    virt_action->handler = vtimer_interrupt;
> +    virt_action->dev_id = NULL;
> +    virt_action->free_on_release = 0;
> +    setup_irq(timer_irq[TIMER_VIRT_PPI], 0, virt_action);
>
>      check_timer_irq_cfg(timer_irq[TIMER_HYP_PPI], "hypervisor");
>      check_timer_irq_cfg(timer_irq[TIMER_VIRT_PPI], "virtual");
> --
> 2.34.1
>

Reviewed-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Best regards,
Mykola