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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi,

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

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

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@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)

-- 
WBR, Volodymyr