[PATCH 0/2] net/iavf: handle PF not enabling Rx timestamps properly

[PATCH 0/2] net/iavf: handle PF not enabling Rx timestamps properly

[Jacob Keller]

In certain cases, the PF for the iavf driver may support
VIRTCHNL_VF_CAP_PTP but will not enable Rx timestamps. If this occurs, the
iavf driver will attempt to continue anyways. When that happens, the
resulting timestamps will appear to function initially. Upon closer
inspection it becomes clear that the timestamps are invalid and bogus.

First, when reporting a timestamp, always check the validity bit instead of
blindly reporting it. This avoids extending an invalid (likely zero'd)
timestamp value from the Rx descriptor.

Second, don't enable the RTE_ETH_RX_OFFLOAD_TIMESTAMP capability if the PF
doesn't indicate we have support. This will prevent applications from
trying to timestamp when it is not properly enabled.

Typically, this should not happen, as the PFs which support
VIRTCHNL_VF_CAP_PTP should support Rx timestamping. However, we recently
discovered a flaw in some implementations of the
VIRTCHNL_OP_1588_PTP_GET_CAPS command. The layout of the capabilities
structure is incorrect, with the caps member placed at a different byte
offset than the expected structure layout. This is the case with the
upstream Linux ice PF driver.

This results in the PF rejecting the request for Rx timestamping, and
leaving timestamping disabled. A proper fix for this situation is
difficult. If we merely changed the structure layout in DPDK, then it would
stop being compatible with other host OSes and with other implementations
of the Linux ice PF. If we changed the layout upstream, it would break
compatibility with the upstream iAVF VF driver.

A proper fix to resolve this will take some time, as we will likely have to
introduce a new flag or ops capability across many drivers. In the mean
time, we should at least make sure the DPDK driver stops reporting bogus
timestamps in such a setup.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Jacob Keller (2):
      net/iavf: check if PF actually indicates Rx timestamps
      net/iavf: check Rx timestamp validity bit

 drivers/net/intel/iavf/iavf_rxtx.h   | 3 +++
 drivers/net/intel/iavf/iavf_ethdev.c | 3 ++-
 drivers/net/intel/iavf/iavf_rxtx.c   | 9 ++++++---
 3 files changed, 11 insertions(+), 4 deletions(-)
---
base-commit: be0112878b4fc3a1b918370ed2fe615c52039d77
change-id: 20251113-jk-dpdk-iavf-rx-timestamping-improvements-a06d21385371

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>

[PATCH 1/2] net/iavf: check if PF actually indicates Rx timestamps

[Jacob Keller]

The iavf driver has support for hardware Rx timestamps since commit h
b5cd735132f6 ("net/iavf: enable Rx timestamp on flex descriptor").

To enable this, the VF must first negotiate PTP capabilities with the PF
by sending the VIRTCHNL_OP_1588_PTP_GET_CAPS command, with the requested
capabilities. The PF will respond with the actually supported subset of
capabilities.

The PF may not actually enable Rx timestamping, even if it reports the
overall PTP capability support. If this happens, the iavf driver logic
will incorrectly report that Rx timestamps can be enabled despite being
rejected by the PF.

This is unlikely in practice, as most PFs which support the
VIRTCHNL_VF_CAP_PTP will support Rx timestamping. However, there are
some cases where this may not be true.

Check that the PF actually reports the Rx timestamping capability
instead of assuming it is enabled. Doing so prevents the DPDK
application from attempting to enable Rx timestamps when they won't
actually be enabled.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/intel/iavf/iavf_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 15e49fe24814..3ef766de4704 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -1177,7 +1177,8 @@ iavf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_CRC)
 		dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_KEEP_CRC;
 
-	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP)
+	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP &&
+	    vf->ptp_caps & VIRTCHNL_1588_PTP_CAP_RX_TSTAMP)
 		dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_VLAN_V2 &&

-- 
2.51.0.rc1.197.g6d975e95c9d7

[PATCH 2/2] net/iavf: check Rx timestamp validity bit

[Jacob Keller]

When reporting an Rx timestamp from the receive descriptor, the iavf
driver does not check the validity bit in the time_stamp_low field. In
the event that hardware does not capture a receive timestamp for any
reason, this valid bit is unset, and the timestamp value in the
descriptor is zeroed out.

The iavf driver ignores this and passes the zero value into the
iavf_tstamp_convert_32b_64b function regardless, and proceeds to treat
the result as a valid timestamp.

Instead of reporting a zero timestamp which users can clearly interpret
as invalid, the raw 0 value from the descriptor is "extended" to the
64-bit timestamp. This results in values which are not immediately
obvious as invalid to users:

  timestamp 1760629088881475583
  timestamp 1760629088881475583
  timestamp 1760629088881475583

