Date: Tue, 02 Jul 2024 17:39:16 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Peter Hilber <peter.hilber@...nsynergy.com>,
 linux-kernel@...r.kernel.org,  virtualization@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org,  linux-rtc@...r.kernel.org, "Ridoux,
 Julien" <ridouxj@...zon.com>,  virtio-dev@...ts.linux.dev, "Luu, Ryan"
 <rluu@...zon.com>
Cc: "Christopher S. Hall" <christopher.s.hall@...el.com>, Jason Wang
 <jasowang@...hat.com>, John Stultz <jstultz@...gle.com>, "Michael S.
 Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org, Richard Cochran
 <richardcochran@...il.com>, Stephen Boyd <sboyd@...nel.org>, Thomas
 Gleixner <tglx@...utronix.de>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Marc
 Zyngier <maz@...nel.org>, Mark Rutland <mark.rutland@....com>, Daniel
 Lezcano <daniel.lezcano@...aro.org>, Alessandro Zummo
 <a.zummo@...ertech.it>,  Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [RFC PATCH v2] ptp: Add vDSO-style vmclock support

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?

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?



Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists