On 02.07.24 18:39, David Woodhouse wrote: > On Tue, 2024-07-02 at 17:03 +0200, Peter Hilber wrote: >>> On 01.07.24 10:57, David Woodhouse wrote: >>>>> If my proposed memory structure is subsumed into the virtio-rtc >>>>> proposal we'd literally use the same names, but for the time being I'll >>>>> update mine to: >>> >>> Do you intend vmclock and virtio-rtc to be ABI compatible? > > I intend you to incorporate a shared memory structure like the vmclock > one into the virtio-rtc standard :) > > Because precision clocks in a VM are fundamentally nonsensical without > a way for the guest to know when live migration has screwed them up. > > So yes, so facilitate that, I'm trying to align things so that the > numeric values of fields like the time_type and smearing hint are > *precisely* the same as the VIRTIO_RTC_xxx values. > > Time pressure *may* mean I have to ship an ACPI-based device with a > preliminary version of the structure before I've finished persuading > you, and before we've completely finalized the structure. I am hoping > to avoid that. > > (In fact, my time pressure only applies to a version of the structure > which carries the disruption_marker field; the actual clock calibration > information doesn't have to be present in the interim implementation.) > > >>> FYI, I see a >>> potential problem in that Virtio does avoid the use of signed integers so >>> far. I did not check carefully if there might be other problems, yet. > > Hm, you use an unsigned integer to convey the tai_offset. I suppose at > +37 and with a plan to stop doing leap seconds in the next decade, > we're unlikely to get back below zero? > I think so. > The other signed integer I had was for the leap second direction, but I > think I'm happy to drop that and just adopt your uint8_t leap field > with VIRTIO_RTC_LEAP_{PRE_POS,PRE_NEG,etc.}. > > > > > >>>>> >>>>> /* >>>>> * What time is exposed in the time_sec/time_frac_sec fields? >>>>> */ >>>>> uint8_t time_type; >>>>> #define VMCLOCK_TIME_UTC 0 /* Since 1970-01-01 00:00:00z */ >>>>> #define VMCLOCK_TIME_TAI 1 /* Since 1970-01-01 00:00:00z */ >>>>> #define VMCLOCK_TIME_MONOTONIC 2 /* Since undefined epoch */ >>>>> #define VMCLOCK_TIME_INVALID 3 /* virtio-rtc uses this for smeared UTC */ >>>>> >>>>> >>>>> I can then use your smearing subtype values as the 'hint' field in the >>>>> shared memory structure. You currently have: >>>>> >>>>> +\begin{lstlisting} >>>>> +#define VIRTIO_RTC_SUBTYPE_STRICT 0 >>>>> +#define VIRTIO_RTC_SUBTYPE_SMEAR 1 >>>>> +#define VIRTIO_RTC_SUBTYPE_SMEAR_NOON_LINEAR 2 >>>>> +#define VIRTIO_RTC_SUBTYPE_LEAP_UNSPECIFIED 3 >>>>> +\end{lstlisting} >>>>> >>> >>> I agree with the above part of your proposal. >>> >>>>> I can certainly ensure that 'noon linear' has the same value. I don't >>>>> think you need both 'SMEAR' and 'LEAP_UNSPECIFIED' though: >>>>> >>>>> >>>>> +\item VIRTIO_RTC_SUBTYPE_SMEAR deviates from the UTC standard by >>>>> + smearing time in the vicinity of the leap second, in a not >>>>> + precisely defined manner. This avoids clock steps due to UTC >>>>> + leap seconds. >>>>> >>>>> ... >>>>> >>>>> +\item VIRTIO_RTC_SUBTYPE_LEAP_UNSPECIFIED may deviate from the UTC >>>>> + standard w.r.t.\ leap second introduction in an unspecified >>>>> way >>>>> + (leap seconds may, or may not, be smeared). >>>>> >>>>> To the client, both of those just mean "for a day or so around a leap >>>>> second event, you can't trust this device to know what the time is". >>>>> There isn't any point in separating "does lie to you" from "might lie >>>>> to you", surely? The guest can't do anything useful with that >>>>> distinction. Let's drop SMEAR and keep only LEAP_UNSPECIFIED? >>> >>> As for VIRTIO_RTC_SUBTYPE_SMEAR, I think this could be dropped indeed >>> (resp., UTC_SLS may be added). >>> >>> But VIRTIO_RTC_CLOCK_SMEARED_UTC is an assurance that there will be no >>> steps (in particular, steps backwards, which some clients might not like) >>> due to leap seconds, while LEAP_UNSPECIFIED provides no such guarantee. >>> >>> So I think this might be better handled by adding, alongside >>> >>>>> #define VIRTIO_RTC_CLOCK_SMEARED_UTC 3 >>> >>> #define VIRTIO_RTC_CLOCK_LEAP_UNSPECIFIED_UTC 4 >>> >>> (or any better name, like VIRTIO_RTC_CLOCK_MAYBE_SMEARED_UTC). >>> >>>>> >>>>> And if you *really* want to parameterise it, I think that's a bad idea >>>>> and it encourages the proliferation of different time "standards", but >>>>> I'd probably just suck it up and do whatever you do because that's not >>>>> strictly within the remit of my live-migration part. >>> >>> I think the above proposal to have subtypes for >>> VIRTIO_RTC_CLOCK_SMEARED_UTC should work. > > To clarify then, the main types are > > VIRTIO_RTC_CLOCK_UTC == 0 > VIRTIO_RTC_CLOCK_TAI == 1 > VIRTIO_RTC_CLOCK_MONOTONIC == 2 > VIRTIO_RTC_CLOCK_SMEARED_UTC == 3 > > And the subtypes are *only* for the case of > VIRTIO_RTC_CLOCK_SMEARED_UTC. They include > > VIRTIO_RTC_SUBTYPE_STRICT > VIRTIO_RTC_SUBTYPE_UNDEFINED /* or whatever you want to call it */ > VIRTIO_RTC_SUBTYPE_SMEAR_NOON_LINEAR > VIRTIO_RTC_SUBTYPE_UTC_SLS /* if it's worth doing this one */ > > Is that what we just agreed on? > > This is a misunderstanding. My idea was that the main types are > VIRTIO_RTC_CLOCK_UTC == 0 > VIRTIO_RTC_CLOCK_TAI == 1 > VIRTIO_RTC_CLOCK_MONOTONIC == 2 > VIRTIO_RTC_CLOCK_SMEARED_UTC == 3 VIRTIO_RTC_CLOCK_MAYBE_SMEARED_UTC == 4 The subtypes would be (1st for clocks other than VIRTIO_RTC_CLOCK_SMEARED_UTC, 2nd to last for VIRTIO_RTC_CLOCK_SMEARED_UTC): #define VIRTIO_RTC_SUBTYPE_STRICT 0 #define VIRTIO_RTC_SUBTYPE_SMEAR_NOON_LINEAR 1 #define VIRTIO_RTC_SUBTYPE_SMEAR_UTC_SLS 2