Re: [RFC PATCH 1/2] ptp: vmclock: add vm generation counter

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On Tue, 2025-11-25 at 15:38 +0000, Chalios, Babis wrote:
> Similar to live migration, loading a VM from some saved state (aka
> snapshot) is also an event that calls for clock adjustments in the
> guest. However, guests might want to take more actions as a response to
> such events, e.g. as discarding UUIDs, resetting network connections,
> reseeding entropy pools, etc. These are actions that guests don't
> typically take during live migration, so add a new field in the
> vmclock_abi called vm_generation_counter which informs the guest about
> such events.
> 
> Signed-off-by: Babis Chalios <bchalios@amazon.es>
> ---
>  include/uapi/linux/vmclock-abi.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/linux/vmclock-abi.h b/include/uapi/linux/vmclock-abi.h
> index 2d99b29ac44a..fbf1c5928273 100644
> --- a/include/uapi/linux/vmclock-abi.h
> +++ b/include/uapi/linux/vmclock-abi.h
> @@ -115,6 +115,12 @@ struct vmclock_abi {
>  	 * bit again after the update, using the about-to-be-valid fields.
>  	 */
>  #define VMCLOCK_FLAG_TIME_MONOTONIC		(1 << 7)
> +	/*
> +	 * If the VM_GEN_COUNTER_PRESENT flag is set, the hypervisor will
> +	 * bump the vm_generation_counter field every time the guest is
> +	 * loaded from some save state (restored from a snapshot).
> +	 */
> +#define VMCLOCK_FLAG_VM_GEN_COUNTER_PRESENT     (1 << 8)
>  
>  	__u8 pad[2];
>  	__u8 clock_status;
> @@ -177,6 +183,19 @@ struct vmclock_abi {
>  	__le64 time_frac_sec;		/* Units of 1/2^64 of a second */
>  	__le64 time_esterror_nanosec;
>  	__le64 time_maxerror_nanosec;
> +
> +	/*
> +	 * This field changes to another non-repeating value when the VM
> +	 * is loaded from a snapshot. This event, typically, represents a
> +	 * "jump" forward in time. As a result, in this case as well, the
> +	 * guest needs to discard any calibrarion against external sources.
> +	 * Loading a snapshot in a VM has different semantics than other VM
> +	 * events such as live migration, i.e. apart from re-adjusting guest
> +	 * clocks a guest user space might want to discard UUIDs, reset
> +	 * network connections or reseed entropy, etc. As a result, we
> +	 * use a dedicated marker for such events.
> +	 */
> +	__le64 vm_generation_counter;
>  };

Looks good, thank you. Just a bit of nitpicking about the comment.

I really don't like talking about a "jump forward in time". That's what
clocks *do*. VMClock tells the guest a relationship between its CPU
counter (TSC, arch timer, etc.) and real time. As long as that
relationship remains within the error bounds which were previously
promised, there is no disruption. It certainly isn't about time jumping
*forward*.

Let's also make it 100% clear that an implementation must *also* bump
the disruption_marker field in this case, not only the
vm_generation_counter. How about...

"A change in this field indicates that the guest has been loaded from a
snapshot. In addition to handling a disruption in time (which will also
be signalled through the disruption_marker field), a guest may wish to
discard UUIDs, reset network connections or reseed entropy, etc."

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]