Re: [RFC PATCH v2] ptp: Add vDSO-style vmclock support

[Peter Hilber <peter.hilber@opensynergy.com>]

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