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

[Jacob Keller <jacob.e.keller@intel.com>]

On 4/6/2026 1:41 PM, Jordan Rhee wrote:
> On Fri, Apr 3, 2026 at 2:18 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>> On 4/3/2026 12:44 PM, Harshitha Ramamurthy wrote:
>>> From: Jordan Rhee <jordanrhee@google.com>
>>>
>>> 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@google.com>
>>> Reviewed-by: Kevin Yang <yyd@google.com>
>>> Reviewed-by: Naman Gulati <namangulati@google.com>
>>> Signed-off-by: Jordan Rhee <jordanrhee@google.com>
>>> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
>>> ---
>>
>>> +/*
>>> + * 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.

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