[PATCH net-next v6 0/3] gve: add support for PTP gettimex64

[PATCH net-next v6 0/3] gve: add support for PTP gettimex64

[Harshitha Ramamurthy]

From: Jordan Rhee <jordanrhee@google.com>

This patch series adds support to obtain near-simultaneous NIC and
system timestamps with gettimex64. This enables daemons like
chrony and phc2sys to synchronize the system clock to the NIC clock.

GVE does not have direct register access to the NIC hardware clock, so
it must issue an AdminQ command to read the NIC clock. Two paths
for obtaining a cross-timestamp are implemented: a precise path using
system counter values sampled by the device, and a fallback path using
system counter values sampled in the driver using
ptp_read_system_prets()/postts().

To use the precise path, the current system clocksource must match the
units returned by the device, which on x86 is X86_TSC and on ARM64 is
ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
by default on GCP VMs using Chrony, so we expect the precise path to be
used the vast majority of the time. If the system clocksource is changed
to kvm-clock, it activates the fallback path. Ethtool counters have been
added to count how many times each path is used.

The uncertainty window in the precise path is typically around 1-2us,
while in the fallback path is around 60-80us. This table shows a
comparison in chrony tracking statistics between the precise path and
fallback path. The RMS offset is nearly 4 orders of magnitude smaller
in the precise path.

|                 | Fallback Path         | Precise path             |
| --------------- | --------------------- | ------------------------ |
| System time     | 0.000000005 s slow    | 0.000000001 s fast       |
| Last offset     | +0.000005606 seconds  | +0.000000001 seconds     |
| RMS offset      | 0.000009020 seconds   | 0.000000002 seconds      |
| Frequency       | 4.115 ppm fast        | 0.362 ppm fast           |
| Residual freq   | +2.515 ppm            | +0.000 ppm               |
| Skew            | 18.480 ppm            | 0.001 ppm                |
| Root delay      | 0.000000001 seconds   | 0.000000001 seconds      |
| Root dispersion | 0.000081905 seconds   | 0.000001169 seconds      |
| Update interval | 0.5 seconds           | 0.5 seconds              |
| Leap status     | Normal                | Normal                   |

The first two patches pave the way for the PTP implementation by
quieting excessive logging and refactoring an existing routine for
thread safety.

---
Changelog:
V6:
- Added a fallback to driver-sampled time sandwich that is used when
  the following conditions are not met:
  - The system clock source is X86_TSC or ARM_ARCH_COUNTER
  - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
  - The architecture is x86 or ARM64
- Added ethtool statistics to count how many cross-timestamps used the
  precise path versus fallback path.
- Fixed printf format specifier.
- Added stub implementions of adjtime and adjfine to prevent NULL
  dereference when phc2sys tries to adjust clock.
- Moved system time snapshot back to gve_ptp_gettimex64() so we can get
  the current system clock source from it. It is OK for it to be outside
  the mutex and retry loop because lock contention and retries should be
  extremely rare, and chrony filters out bad samples.
- Link to v5: https://lore.kernel.org/netdev/20260429012819.3102675-1-hramamurthy@google.com/

V5:
- Reformulate retry loop in terms of total timeout instead of retry
  count (Jakub Kicinski)
- Link to v4: https://lore.kernel.org/netdev/20260406234002.3610542-1-hramamurthy@google.com/

V4:
- Call out change to dev_err_ratelimited() in patch 1 commit message (Jacob Keller)
- Ensure only one log is emitted when command returns GVE_ADMINQ_COMMAND_UNSET (Jacob Keller)
- Link to v3: https://lore.kernel.org/netdev/20260403194427.1830609-1-hramamurthy@google.com/

V3:
- Take system time snapshot inside the mutex
- Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
  than x86 or arm64
- Fix initialization to only register PTP clock once all data is
  initialized
- Link to v2: https://lore.kernel.org/netdev/20260326224527.1044097-1-hramamurthy@google.com/

V2:
- Fixed compilation warning on ARM by casting to u64
- Link to v1: https://lore.kernel.org/netdev/20260323234829.3185051-1-hramamurthy@google.com/
---

Ankit Garg (1):
  gve: make nic clock reads thread safe

Jordan Rhee (2):
  gve: skip error logging for retryable AdminQ commands
  gve: implement PTP gettimex64

 drivers/net/ethernet/google/gve/gve.h         |  20 +-
 drivers/net/ethernet/google/gve/gve_adminq.c  |  27 +-
 drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
 drivers/net/ethernet/google/gve/gve_ethtool.c |   6 +-
 drivers/net/ethernet/google/gve/gve_ptp.c     | 357 ++++++++++++++----
 5 files changed, 319 insertions(+), 95 deletions(-)


base-commit: 8c699be3dad7bba87cdda485dc099226cfc2f706
-- 
2.54.0.545.g6539524ca2-goog

[PATCH net-next v6 1/3] gve: skip error logging for retryable AdminQ commands

[Harshitha Ramamurthy]

From: Jordan Rhee <jordanrhee@google.com>

AdminQ commands may return -EAGAIN under certain transient conditions.
These commands are intended to be retried by the driver, so logging
a formal error to the system log is misleading and creates
unnecessary noise.

Modify the logging logic to skip the error message when the result
is -EAGAIN, and move logging to dev_err_ratelimited() to avoid
spamming the log.

Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
Changes in v4:
- call out change to dev_err_ratelimited() in the commit message (Jacob Keller)
- remove extra print when adminQ status is GVE_ADMINQ_COMMAND_UNSET (Jacob Keller)
---
 drivers/net/ethernet/google/gve/gve_adminq.c | 27 +++++++++++++++-----
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 08587bf40ed4..a65b14835aa0 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -416,16 +416,10 @@ static bool gve_adminq_wait_for_cmd(struct gve_priv *priv, u32 prod_cnt)
 
 static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
 {
-	if (status != GVE_ADMINQ_COMMAND_PASSED &&
-	    status != GVE_ADMINQ_COMMAND_UNSET) {
-		dev_err(&priv->pdev->dev, "AQ command failed with status %d\n", status);
-		priv->adminq_cmd_fail++;
-	}
 	switch (status) {
 	case GVE_ADMINQ_COMMAND_PASSED:
 		return 0;
 	case GVE_ADMINQ_COMMAND_UNSET:
-		dev_err(&priv->pdev->dev, "parse_aq_err: err and status both unset, this should not be possible.\n");
 		return -EINVAL;
 	case GVE_ADMINQ_COMMAND_ERROR_ABORTED:
 	case GVE_ADMINQ_COMMAND_ERROR_CANCELLED:
@@ -455,6 +449,16 @@ static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
 	}
 }
 
+static bool gve_adminq_is_retryable(enum gve_adminq_opcodes opcode)
+{
+	switch (opcode) {
+	case GVE_ADMINQ_REPORT_NIC_TIMESTAMP:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /* Flushes all AQ commands currently queued and waits for them to complete.
  * If there are failures, it will return the first error.
  */
@@ -482,9 +486,18 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
 		cmd = &priv->adminq[i & priv->adminq_mask];
 		status = be32_to_cpu(READ_ONCE(cmd->status));
 		err = gve_adminq_parse_err(priv, status);
-		if (err)
+		if (err) {
+			enum gve_adminq_opcodes opcode =
+				be32_to_cpu(READ_ONCE(cmd->opcode));
+			priv->adminq_cmd_fail++;
+			if (!gve_adminq_is_retryable(opcode) || err != -EAGAIN)
+				dev_err_ratelimited(&priv->pdev->dev,
+						    "AQ command %d failed with status %d\n",
+						    opcode, status);
+
 			// Return the first error if we failed.
 			return err;
+		}
 	}
 
 	return 0;
-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH net-next v6 1/3] gve: skip error logging for retryable AdminQ commands

[Jacob Keller]

On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> From: Jordan Rhee <jordanrhee@google.com>
> 
> AdminQ commands may return -EAGAIN under certain transient conditions.
> These commands are intended to be retried by the driver, so logging
> a formal error to the system log is misleading and creates
> unnecessary noise.
> 
> Modify the logging logic to skip the error message when the result
> is -EAGAIN, and move logging to dev_err_ratelimited() to avoid
> spamming the log.
> 

