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 <[email protected]> wrote: > > Hi Mykola, > > On 01/09/2025 23:10, Mykola Kvach wrote: > > From: Mirela Simonovic <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Saeed Nowshadi <[email protected]> > > Signed-off-by: Mykola Kvach <[email protected]> > > --- > > 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
