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