Excellent!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Jordan Rhee <jordanrhee@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
> Changes in v4:
> - call out change to dev_err_ratelimited() in the commit message (Jacob Keller)
> - remove extra print when adminQ status is GVE_ADMINQ_COMMAND_UNSET (Jacob Keller)
> ---
>  drivers/net/ethernet/google/gve/gve_adminq.c | 27 +++++++++++++++-----
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 08587bf40ed4..a65b14835aa0 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -416,16 +416,10 @@ static bool gve_adminq_wait_for_cmd(struct gve_priv *priv, u32 prod_cnt)
>  
>  static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
>  {
> -	if (status != GVE_ADMINQ_COMMAND_PASSED &&
> -	    status != GVE_ADMINQ_COMMAND_UNSET) {
> -		dev_err(&priv->pdev->dev, "AQ command failed with status %d\n", status);
> -		priv->adminq_cmd_fail++;
> -	}
>  	switch (status) {
>  	case GVE_ADMINQ_COMMAND_PASSED:
>  		return 0;
>  	case GVE_ADMINQ_COMMAND_UNSET:
> -		dev_err(&priv->pdev->dev, "parse_aq_err: err and status both unset, this should not be possible.\n");
>  		return -EINVAL;
>  	case GVE_ADMINQ_COMMAND_ERROR_ABORTED:
>  	case GVE_ADMINQ_COMMAND_ERROR_CANCELLED:
> @@ -455,6 +449,16 @@ static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
>  	}
>  }
>  
> +static bool gve_adminq_is_retryable(enum gve_adminq_opcodes opcode)
> +{
> +	switch (opcode) {
> +	case GVE_ADMINQ_REPORT_NIC_TIMESTAMP:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /* Flushes all AQ commands currently queued and waits for them to complete.
>   * If there are failures, it will return the first error.
>   */
> @@ -482,9 +486,18 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
>  		cmd = &priv->adminq[i & priv->adminq_mask];
>  		status = be32_to_cpu(READ_ONCE(cmd->status));
>  		err = gve_adminq_parse_err(priv, status);
> -		if (err)
> +		if (err) {
> +			enum gve_adminq_opcodes opcode =
> +				be32_to_cpu(READ_ONCE(cmd->opcode));
> +			priv->adminq_cmd_fail++;
> +			if (!gve_adminq_is_retryable(opcode) || err != -EAGAIN)
> +				dev_err_ratelimited(&priv->pdev->dev,
> +						    "AQ command %d failed with status %d\n",
> +						    opcode, status);
> +
>  			// Return the first error if we failed.
>  			return err;
> +		}
>  	}
>  
>  	return 0;

Re: [PATCH net-next v6 1/3] gve: skip error logging for retryable AdminQ commands

[Jordan Rhee]

On Thu, May 7, 2026 at 2:15 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> > From: Jordan Rhee <jordanrhee@google.com>
> >
> > AdminQ commands may return -EAGAIN under certain transient conditions.
> > These commands are intended to be retried by the driver, so logging
> > a formal error to the system log is misleading and creates
> > unnecessary noise.
> >
> > Modify the logging logic to skip the error message when the result
> > is -EAGAIN, and move logging to dev_err_ratelimited() to avoid
> > spamming the log.
> >
>
> Excellent!
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>
> > Reviewed-by: Joshua Washington <joshwash@google.com>
> > Signed-off-by: Jordan Rhee <jordanrhee@google.com>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > ---
> > Changes in v4:
> > - call out change to dev_err_ratelimited() in the commit message (Jacob Keller)
> > - remove extra print when adminQ status is GVE_ADMINQ_COMMAND_UNSET (Jacob Keller)
> > ---
> >  drivers/net/ethernet/google/gve/gve_adminq.c | 27 +++++++++++++++-----
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 08587bf40ed4..a65b14835aa0 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -416,16 +416,10 @@ static bool gve_adminq_wait_for_cmd(struct gve_priv *priv, u32 prod_cnt)
> >
> >  static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
> >  {
> > -     if (status != GVE_ADMINQ_COMMAND_PASSED &&
> > -         status != GVE_ADMINQ_COMMAND_UNSET) {
> > -             dev_err(&priv->pdev->dev, "AQ command failed with status %d\n", status);
> > -             priv->adminq_cmd_fail++;
> > -     }
> >       switch (status) {
> >       case GVE_ADMINQ_COMMAND_PASSED:
> >               return 0;
> >       case GVE_ADMINQ_COMMAND_UNSET:
> > -             dev_err(&priv->pdev->dev, "parse_aq_err: err and status both unset, this should not be possible.\n");
> >               return -EINVAL;
> >       case GVE_ADMINQ_COMMAND_ERROR_ABORTED:
> >       case GVE_ADMINQ_COMMAND_ERROR_CANCELLED:
> > @@ -455,6 +449,16 @@ static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
> >       }
> >  }
> >
> > +static bool gve_adminq_is_retryable(enum gve_adminq_opcodes opcode)
> > +{
> > +     switch (opcode) {
> > +     case GVE_ADMINQ_REPORT_NIC_TIMESTAMP:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> >  /* Flushes all AQ commands currently queued and waits for them to complete.
> >   * If there are failures, it will return the first error.
> >   */
> > @@ -482,9 +486,18 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
> >               cmd = &priv->adminq[i & priv->adminq_mask];
> >               status = be32_to_cpu(READ_ONCE(cmd->status));
> >               err = gve_adminq_parse_err(priv, status);
> > -             if (err)
> > +             if (err) {
> > +                     enum gve_adminq_opcodes opcode =
> > +                             be32_to_cpu(READ_ONCE(cmd->opcode));
> > +                     priv->adminq_cmd_fail++;
> > +                     if (!gve_adminq_is_retryable(opcode) || err != -EAGAIN)

Sashiko says:
---
This isn't a bug, but should the variable err be declared as an int?
The variable err is declared at the beginning of this loop as a u32, but it
receives the return value of gve_adminq_parse_err() which returns signed int
error codes such as -EAGAIN.

When checking err != -EAGAIN here, the code compares an unsigned 32-bit
integer to a negative constant. While this may evaluate correctly due to two's
complement wrap-around, it can trigger compiler warnings for signed and
unsigned integer comparisons.
---

Yes, it should be an int, will fix in v7.

> > +                             dev_err_ratelimited(&priv->pdev->dev,
> > +                                                 "AQ command %d failed with status %d\n",
> > +                                                 opcode, status);
> > +
> >                       // Return the first error if we failed.
> >                       return err;
> > +             }
> >       }
> >
> >       return 0;
>

[PATCH net-next v6 2/3] gve: make nic clock reads thread safe

[Harshitha Ramamurthy]

From: Ankit Garg <nktgrg@google.com>

Add a mutex to protect the shared DMA buffer that receives NIC
timestamp reports. The NIC timestamp will be read from two different
threads: the periodic worker and upcoming `gettimex64`.

Move clock registration to the last step of initialization to ensure
that all data needed by the clock module is initialized before
the clock is exposed to usermode.

Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Ankit Garg <nktgrg@google.com>
Signed-off-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
Changes in v3:
- Reorder init/teardown to register PTP clock last, and simplify code
- Move ptp-related members from gve_priv to gve_ptp
- Only assign priv->ptp after ptp module is successfully initialized
---
 drivers/net/ethernet/google/gve/gve.h         |  12 +-
 drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +-
 drivers/net/ethernet/google/gve/gve_ptp.c     | 134 ++++++++----------
 3 files changed, 63 insertions(+), 86 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 1d66d3834f7e..7b69d0cfc0d5 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -792,6 +792,9 @@ struct gve_ptp {
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
 	struct gve_priv *priv;
+	struct mutex nic_ts_read_lock; /* Protects nic_ts_report */
+	struct gve_nic_ts_report *nic_ts_report;
+	dma_addr_t nic_ts_report_bus;
 };
 
 struct gve_priv {
@@ -923,8 +926,6 @@ struct gve_priv {
 	bool nic_timestamp_supported;
 	struct gve_ptp *ptp;
 	struct kernel_hwtstamp_config ts_config;
-	struct gve_nic_ts_report *nic_ts_report;
-	dma_addr_t nic_ts_report_bus;
 	u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
 };
 
@@ -1201,7 +1202,7 @@ static inline bool gve_supports_xdp_xmit(struct gve_priv *priv)
 
 static inline bool gve_is_clock_enabled(struct gve_priv *priv)
 {
-	return priv->nic_ts_report;
+	return priv->ptp;
 }
 
 /* gqi napi handler defined in gve_main.c */
@@ -1321,14 +1322,9 @@ int gve_flow_rules_reset(struct gve_priv *priv);
 int gve_init_rss_config(struct gve_priv *priv, u16 num_queues);
 /* PTP and timestamping */
 #if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
-int gve_clock_nic_ts_read(struct gve_priv *priv);
 int gve_init_clock(struct gve_priv *priv);
 void gve_teardown_clock(struct gve_priv *priv);
 #else /* CONFIG_PTP_1588_CLOCK */
-static inline int gve_clock_nic_ts_read(struct gve_priv *priv)
-{
-	return -EOPNOTSUPP;
-}
 
 static inline int gve_init_clock(struct gve_priv *priv)
 {
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index dc2213b5ce24..4fd7e8a442c5 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -972,8 +972,7 @@ static int gve_get_ts_info(struct net_device *netdev,
 		info->rx_filters |= BIT(HWTSTAMP_FILTER_NONE) |
 				    BIT(HWTSTAMP_FILTER_ALL);
 
-		if (priv->ptp)
-			info->phc_index = ptp_clock_index(priv->ptp->clock);
+		info->phc_index = ptp_clock_index(priv->ptp->clock);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/google/gve/gve_ptp.c b/drivers/net/ethernet/google/gve/gve_ptp.c
index 06b1cf4a5efc..ad15f1209a83 100644
--- a/drivers/net/ethernet/google/gve/gve_ptp.c
+++ b/drivers/net/ethernet/google/gve/gve_ptp.c
@@ -11,19 +11,20 @@
 #define GVE_NIC_TS_SYNC_INTERVAL_MS 250
 
 /* Read the nic timestamp from hardware via the admin queue. */
-int gve_clock_nic_ts_read(struct gve_priv *priv)
+static int gve_clock_nic_ts_read(struct gve_ptp *ptp, u64 *nic_raw)
 {
-	u64 nic_raw;
 	int err;
 
-	err = gve_adminq_report_nic_ts(priv, priv->nic_ts_report_bus);
+	mutex_lock(&ptp->nic_ts_read_lock);
+	err = gve_adminq_report_nic_ts(ptp->priv, ptp->nic_ts_report_bus);
 	if (err)
-		return err;
+		goto out;
 
-	nic_raw = be64_to_cpu(priv->nic_ts_report->nic_timestamp);
-	WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
+	*nic_raw = be64_to_cpu(ptp->nic_ts_report->nic_timestamp);
 
-	return 0;
+out:
+	mutex_unlock(&ptp->nic_ts_read_lock);
+	return err;
 }
 
 static int gve_ptp_gettimex64(struct ptp_clock_info *info,
@@ -41,17 +42,21 @@ static int gve_ptp_settime64(struct ptp_clock_info *info,
 
 static long gve_ptp_do_aux_work(struct ptp_clock_info *info)
 {
-	const struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
+	struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
 	struct gve_priv *priv = ptp->priv;
+	u64 nic_raw;
 	int err;
 
 	if (gve_get_reset_in_progress(priv) || !gve_get_admin_queue_ok(priv))
 		goto out;
 
-	err = gve_clock_nic_ts_read(priv);
-	if (err && net_ratelimit())
-		dev_err(&priv->pdev->dev,
-			"%s read err %d\n", __func__, err);
+	err = gve_clock_nic_ts_read(ptp, &nic_raw);
+	if (err) {
+		dev_err_ratelimited(&priv->pdev->dev, "%s read err %d\n",
+				    __func__, err);
+		goto out;
+	}
+	WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
 
 out:
 	return msecs_to_jiffies(GVE_NIC_TS_SYNC_INTERVAL_MS);
@@ -65,94 +70,71 @@ static const struct ptp_clock_info gve_ptp_caps = {
 	.do_aux_work	= gve_ptp_do_aux_work,
 };
 
-static int gve_ptp_init(struct gve_priv *priv)
+int gve_init_clock(struct gve_priv *priv)
 {
 	struct gve_ptp *ptp;
+	u64 nic_raw;
 	int err;
 
-	priv->ptp = kzalloc_obj(*priv->ptp);
-	if (!priv->ptp)
+	ptp = kzalloc_obj(*priv->ptp);
+	if (!ptp)
 		return -ENOMEM;
 
-	ptp = priv->ptp;
 	ptp->info = gve_ptp_caps;
-	ptp->clock = ptp_clock_register(&ptp->info, &priv->pdev->dev);
-
-	if (IS_ERR(ptp->clock)) {
-		dev_err(&priv->pdev->dev, "PTP clock registration failed\n");
-		err  = PTR_ERR(ptp->clock);
-		goto free_ptp;
-	}
-
 	ptp->priv = priv;
-	return 0;
-
-free_ptp:
-	kfree(ptp);
-	priv->ptp = NULL;
-	return err;
-}
-
-static void gve_ptp_release(struct gve_priv *priv)
-{
-	struct gve_ptp *ptp = priv->ptp;
-
-	if (!ptp)
-		return;
-
-	if (ptp->clock)
-		ptp_clock_unregister(ptp->clock);
-
-	kfree(ptp);
-	priv->ptp = NULL;
-}
-
-int gve_init_clock(struct gve_priv *priv)
-{
-	int err;
-
-	err = gve_ptp_init(priv);
-	if (err)
-		return err;
-
-	priv->nic_ts_report =
+	mutex_init(&ptp->nic_ts_read_lock);
+	ptp->nic_ts_report =
 		dma_alloc_coherent(&priv->pdev->dev,
 				   sizeof(struct gve_nic_ts_report),
-				   &priv->nic_ts_report_bus,
-				   GFP_KERNEL);
-	if (!priv->nic_ts_report) {
+				   &ptp->nic_ts_report_bus, GFP_KERNEL);
+	if (!ptp->nic_ts_report) {
 		dev_err(&priv->pdev->dev, "%s dma alloc error\n", __func__);
 		err = -ENOMEM;
-		goto release_ptp;
+		goto free_ptp;
 	}
-	err = gve_clock_nic_ts_read(priv);
+
+	err = gve_clock_nic_ts_read(ptp, &nic_raw);
 	if (err) {
 		dev_err(&priv->pdev->dev, "failed to read NIC clock %d\n", err);
-		goto release_nic_ts_report;
+		goto free_dma_mem;
 	}
-	ptp_schedule_worker(priv->ptp->clock,
+	WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
+
+	ptp->clock = ptp_clock_register(&ptp->info, &priv->pdev->dev);
+	if (IS_ERR(ptp->clock)) {
+		dev_err(&priv->pdev->dev, "PTP clock registration failed\n");
+		err = PTR_ERR(ptp->clock);
+		goto free_dma_mem;
+	}
+
+	priv->ptp = ptp;
+	ptp_schedule_worker(ptp->clock,
 			    msecs_to_jiffies(GVE_NIC_TS_SYNC_INTERVAL_MS));
 
 	return 0;
 
-release_nic_ts_report:
-	dma_free_coherent(&priv->pdev->dev,
-			  sizeof(struct gve_nic_ts_report),
-			  priv->nic_ts_report, priv->nic_ts_report_bus);
-	priv->nic_ts_report = NULL;
-release_ptp:
-	gve_ptp_release(priv);
+free_dma_mem:
+	dma_free_coherent(&priv->pdev->dev, sizeof(struct gve_nic_ts_report),
+			  ptp->nic_ts_report, ptp->nic_ts_report_bus);
+	ptp->nic_ts_report = NULL;
+free_ptp:
+	mutex_destroy(&ptp->nic_ts_read_lock);
+	kfree(ptp);
 	return err;
 }
 
 void gve_teardown_clock(struct gve_priv *priv)
 {
-	gve_ptp_release(priv);
+	struct gve_ptp *ptp = priv->ptp;
 
-	if (priv->nic_ts_report) {
-		dma_free_coherent(&priv->pdev->dev,
-				  sizeof(struct gve_nic_ts_report),
-				  priv->nic_ts_report, priv->nic_ts_report_bus);
-		priv->nic_ts_report = NULL;
-	}
+	if (!ptp)
+		return;
+
+	priv->ptp = NULL;
+	ptp_clock_unregister(ptp->clock);
+	dma_free_coherent(&priv->pdev->dev, sizeof(struct gve_nic_ts_report),
+			  ptp->nic_ts_report, ptp->nic_ts_report_bus);
+	ptp->nic_ts_report = NULL;
+	mutex_destroy(&ptp->nic_ts_read_lock);
+	kfree(ptp);
 }
-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH net-next v6 2/3] gve: make nic clock reads thread safe

[Jacob Keller]

On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> From: Ankit Garg <nktgrg@google.com>
> 
> Add a mutex to protect the shared DMA buffer that receives NIC
> timestamp reports. The NIC timestamp will be read from two different
> threads: the periodic worker and upcoming `gettimex64`.
> 
> Move clock registration to the last step of initialization to ensure
> that all data needed by the clock module is initialized before
> the clock is exposed to usermode.
> 
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Ankit Garg <nktgrg@google.com>
> Signed-off-by: Jordan Rhee <jordanrhee@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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

[Harshitha Ramamurthy]

From: Jordan Rhee <jordanrhee@google.com>

Enable chrony and phc2sys to synchronize system clock to NIC clock.

Two paths are implemented: a precise path using system counter values
sampled by the device, and a fallback path using system counter values
sampled in the driver using ptp_read_system_prets()/postts().

To use the precise path, the current system clocksource must match the
units returned by the device, which on x86 is X86_TSC and on ARM64 is
ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
by default on GCP VMs using Chrony, so we expect the precise path to be
used the vast majority of the time. If the system clocksource is changed
to kvm-clock, it activates the fallback path. Ethtool counters have been
added to count how many times each path is used.

The uncertainty window in the precise path is typically around 1-2us,
while in the fallback path is around 60-80us.

Stub implementions of adjfine and adjtime are added to avoid NULL
dereference when phc2sys tries to adjust the clock.

Cc: John Stultz <jstultz@google.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
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>
---
Changes in v6:
- Added a fallback to driver-sampled time sandwich that is used when
  the following conditions are not met:
  - The system clock source is X86_TSC or ARM_ARCH_COUNTER
  - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
  - The architecture is x86 or ARM64
- Added ethtool statistics to count how many cross-timestamps used the
  precise path versus fallback path.
- Fixed printf format specifier.
- Added stub implementions of adjtime and adjfine to prevent NULL
  dereference when phc2sys tries to adjust clock.
- Moved system time snapshot back to gve_ptp_gettimex64() so we can get the
  current system clock source from it. It is OK for it to not be inside
  the mutex or retry loop because lock contention and retries should be
  extremely rare, and chrony filters out bad samples.

Changes in v5:
- Reformulate retry loop in terms of total timeout (Jakub Kicinski)

Changes in v3:
- Take system time snapshot inside the mutex
- Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
  than x86 or arm64

Changes in v2:
 - fix compilation warning on ARM by casting cycles_t to u64
---
 drivers/net/ethernet/google/gve/gve.h         |   8 +
 drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
 drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +
 drivers/net/ethernet/google/gve/gve_ptp.c     | 237 +++++++++++++++++-
 4 files changed, 243 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 7b69d0cfc0d5..4de3ce60060e 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -880,6 +880,14 @@ struct gve_priv {
 	u32 stats_report_trigger_cnt; /* count of device-requested stats-reports since last reset */
 	u32 suspend_cnt; /* count of times suspended */
 	u32 resume_cnt; /* count of times resumed */
+	/* count of cross-timestamps attempted using system timestamps
+	 * from the AQ command
+	 */
+	u32 ptp_precise_xtstamps;
+	/* count of cross-timestamps attempted using system timestamps sampled
+	 * by the driver
+	 */
+	u32 ptp_fallback_xtstamps;
 	struct workqueue_struct *gve_wq;
 	struct work_struct service_task;
 	struct work_struct stats_report_task;
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 22a74b6aa17e..e6dcf6da9091 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -411,8 +411,8 @@ static_assert(sizeof(struct gve_adminq_report_nic_ts) == 16);
 
 struct gve_nic_ts_report {
 	__be64 nic_timestamp; /* NIC clock in nanoseconds */
-	__be64 reserved1;
-	__be64 reserved2;
+	__be64 pre_cycles; /* System cycle counter before NIC clock read */
+	__be64 post_cycles; /* System cycle counter after NIC clock read */
 	__be64 reserved3;
 	__be64 reserved4;
 };
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 4fd7e8a442c5..8a088dcc3603 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -46,6 +46,7 @@ static const char gve_gstrings_main_stats[][ETH_GSTRING_LEN] = {
 	"rx_hsplit_unsplit_pkt",
 	"interface_up_cnt", "interface_down_cnt", "reset_cnt",
 	"page_alloc_fail", "dma_mapping_error", "stats_report_trigger_cnt",
+	"ptp_precise_xtstamps", "ptp_fallback_xtstamps",
 };
 
 static const char gve_gstrings_rx_stats[][ETH_GSTRING_LEN] = {
@@ -269,6 +270,8 @@ gve_get_ethtool_stats(struct net_device *netdev,
 	data[i++] = priv->page_alloc_fail;
 	data[i++] = priv->dma_mapping_error;
 	data[i++] = priv->stats_report_trigger_cnt;
+	data[i++] = priv->ptp_precise_xtstamps;
+	data[i++] = priv->ptp_fallback_xtstamps;
 	i = GVE_MAIN_STATS_LEN;
 
 	rx_base_stats_idx = 0;
diff --git a/drivers/net/ethernet/google/gve/gve_ptp.c b/drivers/net/ethernet/google/gve/gve_ptp.c
index ad15f1209a83..9711521ce1d0 100644
--- a/drivers/net/ethernet/google/gve/gve_ptp.c
+++ b/drivers/net/ethernet/google/gve/gve_ptp.c
@@ -10,28 +10,251 @@
 /* Interval to schedule a nic timestamp calibration, 250ms. */
 #define GVE_NIC_TS_SYNC_INTERVAL_MS 250
 
+/*
+ * Stores cycle counter samples in get_cycles() units from a
+ * sandwiched NIC clock read
+ */
+struct gve_sysclock_sample {
+	/* Cycle counter from NIC before clock read */
+	u64 nic_pre_cycles;
+	/* Cycle counter from NIC after clock read */
+	u64 nic_post_cycles;
+	/* Cycle counter from host before issuing AQ command */
+	cycles_t host_pre_cycles;
+	/* Cycle counter from host after AQ command returns */
+	cycles_t host_post_cycles;
+};
+
+/*
+ * Read NIC clock by issuing the AQ command. The command is subject to
+ * rate limiting and may need to be retried. Requires nic_ts_read_lock
+ * to be held.
+ */
+static int gve_ptp_read_timestamp(struct gve_ptp *ptp, cycles_t *pre_cycles,
+				  cycles_t *post_cycles)
+{
+	unsigned long deadline = jiffies + msecs_to_jiffies(100);
+	unsigned long delay_us = 1000;
+	int err;
+
+	lockdep_assert_held(&ptp->nic_ts_read_lock);
+
+	do {
+		*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;
+	} while (time_before(jiffies, deadline));
+
+	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);
+	if (err) {
+		dev_err_ratelimited(&ptp->priv->pdev->dev,
+				    "AdminQ timestamp read failed: %d\n", err);
 		goto out;
+	}
+
+	ts_report = ptp->nic_ts_report;
+	*nic_raw = be64_to_cpu(ts_report->nic_timestamp);
 
-	*nic_raw = be64_to_cpu(ptp->nic_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)
+{
+	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;
+}
+
+/*
+ * 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 %llu cycles to system time: %d\n",
+				    cycles,
+				    err);
+		return err;
+	}
+
+	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;
+}
+
+static bool
+gve_can_use_system_ts_from_device(enum clocksource_ids system_clock_source,
+				  clockid_t clockid)
+{
+	if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC_RAW)
+		return false;
+
+	/* If the system clock source matches the system clock
+	 * returned by the AdminQ command, we can use the system
+	 * timestamps returned by the device, otherwise we have to
+	 * fall back to sampling system time from the host which
+	 * is less accurate.
+	 */
+	if (IS_ENABLED(CONFIG_X86))
+		return system_clock_source == CSID_X86_TSC;
+	else if (IS_ENABLED(CONFIG_ARM64))
+		return system_clock_source == CSID_ARM_ARCH_COUNTER;
+
+	return false;
+}
+
 static int gve_ptp_gettimex64(struct ptp_clock_info *info,
 			      struct timespec64 *ts,
 			      struct ptp_system_timestamp *sts)
 {
-	return -EOPNOTSUPP;
+	struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
+	struct gve_sysclock_sample sysclock = {0};
+	bool use_system_ts_from_device = false;
+	struct gve_priv *priv = ptp->priv;
+	struct system_time_snapshot snap;
+	u64 nic_ts;
+	int err;
+
+	if (sts) {
+		/* This snapshot is used both to query the current system
+		 * clocksource and to convert the cycle counts returned
+		 * by the AdminQ command to ktime. It does not need to be
+		 * taken inside the retry loop because retries and lock
+		 * contention are expected to be extremely rare.
+		 *
+		 * If the system clock source changes between here and
+		 * when get_device_system_crosststamp() is called,
+		 * get_device_system_crosststamp() will fail which will
+		 * cause one failed sample, and the next one will succeed.
+		 */
+		ktime_get_snapshot(&snap);
+		use_system_ts_from_device =
+			gve_can_use_system_ts_from_device(snap.cs_id,
+							  sts->clockid);
+		if (use_system_ts_from_device)
+			priv->ptp_precise_xtstamps++;
+		else
+			priv->ptp_fallback_xtstamps++;
+	}
+
+	if (unlikely(!use_system_ts_from_device))
+		ptp_read_system_prets(sts);
+
+	err = gve_clock_nic_ts_read(ptp, &nic_ts, sts ? &sysclock : NULL);
+	if (err)
+		return err;
+
+	if (unlikely(!use_system_ts_from_device))
+		ptp_read_system_postts(sts);
+
+	if (sts && likely(use_system_ts_from_device)) {
+		/* Reject samples with out of order system clock values.
+		 * Firmware must return valid non-zero cycle counts.
+		 */
+		if (!(sysclock.host_pre_cycles <= sysclock.nic_pre_cycles &&
+		      sysclock.nic_pre_cycles  <= sysclock.nic_post_cycles &&
+		      sysclock.nic_post_cycles <= sysclock.host_post_cycles)) {
+			dev_err_ratelimited(&priv->pdev->dev,
+					    "AdminQ system clock cycle counts out of order. Expecting %llu <= %llu <= %llu <= %llu\n",
+					    (u64)sysclock.host_pre_cycles,
+					    sysclock.nic_pre_cycles,
+					    sysclock.nic_post_cycles,
+					    (u64)sysclock.host_post_cycles);
+			return -EBADMSG;
+		}
+
+		err = gve_cycles_to_timespec64(priv, sts->clockid, &snap,
+					       sysclock.nic_pre_cycles,
+					       &sts->pre_ts);
+		if (err)
+			return err;
+
+		err = gve_cycles_to_timespec64(priv, sts->clockid, &snap,
+					       sysclock.nic_post_cycles,
+					       &sts->post_ts);
+		if (err)
+			return err;
+	}
+
+	*ts = ns_to_timespec64(nic_ts);
+
+	return 0;
 }
 
 static int gve_ptp_settime64(struct ptp_clock_info *info,
@@ -50,7 +273,7 @@ static long gve_ptp_do_aux_work(struct ptp_clock_info *info)
 	if (gve_get_reset_in_progress(priv) || !gve_get_admin_queue_ok(priv))
 		goto out;
 
-	err = gve_clock_nic_ts_read(ptp, &nic_raw);
+	err = gve_clock_nic_ts_read(ptp, &nic_raw, NULL);
 	if (err) {
 		dev_err_ratelimited(&priv->pdev->dev, "%s read err %d\n",
 				    __func__, err);
@@ -93,7 +316,7 @@ int gve_init_clock(struct gve_priv *priv)
 		goto free_ptp;
 	}
 
-	err = gve_clock_nic_ts_read(ptp, &nic_raw);
+	err = gve_clock_nic_ts_read(ptp, &nic_raw, NULL);
 	if (err) {
 		dev_err(&priv->pdev->dev, "failed to read NIC clock %d\n", err);
 		goto free_dma_mem;
-- 
2.54.0.545.g6539524ca2-goog

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

[Jacob Keller]

On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> From: Jordan Rhee <jordanrhee@google.com>
> 
> Enable chrony and phc2sys to synchronize system clock to NIC clock.
> 
> Two paths are implemented: a precise path using system counter values
> sampled by the device, and a fallback path using system counter values
> sampled in the driver using ptp_read_system_prets()/postts().
> 
> To use the precise path, the current system clocksource must match the
> units returned by the device, which on x86 is X86_TSC and on ARM64 is
> ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
> be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
> by default on GCP VMs using Chrony, so we expect the precise path to be
> used the vast majority of the time. If the system clocksource is changed
> to kvm-clock, it activates the fallback path. Ethtool counters have been
> added to count how many times each path is used.
> 
> The uncertainty window in the precise path is typically around 1-2us,
> while in the fallback path is around 60-80us.
> 
> Stub implementions of adjfine and adjtime are added to avoid NULL
> dereference when phc2sys tries to adjust the clock.
> 
> Cc: John Stultz <jstultz@google.com>
> Cc: Thomas Gleixner <tglx@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> 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>
> ---
> Changes in v6:
> - Added a fallback to driver-sampled time sandwich that is used when
>   the following conditions are not met:
>   - The system clock source is X86_TSC or ARM_ARCH_COUNTER
>   - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
>   - The architecture is x86 or ARM64
> - Added ethtool statistics to count how many cross-timestamps used the
>   precise path versus fallback path.
> - Fixed printf format specifier.
> - Added stub implementions of adjtime and adjfine to prevent NULL
>   dereference when phc2sys tries to adjust clock.

All excellent improvements.

> - Moved system time snapshot back to gve_ptp_gettimex64() so we can get the
>   current system clock source from it. It is OK for it to not be inside
>   the mutex or retry loop because lock contention and retries should be
>   extremely rare, and chrony filters out bad samples.
> 

I'm a bit worried about this part, but if I understand, it would only
affect the fallback variant anyways since the clock samples are taken in
the host in the precise/fast path.

> Changes in v5:
> - Reformulate retry loop in terms of total timeout (Jakub Kicinski)
> 
> Changes in v3:
> - Take system time snapshot inside the mutex
> - Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
>   than x86 or arm64
> 
> Changes in v2:
>  - fix compilation warning on ARM by casting cycles_t to u64
> ---
>  drivers/net/ethernet/google/gve/gve.h         |   8 +
>  drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
>  drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +
>  drivers/net/ethernet/google/gve/gve_ptp.c     | 237 +++++++++++++++++-
>  4 files changed, 243 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 7b69d0cfc0d5..4de3ce60060e 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -880,6 +880,14 @@ struct gve_priv {
>  	u32 stats_report_trigger_cnt; /* count of device-requested stats-reports since last reset */
>  	u32 suspend_cnt; /* count of times suspended */
>  	u32 resume_cnt; /* count of times resumed */
> +	/* count of cross-timestamps attempted using system timestamps
> +	 * from the AQ command
> +	 */
> +	u32 ptp_precise_xtstamps;
> +	/* count of cross-timestamps attempted using system timestamps sampled
> +	 * by the driver
> +	 */

Helpful for analyzing when its not working. Nice!

> +static int gve_cycles_to_clock_fn(ktime_t *device_time,
> +				  struct system_counterval_t *system_counterval,
> +				  void *ctx)
> +{
> +	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

No single kernel can be compiled for both CONFIG_X86 *and* CONFIG_ARM64
simultaneously, so there is no need to check in sequence and an
exclusive if/else path is fine. Ok.

> +
> +static bool
> +gve_can_use_system_ts_from_device(enum clocksource_ids system_clock_source,
> +				  clockid_t clockid)
> +{
> +	if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC_RAW)
> +		return false;
> +
> +	/* If the system clock source matches the system clock
> +	 * returned by the AdminQ command, we can use the system
> +	 * timestamps returned by the device, otherwise we have to
> +	 * fall back to sampling system time from the host which
> +	 * is less accurate.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86))
> +		return system_clock_source == CSID_X86_TSC;
> +	else if (IS_ENABLED(CONFIG_ARM64))
> +		return system_clock_source == CSID_ARM_ARCH_COUNTER;
> +

This feels a bit duplicated to me, since you have the same check
elsewhere. Would it make sense to pull this to its own function check?
Its short enough I think that is only warranted if you have a true
(non-nit) cause to make a v7.

Thus:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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

[Jordan Rhee]

On Thu, May 7, 2026 at 2:23 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> > From: Jordan Rhee <jordanrhee@google.com>
> >
> > Enable chrony and phc2sys to synchronize system clock to NIC clock.
> >
> > Two paths are implemented: a precise path using system counter values
> > sampled by the device, and a fallback path using system counter values
> > sampled in the driver using ptp_read_system_prets()/postts().
> >
> > To use the precise path, the current system clocksource must match the
> > units returned by the device, which on x86 is X86_TSC and on ARM64 is
> > ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
> > be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
> > by default on GCP VMs using Chrony, so we expect the precise path to be
> > used the vast majority of the time. If the system clocksource is changed
> > to kvm-clock, it activates the fallback path. Ethtool counters have been
> > added to count how many times each path is used.
> >
> > The uncertainty window in the precise path is typically around 1-2us,
> > while in the fallback path is around 60-80us.
> >
> > Stub implementions of adjfine and adjtime are added to avoid NULL
> > dereference when phc2sys tries to adjust the clock.
> >
> > Cc: John Stultz <jstultz@google.com>
> > Cc: Thomas Gleixner <tglx@kernel.org>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > 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>
> > ---
> > Changes in v6:
> > - Added a fallback to driver-sampled time sandwich that is used when
> >   the following conditions are not met:
> >   - The system clock source is X86_TSC or ARM_ARCH_COUNTER
> >   - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
> >   - The architecture is x86 or ARM64
> > - Added ethtool statistics to count how many cross-timestamps used the
> >   precise path versus fallback path.
> > - Fixed printf format specifier.
> > - Added stub implementions of adjtime and adjfine to prevent NULL
> >   dereference when phc2sys tries to adjust clock.
>
> All excellent improvements.

Thank you for the review!

>
> > - Moved system time snapshot back to gve_ptp_gettimex64() so we can get the
> >   current system clock source from it. It is OK for it to not be inside
> >   the mutex or retry loop because lock contention and retries should be
> >   extremely rare, and chrony filters out bad samples.
> >
>
> I'm a bit worried about this part, but if I understand, it would only
> affect the fallback variant anyways since the clock samples are taken in
> the host in the precise/fast path.

Good point, the snapshot *is* used in the precise path to interpolate
system timestamps that occur before the current timekeeping interval.
This is my understanding of how historical timestamp interpolation
works (adjust_historical_crosststamp()).

        <--      total_history_cycles                      -->
                               <-- partial_history_cycles -->
-------|------------------+------------------------------------|----------------+---------->
 cycles (input)
  snapshot         sample                               cur_interval        now
-------|------------------+------------------------------------|----------------+---------->
 ktime (output)
        <--                      corr_raw                        -->

At the start of each timekeeping interval, both the current cycle
count and ktime are recorded. Each interval boundary consists of a
(cycle count, ktime) pair.

It computes the ktime corresponding to sampled cycle count as corr_raw
* partial_history_cycles / total_history_cycles. In other words, it
scales the distance between the interval boundaries in ktime units by
the relative position of the sample in the interval computed using the
cycle counts. It adds or subtracts this value to whichever interval
boundary is closer to get the absolute ktime.

How would this be affected by a very old snapshot? The interpolation
would still work, but it may be less accurate if the clock is not
stable. To get a sense of the timescales,
 - the timekeeping interval is typically 4ms (CONFIG_HZ=250).
 - the AQ command typically takes 60-80us.
 - there are 2 sources of contention on the mutex (aux task and gettimex64)
 - worst case retry loop is 100ms (but this should not happen due to
how the firmware rate limiter has been tuned)
 - typical clock stability for the relevant cloud server platforms is
about 10ppm

The vast majority of the time, the sample should fall in the current
or previous timekeeping interval, resulting in the most accurate
interpolation possible.

In the worst case for retries and contention, the snapshot could be up
to 200ms before the current timekeeping interval. Assuming clock
stability of 10ppm, this translates to 10us of drift per 1s, or 2us
per 200ms. This is still considerably better than the root dispersion
in the fallback path (81us), so I think it's OK.

>
> > Changes in v5:
> > - Reformulate retry loop in terms of total timeout (Jakub Kicinski)
> >
> > Changes in v3:
> > - Take system time snapshot inside the mutex
> > - Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
> >   than x86 or arm64
> >
> > Changes in v2:
> >  - fix compilation warning on ARM by casting cycles_t to u64
> > ---
> >  drivers/net/ethernet/google/gve/gve.h         |   8 +
> >  drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
> >  drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +
> >  drivers/net/ethernet/google/gve/gve_ptp.c     | 237 +++++++++++++++++-
> >  4 files changed, 243 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index 7b69d0cfc0d5..4de3ce60060e 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -880,6 +880,14 @@ struct gve_priv {
> >       u32 stats_report_trigger_cnt; /* count of device-requested stats-reports since last reset */
> >       u32 suspend_cnt; /* count of times suspended */
> >       u32 resume_cnt; /* count of times resumed */
> > +     /* count of cross-timestamps attempted using system timestamps
> > +      * from the AQ command
> > +      */
> > +     u32 ptp_precise_xtstamps;
> > +     /* count of cross-timestamps attempted using system timestamps sampled
> > +      * by the driver
> > +      */
>
> Helpful for analyzing when its not working. Nice!
>
> > +static int gve_cycles_to_clock_fn(ktime_t *device_time,
> > +                               struct system_counterval_t *system_counterval,
> > +                               void *ctx)
> > +{
> > +     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
>
> No single kernel can be compiled for both CONFIG_X86 *and* CONFIG_ARM64
> simultaneously, so there is no need to check in sequence and an
> exclusive if/else path is fine. Ok.

Noted, thanks.

>
> > +
> > +static bool
> > +gve_can_use_system_ts_from_device(enum clocksource_ids system_clock_source,
> > +                               clockid_t clockid)
> > +{
> > +     if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC_RAW)
> > +             return false;
> > +
> > +     /* If the system clock source matches the system clock
> > +      * returned by the AdminQ command, we can use the system
> > +      * timestamps returned by the device, otherwise we have to
> > +      * fall back to sampling system time from the host which
> > +      * is less accurate.
> > +      */
> > +     if (IS_ENABLED(CONFIG_X86))
> > +             return system_clock_source == CSID_X86_TSC;
> > +     else if (IS_ENABLED(CONFIG_ARM64))
> > +             return system_clock_source == CSID_ARM_ARCH_COUNTER;
> > +
>
> This feels a bit duplicated to me, since you have the same check
> elsewhere. Would it make sense to pull this to its own function check?
> Its short enough I think that is only warranted if you have a true
> (non-nit) cause to make a v7.

There is a similar check in gve_cycles_to_clock_fn, but the logic
inside the if blocks is different so I'm not sure how they could share
code.

>
> Thus:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thank you very much, appreciate it.

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

[Jacob Keller]

On 5/8/2026 10:43 AM, Jordan Rhee wrote:
> On Thu, May 7, 2026 at 2:23 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
>>
>> I'm a bit worried about this part, but if I understand, it would only
>> affect the fallback variant anyways since the clock samples are taken in
>> the host in the precise/fast path.
> 
> Good point, the snapshot *is* used in the precise path to interpolate
> system timestamps that occur before the current timekeeping interval.
> This is my understanding of how historical timestamp interpolation
> works (adjust_historical_crosststamp()).
> 
>         <--      total_history_cycles                      -->
>                                <-- partial_history_cycles -->
> -------|------------------+------------------------------------|----------------+---------->
>  cycles (input)
>   snapshot         sample                               cur_interval        now
> -------|------------------+------------------------------------|----------------+---------->
>  ktime (output)
>         <--                      corr_raw                        -->
> 
> At the start of each timekeeping interval, both the current cycle
> count and ktime are recorded. Each interval boundary consists of a
> (cycle count, ktime) pair.
> 
> It computes the ktime corresponding to sampled cycle count as corr_raw
> * partial_history_cycles / total_history_cycles. In other words, it
> scales the distance between the interval boundaries in ktime units by
> the relative position of the sample in the interval computed using the
> cycle counts. It adds or subtracts this value to whichever interval
> boundary is closer to get the absolute ktime.
> 
> How would this be affected by a very old snapshot? The interpolation
> would still work, but it may be less accurate if the clock is not
> stable. To get a sense of the timescales,
>  - the timekeeping interval is typically 4ms (CONFIG_HZ=250).
>  - the AQ command typically takes 60-80us.
>  - there are 2 sources of contention on the mutex (aux task and gettimex64)
>  - worst case retry loop is 100ms (but this should not happen due to
> how the firmware rate limiter has been tuned)
>  - typical clock stability for the relevant cloud server platforms is
> about 10ppm
> 
> The vast majority of the time, the sample should fall in the current
> or previous timekeeping interval, resulting in the most accurate
> interpolation possible.
> 
> In the worst case for retries and contention, the snapshot could be up
> to 200ms before the current timekeeping interval. Assuming clock
> stability of 10ppm, this translates to 10us of drift per 1s, or 2us
> per 200ms. This is still considerably better than the root dispersion
> in the fallback path (81us), so I think it's OK.
> 
Excellent analysis, and yes I agree that its acceptable.

Thanks,
Jake

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

[Jordan Rhee]

On Thu, May 7, 2026 at 2:13 PM Harshitha Ramamurthy
<hramamurthy@google.com> wrote:
>
> From: Jordan Rhee <jordanrhee@google.com>
>
> Enable chrony and phc2sys to synchronize system clock to NIC clock.
>
> Two paths are implemented: a precise path using system counter values
> sampled by the device, and a fallback path using system counter values
> sampled in the driver using ptp_read_system_prets()/postts().
>
> To use the precise path, the current system clocksource must match the
> units returned by the device, which on x86 is X86_TSC and on ARM64 is
> ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
> be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
> by default on GCP VMs using Chrony, so we expect the precise path to be
> used the vast majority of the time. If the system clocksource is changed
> to kvm-clock, it activates the fallback path. Ethtool counters have been
> added to count how many times each path is used.
>
> The uncertainty window in the precise path is typically around 1-2us,
> while in the fallback path is around 60-80us.
>
> Stub implementions of adjfine and adjtime are added to avoid NULL
> dereference when phc2sys tries to adjust the clock.

Sashiko says:
---
Is this description accurate? The patch only implements gve_ptp_settime64
and fails to include the adjtime and adjfine callbacks.

When userspace tools like phc2sys attempt to adjust the PTP clock frequency
or offset, the PTP subsystem core (ptp_clock_adjtime) will invoke these
missing callbacks unconditionally. Could this lead to a NULL pointer
dereference and kernel panic?
---

I made a mistake and did not commit my local changes. I will send a v7.

>
> Cc: John Stultz <jstultz@google.com>
> Cc: Thomas Gleixner <tglx@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> 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>
> ---
> Changes in v6:
> - Added a fallback to driver-sampled time sandwich that is used when
>   the following conditions are not met:
>   - The system clock source is X86_TSC or ARM_ARCH_COUNTER
>   - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
>   - The architecture is x86 or ARM64
> - Added ethtool statistics to count how many cross-timestamps used the
>   precise path versus fallback path.
> - Fixed printf format specifier.
> - Added stub implementions of adjtime and adjfine to prevent NULL
>   dereference when phc2sys tries to adjust clock.
> - Moved system time snapshot back to gve_ptp_gettimex64() so we can get the
>   current system clock source from it. It is OK for it to not be inside
>   the mutex or retry loop because lock contention and retries should be
>   extremely rare, and chrony filters out bad samples.
>
> Changes in v5:
> - Reformulate retry loop in terms of total timeout (Jakub Kicinski)
>
> Changes in v3:
> - Take system time snapshot inside the mutex
> - Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
>   than x86 or arm64
>
> Changes in v2:
>  - fix compilation warning on ARM by casting cycles_t to u64
> ---
>  drivers/net/ethernet/google/gve/gve.h         |   8 +
>  drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
>  drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +
>  drivers/net/ethernet/google/gve/gve_ptp.c     | 237 +++++++++++++++++-
>  4 files changed, 243 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 7b69d0cfc0d5..4de3ce60060e 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -880,6 +880,14 @@ struct gve_priv {
>         u32 stats_report_trigger_cnt; /* count of device-requested stats-reports since last reset */
>         u32 suspend_cnt; /* count of times suspended */
>         u32 resume_cnt; /* count of times resumed */
> +       /* count of cross-timestamps attempted using system timestamps
> +        * from the AQ command
> +        */
> +       u32 ptp_precise_xtstamps;
> +       /* count of cross-timestamps attempted using system timestamps sampled
> +        * by the driver
> +        */
> +       u32 ptp_fallback_xtstamps;
>         struct workqueue_struct *gve_wq;
>         struct work_struct service_task;
>         struct work_struct stats_report_task;
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> index 22a74b6aa17e..e6dcf6da9091 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.h
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.h
> @@ -411,8 +411,8 @@ static_assert(sizeof(struct gve_adminq_report_nic_ts) == 16);
>
>  struct gve_nic_ts_report {
>         __be64 nic_timestamp; /* NIC clock in nanoseconds */
> -       __be64 reserved1;
> -       __be64 reserved2;
> +       __be64 pre_cycles; /* System cycle counter before NIC clock read */
> +       __be64 post_cycles; /* System cycle counter after NIC clock read */
>         __be64 reserved3;
>         __be64 reserved4;
>  };
> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index 4fd7e8a442c5..8a088dcc3603 100644
> --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> @@ -46,6 +46,7 @@ static const char gve_gstrings_main_stats[][ETH_GSTRING_LEN] = {
>         "rx_hsplit_unsplit_pkt",
>         "interface_up_cnt", "interface_down_cnt", "reset_cnt",
>         "page_alloc_fail", "dma_mapping_error", "stats_report_trigger_cnt",
> +       "ptp_precise_xtstamps", "ptp_fallback_xtstamps",
>  };
>
>  static const char gve_gstrings_rx_stats[][ETH_GSTRING_LEN] = {
> @@ -269,6 +270,8 @@ gve_get_ethtool_stats(struct net_device *netdev,
>         data[i++] = priv->page_alloc_fail;
>         data[i++] = priv->dma_mapping_error;
>         data[i++] = priv->stats_report_trigger_cnt;
> +       data[i++] = priv->ptp_precise_xtstamps;
> +       data[i++] = priv->ptp_fallback_xtstamps;
>         i = GVE_MAIN_STATS_LEN;
>
>         rx_base_stats_idx = 0;
> diff --git a/drivers/net/ethernet/google/gve/gve_ptp.c b/drivers/net/ethernet/google/gve/gve_ptp.c
> index ad15f1209a83..9711521ce1d0 100644
> --- a/drivers/net/ethernet/google/gve/gve_ptp.c
> +++ b/drivers/net/ethernet/google/gve/gve_ptp.c
> @@ -10,28 +10,251 @@
>  /* Interval to schedule a nic timestamp calibration, 250ms. */
>  #define GVE_NIC_TS_SYNC_INTERVAL_MS 250
>
> +/*
> + * Stores cycle counter samples in get_cycles() units from a
> + * sandwiched NIC clock read
> + */
> +struct gve_sysclock_sample {
> +       /* Cycle counter from NIC before clock read */
> +       u64 nic_pre_cycles;
> +       /* Cycle counter from NIC after clock read */
> +       u64 nic_post_cycles;
> +       /* Cycle counter from host before issuing AQ command */
> +       cycles_t host_pre_cycles;
> +       /* Cycle counter from host after AQ command returns */
> +       cycles_t host_post_cycles;
> +};
> +
> +/*
> + * Read NIC clock by issuing the AQ command. The command is subject to
> + * rate limiting and may need to be retried. Requires nic_ts_read_lock
> + * to be held.
> + */
> +static int gve_ptp_read_timestamp(struct gve_ptp *ptp, cycles_t *pre_cycles,
> +                                 cycles_t *post_cycles)
> +{
> +       unsigned long deadline = jiffies + msecs_to_jiffies(100);
> +       unsigned long delay_us = 1000;
> +       int err;
> +
> +       lockdep_assert_held(&ptp->nic_ts_read_lock);
> +
> +       do {
> +               *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();

Sashiko says:
---
Is rmb() sufficient here to prevent speculative execution on ARM64?
Since rmb() maps to dmb ishld, it only orders memory load instructions.
Because get_cycles() reads the CPU virtual counter register (cntvct_el0),
is an instruction synchronization barrier (isb()) required to halt
speculative execution?
Without it, could the CPU read the timer too early, violating the
nic_post_cycles <= host_post_cycles invariant and causing valid samples
to be falsely rejected with -EBADMSG?
---

The AdminQ command causes a VM Exit which is a serializing event. The
compiler barrier is sufficient to prevent the compiler from reordering
the get_cyles() call before the AdminQ command is issued.

> +               if (likely(err != -EAGAIN))
> +                       return err;
> +
> +               fsleep(delay_us);
> +
> +               /* Exponential backoff */
> +               delay_us *= 2;
> +       } while (time_before(jiffies, deadline));
> +
> +       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);
> +       if (err) {
> +               dev_err_ratelimited(&ptp->priv->pdev->dev,
> +                                   "AdminQ timestamp read failed: %d\n", err);
>                 goto out;
> +       }
> +
> +       ts_report = ptp->nic_ts_report;
> +       *nic_raw = be64_to_cpu(ts_report->nic_timestamp);
>
> -       *nic_raw = be64_to_cpu(ptp->nic_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)
> +{
> +       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;
> +}
> +
> +/*
> + * 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 %llu cycles to system time: %d\n",
> +                                   cycles,
> +                                   err);
> +               return err;
> +       }
> +
> +       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;
> +}
> +
> +static bool
> +gve_can_use_system_ts_from_device(enum clocksource_ids system_clock_source,
> +                                 clockid_t clockid)
> +{
> +       if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC_RAW)
> +               return false;
> +
> +       /* If the system clock source matches the system clock
> +        * returned by the AdminQ command, we can use the system
> +        * timestamps returned by the device, otherwise we have to
> +        * fall back to sampling system time from the host which
> +        * is less accurate.
> +        */
> +       if (IS_ENABLED(CONFIG_X86))
> +               return system_clock_source == CSID_X86_TSC;
> +       else if (IS_ENABLED(CONFIG_ARM64))
> +               return system_clock_source == CSID_ARM_ARCH_COUNTER;
> +
> +       return false;
> +}
> +
>  static int gve_ptp_gettimex64(struct ptp_clock_info *info,
>                               struct timespec64 *ts,
>                               struct ptp_system_timestamp *sts)
>  {
> -       return -EOPNOTSUPP;
> +       struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
> +       struct gve_sysclock_sample sysclock = {0};
> +       bool use_system_ts_from_device = false;
> +       struct gve_priv *priv = ptp->priv;

Sashiko says:
---
Does this function need to check the device active state before proceeding?
The asynchronous worker gve_ptp_do_aux_work() carefully checks
if (gve_get_reset_in_progress(priv) || !gve_get_admin_queue_ok(priv))
before issuing hardware reads.
Since this function handles the PTP ioctl, if a user queries the PTP clock
while the device is suspended or undergoing reset, could it trigger hardware
faults or access freed AdminQ structures?
---

The AdminQ layer is responsible for checking whether it's in good
state to send commands. These checks need to be synchronized properly
with the threads that reset adminQ. Specifically, these checks need to
be performed under priv->adminq_lock. Will fix gve_ptp_do_aux_work()
in a separate patch.

> +       struct system_time_snapshot snap;
> +       u64 nic_ts;
> +       int err;
> +
> +       if (sts) {
> +               /* This snapshot is used both to query the current system
> +                * clocksource and to convert the cycle counts returned
> +                * by the AdminQ command to ktime. It does not need to be
> +                * taken inside the retry loop because retries and lock
> +                * contention are expected to be extremely rare.
> +                *
> +                * If the system clock source changes between here and
> +                * when get_device_system_crosststamp() is called,
> +                * get_device_system_crosststamp() will fail which will
> +                * cause one failed sample, and the next one will succeed.
> +                */
> +               ktime_get_snapshot(&snap);
> +               use_system_ts_from_device =
> +                       gve_can_use_system_ts_from_device(snap.cs_id,
> +                                                         sts->clockid);
> +               if (use_system_ts_from_device)
> +                       priv->ptp_precise_xtstamps++;
> +               else
> +                       priv->ptp_fallback_xtstamps++;
> +       }

Sashiko says:
---
Can these increments cause read-modify-write data races?
Because this function can be executed concurrently by multiple userspace
threads without driver-level serialization, plain u32 increments could result
in lost updates. Should these be atomic_t or moved inside the
nic_ts_read_lock critical section?
---

Yes, it is theoretically possible for .gettimex64 to be executed
concurrently, but this would require an unusual configuration as there
would need to be two entities accessing the PHC. This will not occur
in the typical Chrony configuration. The probability and impact of a
lost increment are negligible, so I don't think it's worth using an
expensive atomic increment here.

> +
> +       if (unlikely(!use_system_ts_from_device))
> +               ptp_read_system_prets(sts);
> +
> +       err = gve_clock_nic_ts_read(ptp, &nic_ts, sts ? &sysclock : NULL);
> +       if (err)
> +               return err;
> +
> +       if (unlikely(!use_system_ts_from_device))
> +               ptp_read_system_postts(sts);

Sashiko says:
---
Does wrapping gve_clock_nic_ts_read() with ptp_read_system_prets() and
ptp_read_system_postts() cause massive clock skew in the fallback path?
gve_clock_nic_ts_read() acquires nic_ts_read_lock, which may block. It also
executes gve_ptp_read_timestamp(), which can sleep for up to 100ms during
AdminQ retries.
Injecting lock contention delays and sleeps between pre_ts and post_ts could
invalidate the cross-timestamping window, causing the PTP core to calculate
highly skewed system timestamps.
---

I provided a detailed analysis to this question in my response to
Jacob. The short answer is no, it will not cause significant
inaccuracy unless the clock is very unstable. Cycle counters are
sufficiently stable on the cloud platforms of interest for the
timescales we are working with. Lock contention and retries are
extremely unlikely, and Chrony mitigates the occasional bad read by
issuing bursts of 25 reads and discarding the outliers.

> +
> +       if (sts && likely(use_system_ts_from_device)) {
> +               /* Reject samples with out of order system clock values.
> +                * Firmware must return valid non-zero cycle counts.
> +                */
> +               if (!(sysclock.host_pre_cycles <= sysclock.nic_pre_cycles &&
> +                     sysclock.nic_pre_cycles  <= sysclock.nic_post_cycles &&
> +                     sysclock.nic_post_cycles <= sysclock.host_post_cycles)) {
> +                       dev_err_ratelimited(&priv->pdev->dev,
> +                                           "AdminQ system clock cycle counts out of order. Expecting %llu <= %llu <= %llu <= %llu\n",
> +                                           (u64)sysclock.host_pre_cycles,
> +                                           sysclock.nic_pre_cycles,
> +                                           sysclock.nic_post_cycles,
> +                                           (u64)sysclock.host_post_cycles);
> +                       return -EBADMSG;
> +               }
> +
> +               err = gve_cycles_to_timespec64(priv, sts->clockid, &snap,
> +                                              sysclock.nic_pre_cycles,
> +                                              &sts->pre_ts);
> +               if (err)
> +                       return err;
> +
> +               err = gve_cycles_to_timespec64(priv, sts->clockid, &snap,
> +                                              sysclock.nic_post_cycles,
> +                                              &sts->post_ts);
> +               if (err)
> +                       return err;
> +       }
> +
> +       *ts = ns_to_timespec64(nic_ts);
> +
> +       return 0;
>  }
>
>  static int gve_ptp_settime64(struct ptp_clock_info *info,
> @@ -50,7 +273,7 @@ static long gve_ptp_do_aux_work(struct ptp_clock_info *info)
>         if (gve_get_reset_in_progress(priv) || !gve_get_admin_queue_ok(priv))
>                 goto out;
>
> -       err = gve_clock_nic_ts_read(ptp, &nic_raw);
> +       err = gve_clock_nic_ts_read(ptp, &nic_raw, NULL);
>         if (err) {
>                 dev_err_ratelimited(&priv->pdev->dev, "%s read err %d\n",
>                                     __func__, err);
> @@ -93,7 +316,7 @@ int gve_init_clock(struct gve_priv *priv)
>                 goto free_ptp;
>         }
>
> -       err = gve_clock_nic_ts_read(ptp, &nic_raw);
> +       err = gve_clock_nic_ts_read(ptp, &nic_raw, NULL);
>         if (err) {
>                 dev_err(&priv->pdev->dev, "failed to read NIC clock %d\n", err);
>                 goto free_dma_mem;
> --
> 2.54.0.545.g6539524ca2-goog
>