First, if the value is printed in base 10 it is not immediately obvious
that the lower 32 bits are zero. Second, multiple packets in sequence
will receive the same "timestamp".

This occurs because of the timestamp extension logic. The receive
descriptor timestamps are 40 bits, with 32 bits of nanosecond precision,
7 bits of subnanosecond precision, and 1 validity bit. The
sub-nanosecond precision bits are discarded. To obtain a 64-bit
timestamp, the upper 32 bits are calculated from the lower 32-bits and a
snapshot of the PHC timer that is captured recently (within ~2 seconds
of the packet timestamp). This enables reporting proper full 64-bit
timestamps without needing to store all 64 bits in the receive
descriptor.

However, when timestamps are not working properly, the raw 'zero' value
is extended regardless of whether hardware indicated it was a valid
timestamp. As a result, users can see what appear at a glance as valid
timestamps. However, they will not match the packet reception time, and
will only update when the upper bits would roll over. This occurs every
2^32 seconds, or approximately once every 4 seconds.

Instead of reporting bogus extended timestamp values which could confuse
user applications, check the validity bit and only report a timestamp of
the valid bit is set. This matches the implementation used in the Linux
PF driver.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/intel/iavf/iavf_rxtx.h | 3 +++
 drivers/net/intel/iavf/iavf_rxtx.c | 9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/intel/iavf/iavf_rxtx.h b/drivers/net/intel/iavf/iavf_rxtx.h
index 5c9339b99f9a..8efb3bd04ee5 100644
--- a/drivers/net/intel/iavf/iavf_rxtx.h
+++ b/drivers/net/intel/iavf/iavf_rxtx.h
@@ -504,6 +504,9 @@ enum iavf_tx_ctx_desc_tunnel_l4_tunnel_type {
 /* for iavf_32b_rx_flex_desc.pkt_len member */
 #define IAVF_RX_FLX_DESC_PKT_LEN_M	(0x3FFF) /* 14-bits */
 
+/* Valid indicator bit for the time_stamp_low field */
+#define IAVF_RX_FLX_DESC_TS_VALID	(0x1UL)
+
 int iavf_dev_rx_queue_setup(struct rte_eth_dev *dev,
 			   uint16_t queue_idx,
 			   uint16_t nb_desc,
diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
index ea49059f83a1..d8662fd81533 100644
--- a/drivers/net/intel/iavf/iavf_rxtx.c
+++ b/drivers/net/intel/iavf/iavf_rxtx.c
@@ -1582,7 +1582,8 @@ iavf_recv_pkts_flex_rxd(void *rx_queue,
 		rxd_to_pkt_fields_ops[rxq->rxdid](rxq, rxm, &rxd);
 		pkt_flags = iavf_flex_rxd_error_to_pkt_flags(rx_stat_err0);
 
-		if (iavf_timestamp_dynflag > 0) {
+		if (iavf_timestamp_dynflag > 0 &&
+		    rxd.wb.time_stamp_low & IAVF_RX_FLX_DESC_TS_VALID) {
 			ts_ns = iavf_tstamp_convert_32b_64b(rxq->phc_time,
 				rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high));
 
@@ -1751,7 +1752,8 @@ iavf_recv_scattered_pkts_flex_rxd(void *rx_queue, struct rte_mbuf **rx_pkts,
 		rxd_to_pkt_fields_ops[rxq->rxdid](rxq, first_seg, &rxd);
 		pkt_flags = iavf_flex_rxd_error_to_pkt_flags(rx_stat_err0);
 
-		if (iavf_timestamp_dynflag > 0) {
+		if (iavf_timestamp_dynflag > 0 &&
+		    rxd.wb.time_stamp_low & IAVF_RX_FLX_DESC_TS_VALID) {
 			ts_ns = iavf_tstamp_convert_32b_64b(rxq->phc_time,
 				rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high));
 
@@ -2036,7 +2038,8 @@ iavf_rx_scan_hw_ring_flex_rxd(struct ci_rx_queue *rxq,
 			stat_err0 = rte_le_to_cpu_16(rxdp[j].wb.status_error0);
 			pkt_flags = iavf_flex_rxd_error_to_pkt_flags(stat_err0);
 
-			if (iavf_timestamp_dynflag > 0) {
+			if (iavf_timestamp_dynflag > 0 &&
+			    rxdp[j].wb.time_stamp_low & IAVF_RX_FLX_DESC_TS_VALID) {
 				ts_ns = iavf_tstamp_convert_32b_64b(rxq->phc_time,
 					rte_le_to_cpu_32(rxdp[j].wb.flex_ts.ts_high));
 

-- 
2.51.0.rc1.197.g6d975e95c9d7