Re: [PATCH v6 01/13] xen/arm: Add suspend and resume timer helpers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Julien,

Thanks for your review and for the time spent on this series.

On Sat, Sep 13, 2025 at 2:04 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 01/09/2025 23:10, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > Timer interrupts must be disabled while the system is suspended to prevent
> > spurious wake-ups.
>
> Yet, you don't seem to disable the virtual interrupt. Can you explain why?

Thanks for the question — looks like I missed calling this out.

The virtual timer is already disabled on vCPU context switch. During the
suspend flow, ctxt_switch_from() calls virt_timer_save(), which clears
CNTV_CTL_EL0.ENABLE and preserves the timer state in
vcpu->arch.virt_timer. Therefore there is no live virtual timer interrupt
source by the time time_suspend() executes.

Also, the context switch happens before the suspend tasklet is invoked,
and time_suspend() is called from that tasklet.

>
> > Suspending the timers involves disabling both the EL1
> > physical timer and the EL2 hypervisor timer.
> I know this is what Arm is naming the timers. But I would rather s/EL1//
> and s/EL2// because the physical timer is also accessible from EL0.

Thanks, makes sense. I'll drop the EL1/EL2 wording and refer to them as
physical timer and hypervisor timer.

>
> Note that Xen doesn't use or expose the physical timer. So it should
> always be disabled at the point "time_suspend()" is called. I am still
> ok to disable it just in case though.

Right, Xen doesn't rely on CNTP, so it should already be disabled by
the time we reach time_suspend().

>
> > Resuming consists of raising
> > the TIMER_SOFTIRQ, which prompts the generic timer code to reprogram the
> > EL2 timer as needed. Re-enabling of the EL1 timer is left to the entity
> > that uses it.
> >
> > Introduce a new helper, disable_physical_timers, to encapsulate disabling
> > of the physical timers.
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in V4:
> >    - Rephrased comment and commit message for better clarity
> >    - Created separate function for disabling physical timers
> >
> > Changes in V3:
> >    - time_suspend and time_resume are now conditionally compiled
> >      under CONFIG_SYSTEM_SUSPEND
> > ---
> >   xen/arch/arm/include/asm/time.h |  5 +++++
> >   xen/arch/arm/time.c             | 38 +++++++++++++++++++++++++++------
> >   2 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/time.h 
> > b/xen/arch/arm/include/asm/time.h
> > index 49ad8c1a6d..f4fd0c6af5 100644
> > --- a/xen/arch/arm/include/asm/time.h
> > +++ b/xen/arch/arm/include/asm/time.h
> > @@ -108,6 +108,11 @@ void preinit_xen_time(void);
> >
> >   void force_update_vcpu_system_time(struct vcpu *v);
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +void time_suspend(void);
> > +void time_resume(void);
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> >   #endif /* __ARM_TIME_H__ */
> >   /*
> >    * Local variables:
> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> > index e74d30d258..ad984fdfdd 100644
> > --- a/xen/arch/arm/time.c
> > +++ b/xen/arch/arm/time.c
> > @@ -303,6 +303,14 @@ static void check_timer_irq_cfg(unsigned int irq, 
> > const char *which)
> >              "WARNING: %s-timer IRQ%u is not level triggered.\n", which, 
> > irq);
> >   }
> >
> > +/* Disable physical timers for EL1 and EL2 on the current CPU */
>
> The name of the times are "physical timer" and "hypervisor timer".

Ack

>
> > +static inline void disable_physical_timers(void)
>
> "Physical is a bit misleading" in this context. All the 3 timers
> (virtual, physical, hypervisor) are physical timers. My preference would
> be to name this function disable_timers() (assuming you also need to
> disable the virtual timer).

As explained above, CNTV is already disabled before suspend, so the helper
only targets CNTP/CNTHP. Renamed it to disable_phys_hyp_timers() accordingly.

>
> > +{
> > +    WRITE_SYSREG(0, CNTP_CTL_EL0);    /* Physical timer disabled */
> > +    WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
> > +    isb();
> > +}
> > +
> >   /* Set up the timer interrupt on this CPU */
> >   void init_timer_interrupt(void)
> >   {
> > @@ -310,9 +318,7 @@ void init_timer_interrupt(void)
> >       WRITE_SYSREG64(0, CNTVOFF_EL2);     /* No VM-specific offset */
> >       /* Do not let the VMs program the physical timer, only read the 
> > physical counter */
> >       WRITE_SYSREG(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
> > -    WRITE_SYSREG(0, CNTP_CTL_EL0);    /* Physical timer disabled */
> > -    WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
> > -    isb();
> > +    disable_physical_timers();
> >
> >       request_irq(timer_irq[TIMER_HYP_PPI], 0, htimer_interrupt,
> >                   "hyptimer", NULL);
> > @@ -330,9 +336,7 @@ void init_timer_interrupt(void)
> >    */
> >   static void deinit_timer_interrupt(void)
> >   {
> > -    WRITE_SYSREG(0, CNTP_CTL_EL0);    /* Disable physical timer */
> > -    WRITE_SYSREG(0, CNTHP_CTL_EL2);   /* Disable hypervisor's timer */
> > -    isb();
> > +    disable_physical_timers();
> >
> >       release_irq(timer_irq[TIMER_HYP_PPI], NULL);
> >       release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
> > @@ -372,6 +376,28 @@ void domain_set_time_offset(struct domain *d, int64_t 
> > time_offset_seconds)
> >       /* XXX update guest visible wallclock time */
> >   }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +void time_suspend(void)
> > +{
> > +    disable_physical_timers();
> > +}
> > +
> > +void time_resume(void)
> > +{
> > +    /*
> > +     * Raising the timer softirq triggers generic code to call 
> > reprogram_timer
> > +     * with the correct timeout (not known here).
> > +     *
> > +     * No further action is needed to restore timekeeping after power down,
> > +     * since the system counter is unaffected. See ARM DDI 0487 L.a, 
> > D12.1.2
> > +     * "The system counter must be implemented in an always-on power 
> > domain."
> > +     */
> > +    raise_softirq(TIMER_SOFTIRQ);
>
> I think we should add a comment about the physical timer.

I'll add a comment in time_resume() clarifying that the physical timer remains
disabled in Xen, while the virtual timer is restored per-vCPU on
context restore.

>
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> >   static int cpu_time_callback(struct notifier_block *nfb,
> >                                unsigned long action,
> >                                void *hcpu)Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola