From: Mirela Simonovic <mirela.simonovic@aggios.com>
Timer interrupts must be disabled while the system is suspended to prevent
spurious wake-ups. Suspending the timers involves disabling both the EL1
physical timer and the EL2 hypervisor timer. 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@aggios.com>
Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
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 */
+static inline void disable_physical_timers(void)
+{
+ 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);
+}
+
+#endif /* CONFIG_SYSTEM_SUSPEND */
+
static int cpu_time_callback(struct notifier_block *nfb,
unsigned long action,
void *hcpu)
--
2.48.1Hi Mykola,
On 02/06/2025 10:04, Mykola Kvach wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> Timer interrupts must be disabled while the system is suspended to prevent
> spurious wake-ups. Suspending the timers involves disabling both the EL1
> physical timer and the EL2 hypervisor timer. 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@aggios.com>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> 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 */
> +static inline void disable_physical_timers(void)
> +{
> + 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);
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> static int cpu_time_callback(struct notifier_block *nfb,
> unsigned long action,
> void *hcpu)
A question. Do you see CPU_DYING gets invoked during platform suspend ?
I wonder how this code path is invoked with
time_suspend()
- Ayan
> --
> 2.48.1
>
>
Hi Ayan
On Fri, Jun 13, 2025 at 5:44 PM Ayan Kumar Halder <ayankuma@amd.com> wrote:
>
> Hi Mykola,
>
> On 02/06/2025 10:04, Mykola Kvach wrote:
> > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> >
> >
> > From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >
> > Timer interrupts must be disabled while the system is suspended to prevent
> > spurious wake-ups. Suspending the timers involves disabling both the EL1
> > physical timer and the EL2 hypervisor timer. 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@aggios.com>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > 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 */
> > +static inline void disable_physical_timers(void)
> > +{
> > + 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);
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > static int cpu_time_callback(struct notifier_block *nfb,
> > unsigned long action,
> > void *hcpu)
> A question. Do you see CPU_DYING gets invoked during platform suspend ?
Yes, it is invoked through the following code flow:
system_suspend (introduced in this patch series)
-> disable_nonboot_cpus
-> cpu_down
-> take_cpu_down
-> _take_cpu_down
> I wonder how this code path is invoked with
>
> time_suspend()
time_suspend is called directly from system_suspend,
which was introduced in one of the latest commits in this patch series.
In one of the previous patch series, there was a request to separate
implementations and usage of functions.
This was done to reduce the amount of code in each commit and to
simplify the review process.
>
> - Ayan
>
> > --
> > 2.48.1
> >
> >
Best regards,
Mykola
© 2016 - 2025 Red Hat, Inc.