Re: [PATCH] x86/suspend: unconditionally raise a timer softirq on resume

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Aug 25, 2025 at 05:15:15PM +0200, Roger Pau Monne wrote:
> The current code to restore the timer state on resume is incomplete.  While
> the local APIC Initial Count Register is saved and restored across
> suspension (even if possibly no longer accurate since it's not adjusted to
> account for the time spent in suspension), the TSC deadline MSR is not
> saved and restored, hence hosts using the TSC deadline timer will likely
> get stuck when resuming from suspension.
> 
> The lack of restoring of the TSC deadline MSR was mitigated by the raising
> of a timer softirq in mwait_idle_with_hints() if the timer had expired,
> previous to commit 3faf0866a33070b926ab78e6298290403f85e76c, which removed
> that logic.
> 
> This patch fixes the usage of the TSC deadline timer with suspension, by
> unconditionally raising a timer softirq on resume, that will take care of
> rearming the hardware timer.  Given that a timer softirq will be
> unconditionally risen, there's no need to save and restore the APIC Initial
> Count Register anymore either.
> 
> Note that secondary processors don't need this special treatment when
> resuming, since they are offlined before suspension and brought back up
> during resume, the first timer that gets setup will trigger a timer softirq
> unconditionally, for example from sched_migrate_timers() that gets called
> for each secondary processor.
> 
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Fixes: fd1291a826e1 ('X86: Prefer TSC-deadline timer in Xen')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Tested-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
>  xen/arch/x86/acpi/power.c | 2 ++
>  xen/arch/x86/apic.c       | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 2ac162c997fe..27d672ad5dbb 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -19,6 +19,7 @@
>  #include <xen/iommu.h>
>  #include <xen/param.h>
>  #include <xen/sched.h>
> +#include <xen/softirq.h>
>  #include <xen/spinlock.h>
>  #include <xen/watchdog.h>
>  
> @@ -310,6 +311,7 @@ static int enter_state(u32 state)
>      thaw_domains();
>      system_state = SYS_STATE_active;
>      spin_unlock(&pm_lock);
> +    raise_softirq(TIMER_SOFTIRQ);
>      return error;
>  }
>  
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index cac5ba39e615..e3a2b84f1aae 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -65,7 +65,6 @@ static struct {
>      unsigned int apic_lvt0;
>      unsigned int apic_lvt1;
>      unsigned int apic_lvterr;
> -    unsigned int apic_tmict;
>      unsigned int apic_tdcr;
>      unsigned int apic_thmr;
>  } apic_pm_state;
> @@ -658,7 +657,6 @@ int lapic_suspend(void)
>      apic_pm_state.apic_lvt0 = apic_read(APIC_LVT0);
>      apic_pm_state.apic_lvt1 = apic_read(APIC_LVT1);
>      apic_pm_state.apic_lvterr = apic_read(APIC_LVTERR);
> -    apic_pm_state.apic_tmict = apic_read(APIC_TMICT);
>      apic_pm_state.apic_tdcr = apic_read(APIC_TDCR);
>      if (maxlvt >= 5)
>          apic_pm_state.apic_thmr = apic_read(APIC_LVTTHMR);
> @@ -718,7 +716,6 @@ int lapic_resume(void)
>          apic_write(APIC_LVTPC, apic_pm_state.apic_lvtpc);
>      apic_write(APIC_LVTT, apic_pm_state.apic_lvtt);
>      apic_write(APIC_TDCR, apic_pm_state.apic_tdcr);
> -    apic_write(APIC_TMICT, apic_pm_state.apic_tmict);
>      apic_write(APIC_ESR, 0);
>      apic_read(APIC_ESR);
>      apic_write(APIC_LVTERR, apic_pm_state.apic_lvterr);
> -- 
> 2.49.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature