Re: [XEN PATCH] x86/vhpet: Add option to always fire hpet timer on resume

[Jan Beulich <jbeulich@suse.com>]

On 27.08.2025 08:01, Vyacheslav Legoshin wrote:
> The following issue was observed on Windows 10 21H2 x64+: when the domain state
> is saved while all cores are executing the 'halt' instruction, and the memory
> save takes a relatively long time (tens of seconds), the HPET counter may
> overflow as follows:
> counter  = 11243f3e4a
> comparator = 910cb70f
> 
> In such cases, the fix implemented in commit
> b144cf45d50b603c2909fc32c6abf7359f86f1aa does not work (because the 'diff' is
> not negative), resulting in the guest VM becoming unresponsive for
> approximately 30 seconds.
> 
> This patch adds an option to always adjust the HPET timer to fire immediately
> after restore.

Thanks for the patch, but issues already start here: There's no Signed-off-by:.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1461,6 +1461,15 @@ HPET can be disabled by specifying `hpet=0`.
>  
>  Deprecated alternative of `hpet=broadcast`.
>  
> +### hpet_drift_fix (x86)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Always set HPET timer to fire immediately after domain restore.
> +This option can be used to fix unresponsive snapshots with modern x64 Windows
> +systems (21H2+) which use non-periodic timers.

I'm not convinced making this a global option is appropriate. If an option is
needed, it would better be a per-domain setting. Whether an option is needed
in the first place is tbd.

And then, if a global option was used, then please with dashes in favor of
underscores in its name.

> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -11,6 +11,7 @@
>  #include <asm/current.h>
>  #include <asm/hpet.h>
>  #include <asm/mc146818rtc.h>
> +#include <xen/param.h>
>  #include <xen/sched.h>
>  #include <xen/event.h>
>  #include <xen/trace.h>
> @@ -222,6 +223,9 @@ static void cf_check hpet_timer_fired(struct vcpu *v, void *data)
>   * 1/(2^10) second, namely, 0.9765625 milliseconds */
>  #define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
>  
> +bool hpet_drift_fix;

static and __ro_after_init.

> @@ -268,11 +272,18 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
>       * are restoring after migrate, treat any wrap as past since the value
>       * is unlikely to be 'small'.
>       */
> -    if ( (int64_t)diff < 0 )
> -        diff = (timer_is_32bit(h, tn) &&
> -                vhpet_domain(h)->creation_finished &&
> -                (-diff > HPET_TINY_TIME_SPAN))
> -            ? (uint32_t)diff : 0;
> +    if (hpet_drift_fix && !vhpet_domain(h)->creation_finished)

Nit (style): Missing blanks (see e.g. the other if() you're altering).

> +    {
> +        diff = 0;
> +    }

No real need for figure braces here.

The comment ahead of the construct also wants amending / updating.

> +    else
> +    {
> +        if ( (int64_t)diff < 0 )

"else if()" please, reducing the diff quite a bit.

Jan