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

[Jordan Rhee]

On Thu, Apr 9, 2026 at 8:16 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Mon,  6 Apr 2026 23:40:02 +0000 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.
>
> Interesting. I'd like this looked over by David Woodhouse and tglx.
> Please repost after the merge window or send them an RFC.

Ack, will include comparison against ptp_read_system_prets/postts()
implementation.

>
> > +static int gve_ptp_read_timestamp(struct gve_ptp *ptp, cycles_t *pre_cycles,
> > +                               cycles_t *post_cycles,
> > +                               struct system_time_snapshot *snap)
> > +{
> > +     unsigned long delay_us = 1000;
> > +     int retry_count = 0;
> > +     int err;
> > +
> > +     lockdep_assert_held(&ptp->nic_ts_read_lock);
> > +
> > +     do {
>
> This can't be a for () loop with 5 iterations?

I've reformulated it to be in terms of total timeout, and kept it as a
do-while to guarantee that at least 1 iteration is run.

>
> > +             if (snap)
> > +                     ktime_get_snapshot(snap);
> > +
> > +             *pre_cycles = get_cycles();
> > +             err = gve_adminq_report_nic_ts(ptp->priv,
> > +                                            ptp->nic_ts_report_bus);
> > +
> > +             /* Prevent get_cycles() from being speculatively executed
> > +              * before the AdminQ command
> > +              */
> > +             rmb();
> > +             *post_cycles = get_cycles();
> > +             if (likely(err != -EAGAIN))
> > +                     return err;
> > +
> > +             fsleep(delay_us);
> > +
> > +             /* Exponential backoff */
> > +             delay_us *= 2;
> > +             retry_count++;
> > +     } while (retry_count < 5);
> > +
> > +     return -ETIMEDOUT;
> > +}
> > +
> >  /* Read the nic timestamp from hardware via the admin queue. */
> > -static int gve_clock_nic_ts_read(struct gve_ptp *ptp, u64 *nic_raw)
> > +static int gve_clock_nic_ts_read(struct gve_ptp *ptp, u64 *nic_raw,
> > +                              struct gve_sysclock_sample *sysclock)
> >  {
> > +     cycles_t host_pre_cycles, host_post_cycles;
> > +     struct gve_nic_ts_report *ts_report;
> >       int err;
> >
> >       mutex_lock(&ptp->nic_ts_read_lock);
> > -     err = gve_adminq_report_nic_ts(ptp->priv, ptp->nic_ts_report_bus);
> > -     if (err)
> > +     err = gve_ptp_read_timestamp(ptp, &host_pre_cycles, &host_post_cycles,
> > +                                  sysclock ? &sysclock->snapshot : NULL);
> > +     if (err) {
> > +             dev_err_ratelimited(&ptp->priv->pdev->dev,
> > +                                 "AdminQ timestamp read failed: %d\n", err);
> >               goto out;
> > +     }
> >
> > -     *nic_raw = be64_to_cpu(ptp->nic_ts_report->nic_timestamp);
> > +     ts_report = ptp->nic_ts_report;
> > +     *nic_raw = be64_to_cpu(ts_report->nic_timestamp);
> > +
> > +     if (sysclock) {
> > +             sysclock->nic_pre_cycles = be64_to_cpu(ts_report->pre_cycles);
> > +             sysclock->nic_post_cycles = be64_to_cpu(ts_report->post_cycles);
> > +             sysclock->host_pre_cycles = host_pre_cycles;
> > +             sysclock->host_post_cycles = host_post_cycles;
> > +     }
> >
> >  out:
> >       mutex_unlock(&ptp->nic_ts_read_lock);
> >       return err;
> >  }
> >
> > +struct gve_cycles_to_clock_callback_ctx {
> > +     u64 cycles;
> > +};
> > +
> > +static int gve_cycles_to_clock_fn(ktime_t *device_time,
> > +                               struct system_counterval_t *system_counterval,
> > +                               void *ctx)
>
> Does this do anything GVE specific??

No, however, I would caution against adding an API to the kernel to
convert TSC to ktime since this could invite further abuses of TSC.

>
> > +{
> > +     struct gve_cycles_to_clock_callback_ctx *context = ctx;
> > +
> > +     *device_time = 0;
> > +
> > +     system_counterval->cycles = context->cycles;
> > +     system_counterval->use_nsecs = false;
> > +
> > +     if (IS_ENABLED(CONFIG_X86))
> > +             system_counterval->cs_id = CSID_X86_TSC;
> > +     else if (IS_ENABLED(CONFIG_ARM64))
> > +             system_counterval->cs_id = CSID_ARM_ARCH_COUNTER;
> > +     else
> > +             return -EOPNOTSUPP;
> > +
> > +     return 0;
> > +}
> --
> pw-bot: cr