Re: [PATCH net-next v3 3/3] gve: implement PTP gettimex64

[Jordan Rhee]

On Wed, Apr 8, 2026 at 3:43 PM Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
>
> On 4/6/2026 1:41 PM, Jordan Rhee wrote:
> > On Fri, Apr 3, 2026 at 2:18 PM Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
> >>
> >> On 4/3/2026 12:44 PM, Harshitha Ramamurthy wrote:
> >>> From: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> >>>
> >>> Enable chrony and phc2sys to synchronize system clock to NIC clock.
> >>>
> >>> The system cycle counters are sampled by the device to minimize the
> >>> uncertainty window. If the system times are sampled in the host, the
> >>> delta between pre and post readings is 100us or more due to AQ command
> >>> latency. The system times returned by the device have a delta of ~1us,
> >>> which enables significantly more accurate clock synchronization.
> >>>
> >>> Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> >>> Reviewed-by: Kevin Yang <yyd@xxxxxxxxxx>
> >>> Reviewed-by: Naman Gulati <namangulati@xxxxxxxxxx>
> >>> Signed-off-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> >>> Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> >>> ---
> >>
> >>> +/*
> >>> + * Convert a raw cycle count (e.g. from get_cycles()) to the system clock
> >>> + * type specified by clockid. The system_time_snapshot must be taken before
> >>> + * the cycle counter is sampled.
> >>> + */
> >>> +static int gve_cycles_to_timespec64(struct gve_priv *priv, clockid_t clockid,
> >>> +                                 struct system_time_snapshot *snap,
> >>> +                                 u64 cycles, struct timespec64 *ts)
> >>> +{
> >>> +     struct gve_cycles_to_clock_callback_ctx ctx = {0};
> >>> +     struct system_device_crosststamp xtstamp;
> >>> +     int err;
> >>> +
> >>> +     ctx.cycles = cycles;
> >>> +     err = get_device_system_crosststamp(gve_cycles_to_clock_fn, &ctx, snap,
> >>> +                                         &xtstamp);
> >>> +     if (err) {
> >>> +             dev_err_ratelimited(&priv->pdev->dev,
> >>> +                                 "get_device_system_crosststamp() failed to convert %lld cycles to system time: %d\n",
> >>> +                                 cycles,
> >>> +                                 err);
> >>> +             return err;
> >>> +     }
> >>> +
> >>
> >> This looks a lot like a cross timestamp (i.e. something like PCIe PTM)
> >> Why not just implement the .crosstimestamp and PTP_SYS_OFF_PRECISE? Does
> >> that not work properly? Or is this not really a cross timestamp despite
> >> use of the get_device_system_crosststamp handler? :D
> >
> > .crosstimestamp is for devices that support simultaneous NIC and
> > system timestamps. Devices that don't support simultaneous timestamps
> > have to take a system time sandwich by calling
> > ptp_read_system_prets()/ptp_read_system_postts() on either side of the
> > NIC timestamp. Upper layers (e.g. chrony) use the sandwich delta in
> > nontrivial ways when estimating the system clock / NIC clock offset.
> > This is information that must be preserved, and it would be incorrect
> > to implement .crosstimestamp by returning the midpoint of the
> > sandwich, as tempting as that implementation might be.
> >
>
> True.
>
> > Gvnic does not support simultaneous NIC and system timestamps, so it
> > must use the sandwich technique. Since the NIC timestamp is obtained
> > using a firmware (hypervisor) call, the uncertainty window would be
> > too large if it were taken inside the VM. Gvnic takes the sandwich in
> > the hypervisor and returns the raw TSC values to the VM.
> > get_device_system_crosststamp() is used to convert the TSCs to system
> > times, which I believe is the only correct way to do this conversion.
> > Jordan
> >
>
> Hmm. The function says:
>
> "Synchronously capture system/device timestamp". That is what confuses
> me. Your implementation uses gve_cycles_to_clock_fn() which just sets
> some values in the system_counterval struct and exits. It doesn't
> "capture a system/device timestamp" tuple.
>
> This does feel a bit weird. No other caller appears to exist outside of
> the cross timestamp implementations.
>
> It sounds like what you want is a function that takes a cycles count
> value and does the conversion from TSC to the appropriate clock, along
> with all of the interopolation etc. What you've done is sort of a cludge
> around get_device_system_crosststamp() to force it to do that for you
> without actually using it as intended.
>
> I'd argue it would be better to have a cycles_to_ktime() or something
> which takes the TSC cycles value and the appropriate clock and does the
> exact same flow as get_device_system_crosststamp() for converting the
> cycles into proper ktime values without the mess of the callback
> function etc.

Yes, that's exactly how I'm using get_device_system_crosststamp().
There is no cycles_to_ktime() function, and adding one would make this
patch more difficult to backport to older kernels.

There is precedent for using get_device_system_crosststamp() this way
in the virtio_rtc driver. In viortc_ptp_getcrosststamp(),  the
timestamp is sampled before calling get_device_system_crosststamp(),
and the callback simply populates the return values from the context.
I believe the snapshot parameter was added to support this use case.

>
> I guess in principle what you've implemented is "correct" and
> functional, but it definitely feels a bit weird to use the API in this
> way. It smells like a neat hack instead of a proper interface for this
> purpose.
>
> That said, I won't object strongly if the maintainers are fine with
> using it for this purpose.
>
> Thanks,
> Jake