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

[Jordan Rhee]

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.

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

>
> Thanks,
> Jake
>
> > +     switch (clockid) {
> > +     case CLOCK_REALTIME:
> > +             *ts = ktime_to_timespec64(xtstamp.sys_realtime);
> > +             break;
> > +     case CLOCK_MONOTONIC_RAW:
> > +             *ts = ktime_to_timespec64(xtstamp.sys_monoraw);
> > +             break;
> > +     default:
> > +             dev_err_ratelimited(&priv->pdev->dev,
> > +                                 "Cycle count conversion to clockid %d not supported\n",
> > +                                 clockid);
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     return 0;
> > +}
> > +