[PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

RFCv2 for four new features to the virtio network device:

1. pass tx flow state to host, for routing + telemetry
2. pass rx tstamp to guest, for better RTT estimation
3. pass tx tstamp to guest, idem
3. pass tx delivery time to host, for accurate pacing

All would introduce an extension to the virtio spec.
Concurrently with code review I will write ballots to
https://www.oasis-open.org/committees/ballots.php?wg_abbrev=virtio

These changes are to the driver side. Evaluation additionally requires
achanges to qemu and at least one back-end. I implemented preliminary
support in Linux vhost-net. Both patches available through github at

https://github.com/wdebruij/linux/tree/virtio-net-txhash-2
https://github.com/wdebruij/qemu/tree/virtio-net-txhash-2

Changes RFC -> RFCv2
  - add transmit timestamp patch
  - see individual patches for other changes

Willem de Bruijn (4):
  virtio-net: support transmit hash report
  virtio-net: support receive timestamp
  virtio-net: support transmit timestamp
  virtio-net: support future packet transmit time

 drivers/net/virtio_net.c        | 193 +++++++++++++++++++++++++++++++-
 drivers/virtio/virtio_ring.c    |   3 +-
 include/linux/virtio.h          |   1 +
 include/uapi/linux/virtio_net.h |  24 +++-
 4 files changed, 214 insertions(+), 7 deletions(-)

-- 
2.30.0.478.g8a0d178c01-goog

[PATCH RFC v2 1/4] virtio-net: support transmit hash report

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Virtio-net supports sharing the flow hash from device to driver on rx.
Do the same in the other direction for robust routing and telemetry.

Linux derives ipv6 flowlabel and ECMP multipath from sk->sk_txhash,
and updates this field on error with sk_rethink_txhash. Allow the host
stack to do the same.

Concrete examples of error conditions that are resolved are
mentioned in the commits that add sk_rethink_txhash calls. Such as
commit 7788174e8726 ("tcp: change IPv6 flow-label upon receiving
spurious retransmission").

Experimental results mirror what the theory suggests: where IPv6
FlowLabel is included in path selection (e.g., LAG/ECMP), flowlabel
rotation on TCP timeout avoids the vast majority of TCP disconnects
that would otherwise have occurred during link failures in long-haul
backbones, when an alternative path is available.

Rotation can be applied to various bad connection signals, such as
timeouts and spurious retransmissions. In aggregate, such flow level
signals can help locate network issues. Reserve field hash_state to
share this info. For now, always set VIRTIO_NET_HASH_STATE_DEFAULT.
Passing information between TCP stack and driver is future work.

Changes RFC->RFCv2
  - drop unused VIRTIO_NET_HASH_STATE_TIMEOUT_BIT
  - convert from cpu_to_virtioXX to cpu_to_leXX

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 26 +++++++++++++++++++++++---
 include/uapi/linux/virtio_net.h |  9 ++++++++-
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ba8e63792549..7f822b2a5205 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -201,6 +201,9 @@ struct virtnet_info {
 	/* Host will merge rx buffers for big packets (shake it! shake it!) */
 	bool mergeable_rx_bufs;
 
+	/* Driver will pass tx path info to the device */
+	bool has_tx_hash;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -394,9 +397,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 
 	hdr_len = vi->hdr_len;
 	if (vi->mergeable_rx_bufs)
-		hdr_padded_len = sizeof(*hdr);
+		hdr_padded_len = max_t(unsigned int, hdr_len, sizeof(*hdr));
 	else
-		hdr_padded_len = sizeof(struct padded_vnet_hdr);
+		hdr_padded_len = ALIGN(hdr_len, 16);
 
 	/* hdr_valid means no XDP, so we can copy the vnet header */
 	if (hdr_valid)
@@ -1528,6 +1531,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 	struct virtnet_info *vi = sq->vq->vdev->priv;
+	struct virtio_net_hdr_v1_hash *ht;
 	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
@@ -1552,6 +1556,16 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	if (vi->mergeable_rx_bufs)
 		hdr->num_buffers = 0;
 
+	ht = (void *)hdr;
+	if (vi->has_tx_hash) {
+		u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
+					    VIRTIO_NET_HASH_REPORT_OTHER;
+
+		ht->hash_value = cpu_to_le32(skb->hash);
+		ht->hash_report = cpu_to_le16(report);
+		ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
+	}
+
 	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
 	if (can_push) {
 		__skb_push(skb, hdr_len);
@@ -3050,6 +3064,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 	else
 		vi->hdr_len = sizeof(struct virtio_net_hdr);
 
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_TX_HASH)) {
+		vi->has_tx_hash = true;
+		vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
+	}
+
 	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
 	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 		vi->any_header_sg = true;
@@ -3240,7 +3259,8 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
-	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY
+	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
+	VIRTIO_NET_F_TX_HASH
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 3f55a4215f11..273d43c35f59 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,7 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
 #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
 #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
@@ -170,8 +171,14 @@ struct virtio_net_hdr_v1_hash {
 #define VIRTIO_NET_HASH_REPORT_IPv6_EX         7
 #define VIRTIO_NET_HASH_REPORT_TCPv6_EX        8
 #define VIRTIO_NET_HASH_REPORT_UDPv6_EX        9
+#define VIRTIO_NET_HASH_REPORT_L4              10
+#define VIRTIO_NET_HASH_REPORT_OTHER           11
 	__le16 hash_report;
-	__le16 padding;
+	union {
+		__le16 padding;
+#define VIRTIO_NET_HASH_STATE_DEFAULT          0
+		__le16 hash_state;
+	};
 };
 
 #ifndef VIRTIO_NET_NO_LEGACY
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH RFC v2 2/4] virtio-net: support receive timestamp

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Add optional PTP hardware rx timestamp offload for virtio-net.

Accurate RTT measurement requires timestamps close to the wire.
Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the
virtio-net header is expanded with room for a timestamp.

A device may pass receive timestamps for all or some packets. Flag
VIRTIO_NET_HDR_F_TSTAMP signals whether a timestamp is recorded.

A driver that supports hardware timestamping must also support
ioctl SIOCSHWTSTAMP. Implement that, as well as information getters
ioctl SIOCGHWTSTAMP and ethtool get_ts_info (`ethtool -T $DEV`).

The timestamp straddles (virtual) hardware domains. Like PTP, use
international atomic time (CLOCK_TAI) as global clock base. The driver
must sync with the device, e.g., through kvm-clock.

Tested:
  guest: ./timestamping eth0 \
          SOF_TIMESTAMPING_RAW_HARDWARE \
          SOF_TIMESTAMPING_RX_HARDWARE
  host: nc -4 -u 192.168.1.1 319

Changes RFC -> RFCv2
  - rename virtio_net_hdr_v12 to virtio_net_hdr_hash_ts
  - add ethtool .get_ts_info to query capabilities
  - add ioctl SIOC[GS]HWTSTAMP to configure feature
  - add vi->enable_rx_tstamp to store configuration
  - convert virtioXX_to_cpu to leXX_to_cpu
  - convert reserved to __u32

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 113 +++++++++++++++++++++++++++++++-
 include/uapi/linux/virtio_net.h |  13 ++++
 2 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7f822b2a5205..ac44c5efa0bc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -204,6 +204,12 @@ struct virtnet_info {
 	/* Driver will pass tx path info to the device */
 	bool has_tx_hash;
 
+	/* Device can pass CLOCK_TAI receive time to the driver */
+	bool has_rx_tstamp;
+
+	/* Device will pass rx timestamp. Requires has_rx_tstamp */
+	bool enable_rx_tstamp;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -292,6 +298,13 @@ static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff *skb)
 	return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
 }
 
+static inline struct virtio_net_hdr_hash_ts *skb_vnet_hdr_ht(struct sk_buff *skb)
+{
+	BUILD_BUG_ON(sizeof(struct virtio_net_hdr_hash_ts) > sizeof(skb->cb));
+
+	return (void *)skb->cb;
+}
+
 /*
  * private is used to chain pages for big packets, put the whole
  * most recent used list in the beginning for reuse
@@ -1030,6 +1043,19 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	return NULL;
 }
 
+static inline void virtnet_record_rx_tstamp(const struct virtnet_info *vi,
+					    struct sk_buff *skb)
+{
+	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
+
+	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
+	    vi->enable_rx_tstamp) {
+		u64 ts = le64_to_cpu(h->tstamp);
+
+		skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(ts);
+	}
+}
+
 static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 			void *buf, unsigned int len, void **ctx,
 			unsigned int *xdp_xmit,
@@ -1076,6 +1102,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 		goto frame_err;
 	}
 
+	virtnet_record_rx_tstamp(vi, skb);
 	skb_record_rx_queue(skb, vq2rxq(rq->vq));
 	skb->protocol = eth_type_trans(skb, dev);
 	pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
@@ -2263,6 +2290,28 @@ static int virtnet_get_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static int virtnet_get_ts_info(struct net_device *dev,
+			       struct ethtool_ts_info *info)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	/* setup default software timestamp */
+	ethtool_op_get_ts_info(dev, info);
+
+	/* return rx capabilities (which may differ from current enable) */
+	if (vi->has_rx_tstamp) {
+		info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
+					 SOF_TIMESTAMPING_RAW_HARDWARE;
+		info->rx_filters = HWTSTAMP_FILTER_ALL;
+	} else {
+		info->rx_filters = HWTSTAMP_FILTER_NONE;
+	}
+
+	info->tx_types = HWTSTAMP_TX_OFF;
+
+	return 0;
+}
+
 static void virtnet_init_settings(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -2300,7 +2349,7 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
 	.get_ethtool_stats = virtnet_get_ethtool_stats,
 	.set_channels = virtnet_set_channels,
 	.get_channels = virtnet_get_channels,
-	.get_ts_info = ethtool_op_get_ts_info,
+	.get_ts_info = virtnet_get_ts_info,
 	.get_link_ksettings = virtnet_get_link_ksettings,
 	.set_link_ksettings = virtnet_set_link_ksettings,
 	.set_coalesce = virtnet_set_coalesce,
@@ -2558,6 +2607,60 @@ static int virtnet_set_features(struct net_device *dev,
 	return 0;
 }
 
+static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct hwtstamp_config tsconf;
+
+	if (copy_from_user(&tsconf, ifr->ifr_data, sizeof(tsconf)))
+		return -EFAULT;
+	if (tsconf.flags)
+		return -EINVAL;
+	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
+		return -ERANGE;
+	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
+	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
+		tsconf.rx_filter = HWTSTAMP_FILTER_ALL;
+
+	if (!vi->has_rx_tstamp)
+		tsconf.rx_filter = HWTSTAMP_FILTER_NONE;
+	else
+		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
+
+	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	struct hwtstamp_config tsconf;
+
+	tsconf.flags = 0;
+	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
+						  HWTSTAMP_FILTER_NONE;
+	tsconf.tx_type = HWTSTAMP_TX_OFF;
+
+	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int virtnet_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return virtnet_ioctl_set_hwtstamp(dev, ifr);
+
+	case SIOCGHWTSTAMP:
+		return virtnet_ioctl_get_hwtstamp(dev, ifr);
+	}
+	return -EOPNOTSUPP;
+}
+
 static const struct net_device_ops virtnet_netdev = {
 	.ndo_open            = virtnet_open,
 	.ndo_stop   	     = virtnet_close,
@@ -2573,6 +2676,7 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
 	.ndo_set_features	= virtnet_set_features,
+	.ndo_do_ioctl		= virtnet_ioctl,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -3069,6 +3173,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 		vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
 	}
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) {
+		vi->has_rx_tstamp = true;
+		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
+	}
+
 	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
 	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 		vi->any_header_sg = true;
@@ -3260,7 +3369,7 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-	VIRTIO_NET_F_TX_HASH
+	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 273d43c35f59..a5c84410cf92 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,7 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
 #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
 #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
@@ -126,6 +127,7 @@ struct virtio_net_hdr_v1 {
 #define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	/* Use csum_start, csum_offset */
 #define VIRTIO_NET_HDR_F_DATA_VALID	2	/* Csum is valid */
 #define VIRTIO_NET_HDR_F_RSC_INFO	4	/* rsc info in csum_ fields */
+#define VIRTIO_NET_HDR_F_TSTAMP		8	/* timestamp is recorded */
 	__u8 flags;
 #define VIRTIO_NET_HDR_GSO_NONE		0	/* Not a GSO frame */
 #define VIRTIO_NET_HDR_GSO_TCPV4	1	/* GSO frame, IPv4 TCP (TSO) */
@@ -181,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
 	};
 };
 
+struct virtio_net_hdr_hash_ts {
+	struct virtio_net_hdr_v1 hdr;
+	struct {
+		__le32 value;
+		__le16 report;
+		__le16 flow_state;
+	} hash;
+	__u32 reserved;
+	__le64 tstamp;
+};
+
 #ifndef VIRTIO_NET_NO_LEGACY
 /* This header comes first in the scatter-gather list.
  * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH RFC v2 2/4] virtio-net: support receive timestamp

[Jason Wang]

On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Add optional PTP hardware rx timestamp offload for virtio-net.
>
> Accurate RTT measurement requires timestamps close to the wire.
> Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the
> virtio-net header is expanded with room for a timestamp.
>
> A device may pass receive timestamps for all or some packets. Flag
> VIRTIO_NET_HDR_F_TSTAMP signals whether a timestamp is recorded.
>
> A driver that supports hardware timestamping must also support
> ioctl SIOCSHWTSTAMP. Implement that, as well as information getters
> ioctl SIOCGHWTSTAMP and ethtool get_ts_info (`ethtool -T $DEV`).
>
> The timestamp straddles (virtual) hardware domains. Like PTP, use
> international atomic time (CLOCK_TAI) as global clock base. The driver
> must sync with the device, e.g., through kvm-clock.
>
> Tested:
>    guest: ./timestamping eth0 \
>            SOF_TIMESTAMPING_RAW_HARDWARE \
>            SOF_TIMESTAMPING_RX_HARDWARE
>    host: nc -4 -u 192.168.1.1 319
>
> Changes RFC -> RFCv2
>    - rename virtio_net_hdr_v12 to virtio_net_hdr_hash_ts
>    - add ethtool .get_ts_info to query capabilities
>    - add ioctl SIOC[GS]HWTSTAMP to configure feature
>    - add vi->enable_rx_tstamp to store configuration
>    - convert virtioXX_to_cpu to leXX_to_cpu
>    - convert reserved to __u32
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>   drivers/net/virtio_net.c        | 113 +++++++++++++++++++++++++++++++-
>   include/uapi/linux/virtio_net.h |  13 ++++
>   2 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7f822b2a5205..ac44c5efa0bc 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -204,6 +204,12 @@ struct virtnet_info {
>   	/* Driver will pass tx path info to the device */
>   	bool has_tx_hash;
>   
> +	/* Device can pass CLOCK_TAI receive time to the driver */
> +	bool has_rx_tstamp;
> +
> +	/* Device will pass rx timestamp. Requires has_rx_tstamp */
> +	bool enable_rx_tstamp;
> +
>   	/* Has control virtqueue */
>   	bool has_cvq;
>   
> @@ -292,6 +298,13 @@ static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff *skb)
>   	return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
>   }
>   
> +static inline struct virtio_net_hdr_hash_ts *skb_vnet_hdr_ht(struct sk_buff *skb)
> +{
> +	BUILD_BUG_ON(sizeof(struct virtio_net_hdr_hash_ts) > sizeof(skb->cb));
> +
> +	return (void *)skb->cb;
> +}
> +
>   /*
>    * private is used to chain pages for big packets, put the whole
>    * most recent used list in the beginning for reuse
> @@ -1030,6 +1043,19 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   	return NULL;
>   }
>   
> +static inline void virtnet_record_rx_tstamp(const struct virtnet_info *vi,
> +					    struct sk_buff *skb)
> +{
> +	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> +
> +	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> +	    vi->enable_rx_tstamp) {
> +		u64 ts = le64_to_cpu(h->tstamp);
> +
> +		skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(ts);
> +	}
> +}
> +
>   static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>   			void *buf, unsigned int len, void **ctx,
>   			unsigned int *xdp_xmit,
> @@ -1076,6 +1102,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>   		goto frame_err;
>   	}
>   
> +	virtnet_record_rx_tstamp(vi, skb);
>   	skb_record_rx_queue(skb, vq2rxq(rq->vq));
>   	skb->protocol = eth_type_trans(skb, dev);
>   	pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
> @@ -2263,6 +2290,28 @@ static int virtnet_get_coalesce(struct net_device *dev,
>   	return 0;
>   }
>   
> +static int virtnet_get_ts_info(struct net_device *dev,
> +			       struct ethtool_ts_info *info)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +
> +	/* setup default software timestamp */
> +	ethtool_op_get_ts_info(dev, info);
> +
> +	/* return rx capabilities (which may differ from current enable) */
> +	if (vi->has_rx_tstamp) {
> +		info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> +					 SOF_TIMESTAMPING_RAW_HARDWARE;
> +		info->rx_filters = HWTSTAMP_FILTER_ALL;
> +	} else {
> +		info->rx_filters = HWTSTAMP_FILTER_NONE;
> +	}
> +
> +	info->tx_types = HWTSTAMP_TX_OFF;
> +
> +	return 0;
> +}
> +
>   static void virtnet_init_settings(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> @@ -2300,7 +2349,7 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
>   	.get_ethtool_stats = virtnet_get_ethtool_stats,
>   	.set_channels = virtnet_set_channels,
>   	.get_channels = virtnet_get_channels,
> -	.get_ts_info = ethtool_op_get_ts_info,
> +	.get_ts_info = virtnet_get_ts_info,
>   	.get_link_ksettings = virtnet_get_link_ksettings,
>   	.set_link_ksettings = virtnet_set_link_ksettings,
>   	.set_coalesce = virtnet_set_coalesce,
> @@ -2558,6 +2607,60 @@ static int virtnet_set_features(struct net_device *dev,
>   	return 0;
>   }
>   
> +static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	struct hwtstamp_config tsconf;
> +
> +	if (copy_from_user(&tsconf, ifr->ifr_data, sizeof(tsconf)))
> +		return -EFAULT;
> +	if (tsconf.flags)
> +		return -EINVAL;
> +	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
> +		return -ERANGE;
> +	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
> +	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
> +		tsconf.rx_filter = HWTSTAMP_FILTER_ALL;
> +
> +	if (!vi->has_rx_tstamp)
> +		tsconf.rx_filter = HWTSTAMP_FILTER_NONE;
> +	else
> +		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
> +
> +	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	struct hwtstamp_config tsconf;
> +
> +	tsconf.flags = 0;
> +	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
> +						  HWTSTAMP_FILTER_NONE;
> +	tsconf.tx_type = HWTSTAMP_TX_OFF;
> +
> +	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int virtnet_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	switch (cmd) {
> +	case SIOCSHWTSTAMP:
> +		return virtnet_ioctl_set_hwtstamp(dev, ifr);
> +
> +	case SIOCGHWTSTAMP:
> +		return virtnet_ioctl_get_hwtstamp(dev, ifr);
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
>   static const struct net_device_ops virtnet_netdev = {
>   	.ndo_open            = virtnet_open,
>   	.ndo_stop   	     = virtnet_close,
> @@ -2573,6 +2676,7 @@ static const struct net_device_ops virtnet_netdev = {
>   	.ndo_features_check	= passthru_features_check,
>   	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
>   	.ndo_set_features	= virtnet_set_features,
> +	.ndo_do_ioctl		= virtnet_ioctl,
>   };
>   
>   static void virtnet_config_changed_work(struct work_struct *work)
> @@ -3069,6 +3173,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>   		vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
>   	}
>   
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) {
> +		vi->has_rx_tstamp = true;
> +		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);


Does this mean even if the device doesn't pass timestamp, the header 
still contains the timestamp fields.


> +	}
> +
>   	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
>   	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>   		vi->any_header_sg = true;
> @@ -3260,7 +3369,7 @@ static struct virtio_device_id id_table[] = {
>   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_TX_HASH
> +	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
>   
>   static unsigned int features[] = {
>   	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 273d43c35f59..a5c84410cf92 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,7 @@
>   					 * Steering */
>   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>   
> +#define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
>   #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
>   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
>   #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
> @@ -126,6 +127,7 @@ struct virtio_net_hdr_v1 {
>   #define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	/* Use csum_start, csum_offset */
>   #define VIRTIO_NET_HDR_F_DATA_VALID	2	/* Csum is valid */
>   #define VIRTIO_NET_HDR_F_RSC_INFO	4	/* rsc info in csum_ fields */
> +#define VIRTIO_NET_HDR_F_TSTAMP		8	/* timestamp is recorded */
>   	__u8 flags;
>   #define VIRTIO_NET_HDR_GSO_NONE		0	/* Not a GSO frame */
>   #define VIRTIO_NET_HDR_GSO_TCPV4	1	/* GSO frame, IPv4 TCP (TSO) */
> @@ -181,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
>   	};
>   };
>   
> +struct virtio_net_hdr_hash_ts {
> +	struct virtio_net_hdr_v1 hdr;
> +	struct {
> +		__le32 value;
> +		__le16 report;
> +		__le16 flow_state;
> +	} hash;


Any reason for not embedding structure virtio_net_hdr_v1_hash?

Thanks


> +	__u32 reserved;
> +	__le64 tstamp;
> +};
> +
>   #ifndef VIRTIO_NET_NO_LEGACY
>   /* This header comes first in the scatter-gather list.
>    * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must

Re: [PATCH RFC v2 2/4] virtio-net: support receive timestamp

[Willem de Bruijn]

On Mon, Feb 8, 2021 at 11:13 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Add optional PTP hardware rx timestamp offload for virtio-net.
> >
> > Accurate RTT measurement requires timestamps close to the wire.
> > Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the
> > virtio-net header is expanded with room for a timestamp.
> >
> > A device may pass receive timestamps for all or some packets. Flag
> > VIRTIO_NET_HDR_F_TSTAMP signals whether a timestamp is recorded.
> >
> > A driver that supports hardware timestamping must also support
> > ioctl SIOCSHWTSTAMP. Implement that, as well as information getters
> > ioctl SIOCGHWTSTAMP and ethtool get_ts_info (`ethtool -T $DEV`).
> >
> > The timestamp straddles (virtual) hardware domains. Like PTP, use
> > international atomic time (CLOCK_TAI) as global clock base. The driver
> > must sync with the device, e.g., through kvm-clock.
> >
> > Tested:
> >    guest: ./timestamping eth0 \
> >            SOF_TIMESTAMPING_RAW_HARDWARE \
> >            SOF_TIMESTAMPING_RX_HARDWARE
> >    host: nc -4 -u 192.168.1.1 319
> >
> > Changes RFC -> RFCv2
> >    - rename virtio_net_hdr_v12 to virtio_net_hdr_hash_ts
> >    - add ethtool .get_ts_info to query capabilities
> >    - add ioctl SIOC[GS]HWTSTAMP to configure feature
> >    - add vi->enable_rx_tstamp to store configuration
> >    - convert virtioXX_to_cpu to leXX_to_cpu
> >    - convert reserved to __u32
> >
> > Signed-off-by: Willem de Bruijn <willemb@google.com>

> >   static const struct net_device_ops virtnet_netdev = {
> >       .ndo_open            = virtnet_open,
> >       .ndo_stop            = virtnet_close,
> > @@ -2573,6 +2676,7 @@ static const struct net_device_ops virtnet_netdev = {
> >       .ndo_features_check     = passthru_features_check,
> >       .ndo_get_phys_port_name = virtnet_get_phys_port_name,
> >       .ndo_set_features       = virtnet_set_features,
> > +     .ndo_do_ioctl           = virtnet_ioctl,
> >   };
> >
> >   static void virtnet_config_changed_work(struct work_struct *work)
> > @@ -3069,6 +3173,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> >               vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
> >       }
> >
> > +     if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) {
> > +             vi->has_rx_tstamp = true;
> > +             vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
>
>
> Does this mean even if the device doesn't pass timestamp, the header
> still contains the timestamp fields.

Yes. As implemented, the size of the header is constant across
packets. If both sides negotiate the feature, then all headers reserve
space, whether or not the specific packet has a timestamp.

So far headers are fixed size. I suppose we could investigate variable
size headers. This goes back to our discussion in the previous
patchset, that we can always add a packed-header feature later, if the
number of optional features reaches a size that makes the complexity
worthwhile.

> > +     }
> > +
> >       if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
> >           virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> >               vi->any_header_sg = true;
> > @@ -3260,7 +3369,7 @@ static struct virtio_device_id id_table[] = {
> >       VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >       VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >       VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> > -     VIRTIO_NET_F_TX_HASH
> > +     VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
> >
> >   static unsigned int features[] = {
> >       VIRTNET_FEATURES,
> > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> > index 273d43c35f59..a5c84410cf92 100644
> > --- a/include/uapi/linux/virtio_net.h
> > +++ b/include/uapi/linux/virtio_net.h
> > @@ -57,6 +57,7 @@
> >                                        * Steering */
> >   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23       /* Set MAC address */
> >
> > +#define VIRTIO_NET_F_RX_TSTAMP         55    /* Device sends TAI receive time */
> >   #define VIRTIO_NET_F_TX_HASH          56    /* Driver sends hash report */
> >   #define VIRTIO_NET_F_HASH_REPORT  57        /* Supports hash report */
> >   #define VIRTIO_NET_F_RSS      60    /* Supports RSS RX steering */
> > @@ -126,6 +127,7 @@ struct virtio_net_hdr_v1 {
> >   #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1       /* Use csum_start, csum_offset */
> >   #define VIRTIO_NET_HDR_F_DATA_VALID 2       /* Csum is valid */
> >   #define VIRTIO_NET_HDR_F_RSC_INFO   4       /* rsc info in csum_ fields */
> > +#define VIRTIO_NET_HDR_F_TSTAMP              8       /* timestamp is recorded */
> >       __u8 flags;
> >   #define VIRTIO_NET_HDR_GSO_NONE             0       /* Not a GSO frame */
> >   #define VIRTIO_NET_HDR_GSO_TCPV4    1       /* GSO frame, IPv4 TCP (TSO) */
> > @@ -181,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
> >       };
> >   };
> >
> > +struct virtio_net_hdr_hash_ts {
> > +     struct virtio_net_hdr_v1 hdr;
> > +     struct {
> > +             __le32 value;
> > +             __le16 report;
> > +             __le16 flow_state;
> > +     } hash;
>
>
> Any reason for not embedding structure virtio_net_hdr_v1_hash?

Just that it becomes an onion of struct inside structs. I can change
if you prefer.

> Thanks

As always, thanks for reviewing, Jason.

Re: [PATCH RFC v2 2/4] virtio-net: support receive timestamp

[Jason Wang]

On 2021/2/9 下午9:53, Willem de Bruijn wrote:
> On Mon, Feb 8, 2021 at 11:13 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@google.com>
>>>
>>> Add optional PTP hardware rx timestamp offload for virtio-net.
>>>
>>> Accurate RTT measurement requires timestamps close to the wire.
>>> Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the
>>> virtio-net header is expanded with room for a timestamp.
>>>
>>> A device may pass receive timestamps for all or some packets. Flag
>>> VIRTIO_NET_HDR_F_TSTAMP signals whether a timestamp is recorded.
>>>
>>> A driver that supports hardware timestamping must also support
>>> ioctl SIOCSHWTSTAMP. Implement that, as well as information getters
>>> ioctl SIOCGHWTSTAMP and ethtool get_ts_info (`ethtool -T $DEV`).
>>>
>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
>>> international atomic time (CLOCK_TAI) as global clock base. The driver
>>> must sync with the device, e.g., through kvm-clock.
>>>
>>> Tested:
>>>     guest: ./timestamping eth0 \
>>>             SOF_TIMESTAMPING_RAW_HARDWARE \
>>>             SOF_TIMESTAMPING_RX_HARDWARE
>>>     host: nc -4 -u 192.168.1.1 319
>>>
>>> Changes RFC -> RFCv2
>>>     - rename virtio_net_hdr_v12 to virtio_net_hdr_hash_ts
>>>     - add ethtool .get_ts_info to query capabilities
>>>     - add ioctl SIOC[GS]HWTSTAMP to configure feature
>>>     - add vi->enable_rx_tstamp to store configuration
>>>     - convert virtioXX_to_cpu to leXX_to_cpu
>>>     - convert reserved to __u32
>>>
>>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>>>    static const struct net_device_ops virtnet_netdev = {
>>>        .ndo_open            = virtnet_open,
>>>        .ndo_stop            = virtnet_close,
>>> @@ -2573,6 +2676,7 @@ static const struct net_device_ops virtnet_netdev = {
>>>        .ndo_features_check     = passthru_features_check,
>>>        .ndo_get_phys_port_name = virtnet_get_phys_port_name,
>>>        .ndo_set_features       = virtnet_set_features,
>>> +     .ndo_do_ioctl           = virtnet_ioctl,
>>>    };
>>>
>>>    static void virtnet_config_changed_work(struct work_struct *work)
>>> @@ -3069,6 +3173,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>                vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
>>>        }
>>>
>>> +     if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) {
>>> +             vi->has_rx_tstamp = true;
>>> +             vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
>>
>> Does this mean even if the device doesn't pass timestamp, the header
>> still contains the timestamp fields.
> Yes. As implemented, the size of the header is constant across
> packets. If both sides negotiate the feature, then all headers reserve
> space, whether or not the specific packet has a timestamp.
>
> So far headers are fixed size. I suppose we could investigate variable
> size headers. This goes back to our discussion in the previous
> patchset, that we can always add a packed-header feature later, if the
> number of optional features reaches a size that makes the complexity
> worthwhile.


Right, so for timstamp it's probably OK but we probably need to do as 
you said here if we want to add more in the header. Let's see how 
Michael think about this.


>
>>> +     }
>>> +
>>>        if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
>>>            virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>>>                vi->any_header_sg = true;
>>> @@ -3260,7 +3369,7 @@ static struct virtio_device_id id_table[] = {
>>>        VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>        VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>        VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>>> -     VIRTIO_NET_F_TX_HASH
>>> +     VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
>>>
>>>    static unsigned int features[] = {
>>>        VIRTNET_FEATURES,
>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>> index 273d43c35f59..a5c84410cf92 100644
>>> --- a/include/uapi/linux/virtio_net.h
>>> +++ b/include/uapi/linux/virtio_net.h
>>> @@ -57,6 +57,7 @@
>>>                                         * Steering */
>>>    #define VIRTIO_NET_F_CTRL_MAC_ADDR 23       /* Set MAC address */
>>>
>>> +#define VIRTIO_NET_F_RX_TSTAMP         55    /* Device sends TAI receive time */
>>>    #define VIRTIO_NET_F_TX_HASH          56    /* Driver sends hash report */
>>>    #define VIRTIO_NET_F_HASH_REPORT  57        /* Supports hash report */
>>>    #define VIRTIO_NET_F_RSS      60    /* Supports RSS RX steering */
>>> @@ -126,6 +127,7 @@ struct virtio_net_hdr_v1 {
>>>    #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1       /* Use csum_start, csum_offset */
>>>    #define VIRTIO_NET_HDR_F_DATA_VALID 2       /* Csum is valid */
>>>    #define VIRTIO_NET_HDR_F_RSC_INFO   4       /* rsc info in csum_ fields */
>>> +#define VIRTIO_NET_HDR_F_TSTAMP              8       /* timestamp is recorded */
>>>        __u8 flags;
>>>    #define VIRTIO_NET_HDR_GSO_NONE             0       /* Not a GSO frame */
>>>    #define VIRTIO_NET_HDR_GSO_TCPV4    1       /* GSO frame, IPv4 TCP (TSO) */
>>> @@ -181,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
>>>        };
>>>    };
>>>
>>> +struct virtio_net_hdr_hash_ts {
>>> +     struct virtio_net_hdr_v1 hdr;
>>> +     struct {
>>> +             __le32 value;
>>> +             __le16 report;
>>> +             __le16 flow_state;
>>> +     } hash;
>>
>> Any reason for not embedding structure virtio_net_hdr_v1_hash?
> Just that it becomes an onion of struct inside structs. I can change
> if you prefer.


Yes please (unless Michael has other opinion).


>
>> Thanks
> As always, thanks for reviewing, Jason.
>

You're welcome :)

Thanks

[PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Add optional PTP hardware tx timestamp offload for virtio-net.

Accurate RTT measurement requires timestamps close to the wire.
Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
equivalent to VIRTIO_NET_F_RX_TSTAMP.

The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
returned on completion. If the feature is negotiated, the device
either places the timestamp or clears the feature bit.

The timestamp straddles (virtual) hardware domains. Like PTP, use
international atomic time (CLOCK_TAI) as global clock base. The driver
must sync with the device, e.g., through kvm-clock.

Modify can_push to ensure that on tx completion the header, and thus
timestamp, is in a predicatable location at skb_vnet_hdr.

RFC: this implementation relies on the device writing to the buffer.
That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
The virtio changes should be a separate patch at the least.

Tested: modified txtimestamp.c to with h/w timestamping:
  -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
  +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
  + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
 drivers/virtio/virtio_ring.c    |  3 +-
 include/linux/virtio.h          |  1 +
 include/uapi/linux/virtio_net.h |  1 +
 4 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ac44c5efa0bc..fc8ecd3a333a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -210,6 +210,12 @@ struct virtnet_info {
 	/* Device will pass rx timestamp. Requires has_rx_tstamp */
 	bool enable_rx_tstamp;
 
+	/* Device can pass CLOCK_TAI transmit time to the driver */
+	bool has_tx_tstamp;
+
+	/* Device will pass tx timestamp. Requires has_tx_tstamp */
+	bool enable_tx_tstamp;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 	return stats.packets;
 }
 
+static void virtnet_record_tx_tstamp(const struct send_queue *sq,
+				     struct sk_buff *skb)
+{
+	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
+	const struct virtnet_info *vi = sq->vq->vdev->priv;
+	struct skb_shared_hwtstamps ts;
+
+	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
+	    vi->enable_tx_tstamp) {
+		ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
+		skb_tstamp_tx(skb, &ts);
+	}
+}
+
 static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
 {
 	unsigned int len;
@@ -1412,6 +1432,7 @@ static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
 		if (likely(!is_xdp_frame(ptr))) {
 			struct sk_buff *skb = ptr;
 
+			virtnet_record_tx_tstamp(sq, skb);
 			pr_debug("Sent skb %p\n", skb);
 
 			bytes += skb->len;
@@ -1558,7 +1579,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	struct virtio_net_hdr_v1_hash *ht;
+	struct virtio_net_hdr_hash_ts *ht;
 	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
@@ -1567,7 +1588,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 
 	can_push = vi->any_header_sg &&
 		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
-		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
+		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len &&
+		!vi->enable_tx_tstamp;
 	/* Even if we can, don't push here yet as this would skew
 	 * csum_start offset below. */
 	if (can_push)
@@ -1588,10 +1610,12 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
 					    VIRTIO_NET_HASH_REPORT_OTHER;
 
-		ht->hash_value = cpu_to_le32(skb->hash);
-		ht->hash_report = cpu_to_le16(report);
-		ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
+		ht->hash.value = cpu_to_le32(skb->hash);
+		ht->hash.report = cpu_to_le16(report);
+		ht->hash.flow_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
 	}
+	if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+		ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
 
 	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
 	if (can_push) {
@@ -2307,7 +2331,13 @@ static int virtnet_get_ts_info(struct net_device *dev,
 		info->rx_filters = HWTSTAMP_FILTER_NONE;
 	}
 
-	info->tx_types = HWTSTAMP_TX_OFF;
+	if (vi->has_tx_tstamp) {
+		info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
+					 SOF_TIMESTAMPING_RAW_HARDWARE;
+		info->tx_types = HWTSTAMP_TX_ON;
+	} else {
+		info->tx_types = HWTSTAMP_TX_OFF;
+	}
 
 	return 0;
 }
@@ -2616,7 +2646,8 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 		return -EFAULT;
 	if (tsconf.flags)
 		return -EINVAL;
-	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
+	if (tsconf.tx_type != HWTSTAMP_TX_OFF &&
+	    tsconf.tx_type != HWTSTAMP_TX_ON)
 		return -ERANGE;
 	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
 	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
@@ -2627,6 +2658,11 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 	else
 		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
 
+	if (!vi->has_tx_tstamp)
+		tsconf.tx_type = HWTSTAMP_TX_OFF;
+	else
+		vi->enable_tx_tstamp = tsconf.tx_type == HWTSTAMP_TX_ON;
+
 	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
 		return -EFAULT;
 
@@ -2641,7 +2677,8 @@ static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 	tsconf.flags = 0;
 	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
 						  HWTSTAMP_FILTER_NONE;
-	tsconf.tx_type = HWTSTAMP_TX_OFF;
+	tsconf.tx_type = vi->enable_tx_tstamp ? HWTSTAMP_TX_ON :
+						HWTSTAMP_TX_OFF;
 
 	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
 		return -EFAULT;
@@ -3178,6 +3215,12 @@ static int virtnet_probe(struct virtio_device *vdev)
 		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
 	}
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP) &&
+	    !vring_use_dma_api(vdev)) {
+		vi->has_tx_tstamp = true;
+		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
+	}
+
 	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
 	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 		vi->any_header_sg = true;
@@ -3369,7 +3412,7 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
+	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..cf5d5d1f9b14 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -238,7 +238,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
  * unconditionally on data path.
  */
 
-static bool vring_use_dma_api(struct virtio_device *vdev)
+bool vring_use_dma_api(struct virtio_device *vdev)
 {
 	if (!virtio_has_dma_quirk(vdev))
 		return true;
@@ -257,6 +257,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(vring_use_dma_api);
 
 size_t virtio_max_dma_size(struct virtio_device *vdev)
 {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 55ea329fe72a..5289e2812e95 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -140,6 +140,7 @@ int virtio_device_freeze(struct virtio_device *dev);
 int virtio_device_restore(struct virtio_device *dev);
 #endif
 
+bool vring_use_dma_api(struct virtio_device *vdev);
 size_t virtio_max_dma_size(struct virtio_device *vdev);
 
 #define virtio_device_for_each_vq(vdev, vq) \
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index a5c84410cf92..b5d6f0c6cead 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,7 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#define VIRTIO_NET_F_TX_TSTAMP	  54	/* Device sends TAI transmit time */
 #define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
 #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Jason Wang]

On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Add optional PTP hardware tx timestamp offload for virtio-net.
>
> Accurate RTT measurement requires timestamps close to the wire.
> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> equivalent to VIRTIO_NET_F_RX_TSTAMP.
>
> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> returned on completion. If the feature is negotiated, the device
> either places the timestamp or clears the feature bit.
>
> The timestamp straddles (virtual) hardware domains. Like PTP, use
> international atomic time (CLOCK_TAI) as global clock base. The driver
> must sync with the device, e.g., through kvm-clock.
>
> Modify can_push to ensure that on tx completion the header, and thus
> timestamp, is in a predicatable location at skb_vnet_hdr.
>
> RFC: this implementation relies on the device writing to the buffer.
> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
> The virtio changes should be a separate patch at the least.
>
> Tested: modified txtimestamp.c to with h/w timestamping:
>    -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
>    +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
>    + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>   drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
>   drivers/virtio/virtio_ring.c    |  3 +-
>   include/linux/virtio.h          |  1 +
>   include/uapi/linux/virtio_net.h |  1 +
>   4 files changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ac44c5efa0bc..fc8ecd3a333a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -210,6 +210,12 @@ struct virtnet_info {
>   	/* Device will pass rx timestamp. Requires has_rx_tstamp */
>   	bool enable_rx_tstamp;
>   
> +	/* Device can pass CLOCK_TAI transmit time to the driver */
> +	bool has_tx_tstamp;
> +
> +	/* Device will pass tx timestamp. Requires has_tx_tstamp */
> +	bool enable_tx_tstamp;
> +
>   	/* Has control virtqueue */
>   	bool has_cvq;
>   
> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>   	return stats.packets;
>   }
>   
> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
> +				     struct sk_buff *skb)
> +{
> +	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> +	const struct virtnet_info *vi = sq->vq->vdev->priv;
> +	struct skb_shared_hwtstamps ts;
> +
> +	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> +	    vi->enable_tx_tstamp) {
> +		ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
> +		skb_tstamp_tx(skb, &ts);


This probably won't work since the buffer is read-only from the device. 
(See virtqueue_add_outbuf()).

Another issue that I vaguely remember that the virtio spec forbids out 
buffer after in buffer.


> +	}
> +}
> +
>   static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
>   {
>   	unsigned int len;
> @@ -1412,6 +1432,7 @@ static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
>   		if (likely(!is_xdp_frame(ptr))) {
>   			struct sk_buff *skb = ptr;
>   
> +			virtnet_record_tx_tstamp(sq, skb);
>   			pr_debug("Sent skb %p\n", skb);
>   
>   			bytes += skb->len;
> @@ -1558,7 +1579,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>   	struct virtio_net_hdr_mrg_rxbuf *hdr;
>   	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
>   	struct virtnet_info *vi = sq->vq->vdev->priv;
> -	struct virtio_net_hdr_v1_hash *ht;
> +	struct virtio_net_hdr_hash_ts *ht;
>   	int num_sg;
>   	unsigned hdr_len = vi->hdr_len;
>   	bool can_push;
> @@ -1567,7 +1588,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>   
>   	can_push = vi->any_header_sg &&
>   		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> -		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len &&
> +		!vi->enable_tx_tstamp;
>   	/* Even if we can, don't push here yet as this would skew
>   	 * csum_start offset below. */
>   	if (can_push)
> @@ -1588,10 +1610,12 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>   		u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
>   					    VIRTIO_NET_HASH_REPORT_OTHER;
>   
> -		ht->hash_value = cpu_to_le32(skb->hash);
> -		ht->hash_report = cpu_to_le16(report);
> -		ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> +		ht->hash.value = cpu_to_le32(skb->hash);
> +		ht->hash.report = cpu_to_le16(report);
> +		ht->hash.flow_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
>   	}
> +	if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
> +		ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
>   
>   	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
>   	if (can_push) {
> @@ -2307,7 +2331,13 @@ static int virtnet_get_ts_info(struct net_device *dev,
>   		info->rx_filters = HWTSTAMP_FILTER_NONE;
>   	}
>   
> -	info->tx_types = HWTSTAMP_TX_OFF;
> +	if (vi->has_tx_tstamp) {
> +		info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
> +					 SOF_TIMESTAMPING_RAW_HARDWARE;
> +		info->tx_types = HWTSTAMP_TX_ON;
> +	} else {
> +		info->tx_types = HWTSTAMP_TX_OFF;
> +	}
>   
>   	return 0;
>   }
> @@ -2616,7 +2646,8 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>   		return -EFAULT;
>   	if (tsconf.flags)
>   		return -EINVAL;
> -	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
> +	if (tsconf.tx_type != HWTSTAMP_TX_OFF &&
> +	    tsconf.tx_type != HWTSTAMP_TX_ON)
>   		return -ERANGE;
>   	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
>   	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
> @@ -2627,6 +2658,11 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>   	else
>   		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
>   
> +	if (!vi->has_tx_tstamp)
> +		tsconf.tx_type = HWTSTAMP_TX_OFF;
> +	else
> +		vi->enable_tx_tstamp = tsconf.tx_type == HWTSTAMP_TX_ON;
> +
>   	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
>   		return -EFAULT;
>   
> @@ -2641,7 +2677,8 @@ static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>   	tsconf.flags = 0;
>   	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
>   						  HWTSTAMP_FILTER_NONE;
> -	tsconf.tx_type = HWTSTAMP_TX_OFF;
> +	tsconf.tx_type = vi->enable_tx_tstamp ? HWTSTAMP_TX_ON :
> +						HWTSTAMP_TX_OFF;
>   
>   	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
>   		return -EFAULT;
> @@ -3178,6 +3215,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>   		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
>   	}
>   
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP) &&
> +	    !vring_use_dma_api(vdev)) {
> +		vi->has_tx_tstamp = true;
> +		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> +	}
> +
>   	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
>   	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>   		vi->any_header_sg = true;
> @@ -3369,7 +3412,7 @@ static struct virtio_device_id id_table[] = {
>   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
> +	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
>   
>   static unsigned int features[] = {
>   	VIRTNET_FEATURES,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 71e16b53e9c1..cf5d5d1f9b14 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -238,7 +238,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
>    * unconditionally on data path.
>    */
>   
> -static bool vring_use_dma_api(struct virtio_device *vdev)
> +bool vring_use_dma_api(struct virtio_device *vdev)
>   {
>   	if (!virtio_has_dma_quirk(vdev))
>   		return true;
> @@ -257,6 +257,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>   
>   	return false;
>   }
> +EXPORT_SYMBOL_GPL(vring_use_dma_api);
>   
>   size_t virtio_max_dma_size(struct virtio_device *vdev)
>   {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 55ea329fe72a..5289e2812e95 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -140,6 +140,7 @@ int virtio_device_freeze(struct virtio_device *dev);
>   int virtio_device_restore(struct virtio_device *dev);
>   #endif
>   
> +bool vring_use_dma_api(struct virtio_device *vdev);
>   size_t virtio_max_dma_size(struct virtio_device *vdev);
>   
>   #define virtio_device_for_each_vq(vdev, vq) \
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index a5c84410cf92..b5d6f0c6cead 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,7 @@
>   					 * Steering */
>   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>   
> +#define VIRTIO_NET_F_TX_TSTAMP	  54	/* Device sends TAI transmit time */
>   #define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */


I wonder how much value to split into two features.

Thanks


>   #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
>   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Michael S. Tsirkin]

On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
> 
> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > Add optional PTP hardware tx timestamp offload for virtio-net.
> > 
> > Accurate RTT measurement requires timestamps close to the wire.
> > Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> > equivalent to VIRTIO_NET_F_RX_TSTAMP.
> > 
> > The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> > returned on completion. If the feature is negotiated, the device
> > either places the timestamp or clears the feature bit.
> > 
> > The timestamp straddles (virtual) hardware domains. Like PTP, use
> > international atomic time (CLOCK_TAI) as global clock base. The driver
> > must sync with the device, e.g., through kvm-clock.
> > 
> > Modify can_push to ensure that on tx completion the header, and thus
> > timestamp, is in a predicatable location at skb_vnet_hdr.
> > 
> > RFC: this implementation relies on the device writing to the buffer.
> > That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
> > The virtio changes should be a separate patch at the least.
> > 
> > Tested: modified txtimestamp.c to with h/w timestamping:
> >    -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
> >    +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
> >    + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
> > 
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> >   drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
> >   drivers/virtio/virtio_ring.c    |  3 +-
> >   include/linux/virtio.h          |  1 +
> >   include/uapi/linux/virtio_net.h |  1 +
> >   4 files changed, 56 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index ac44c5efa0bc..fc8ecd3a333a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -210,6 +210,12 @@ struct virtnet_info {
> >   	/* Device will pass rx timestamp. Requires has_rx_tstamp */
> >   	bool enable_rx_tstamp;
> > +	/* Device can pass CLOCK_TAI transmit time to the driver */
> > +	bool has_tx_tstamp;
> > +
> > +	/* Device will pass tx timestamp. Requires has_tx_tstamp */
> > +	bool enable_tx_tstamp;
> > +
> >   	/* Has control virtqueue */
> >   	bool has_cvq;
> > @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> >   	return stats.packets;
> >   }
> > +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
> > +				     struct sk_buff *skb)
> > +{
> > +	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> > +	const struct virtnet_info *vi = sq->vq->vdev->priv;
> > +	struct skb_shared_hwtstamps ts;
> > +
> > +	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> > +	    vi->enable_tx_tstamp) {
> > +		ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
> > +		skb_tstamp_tx(skb, &ts);
> 
> 
> This probably won't work since the buffer is read-only from the device. (See
> virtqueue_add_outbuf()).
> 
> Another issue that I vaguely remember that the virtio spec forbids out
> buffer after in buffer.

Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
have this statement:

	The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
	ments.


similarly

Device Requirements: The Virtqueue Descriptor Table
	A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
	buffer.



> 
> > +	}
> > +}
> > +
> >   static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
> >   {
> >   	unsigned int len;
> > @@ -1412,6 +1432,7 @@ static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
> >   		if (likely(!is_xdp_frame(ptr))) {
> >   			struct sk_buff *skb = ptr;
> > +			virtnet_record_tx_tstamp(sq, skb);
> >   			pr_debug("Sent skb %p\n", skb);
> >   			bytes += skb->len;
> > @@ -1558,7 +1579,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> >   	struct virtio_net_hdr_mrg_rxbuf *hdr;
> >   	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
> >   	struct virtnet_info *vi = sq->vq->vdev->priv;
> > -	struct virtio_net_hdr_v1_hash *ht;
> > +	struct virtio_net_hdr_hash_ts *ht;
> >   	int num_sg;
> >   	unsigned hdr_len = vi->hdr_len;
> >   	bool can_push;
> > @@ -1567,7 +1588,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> >   	can_push = vi->any_header_sg &&
> >   		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> > -		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> > +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len &&
> > +		!vi->enable_tx_tstamp;
> >   	/* Even if we can, don't push here yet as this would skew
> >   	 * csum_start offset below. */
> >   	if (can_push)
> > @@ -1588,10 +1610,12 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> >   		u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
> >   					    VIRTIO_NET_HASH_REPORT_OTHER;
> > -		ht->hash_value = cpu_to_le32(skb->hash);
> > -		ht->hash_report = cpu_to_le16(report);
> > -		ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> > +		ht->hash.value = cpu_to_le32(skb->hash);
> > +		ht->hash.report = cpu_to_le16(report);
> > +		ht->hash.flow_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> >   	}
> > +	if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
> > +		ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
> >   	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
> >   	if (can_push) {
> > @@ -2307,7 +2331,13 @@ static int virtnet_get_ts_info(struct net_device *dev,
> >   		info->rx_filters = HWTSTAMP_FILTER_NONE;
> >   	}
> > -	info->tx_types = HWTSTAMP_TX_OFF;
> > +	if (vi->has_tx_tstamp) {
> > +		info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
> > +					 SOF_TIMESTAMPING_RAW_HARDWARE;
> > +		info->tx_types = HWTSTAMP_TX_ON;
> > +	} else {
> > +		info->tx_types = HWTSTAMP_TX_OFF;
> > +	}
> >   	return 0;
> >   }
> > @@ -2616,7 +2646,8 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> >   		return -EFAULT;
> >   	if (tsconf.flags)
> >   		return -EINVAL;
> > -	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
> > +	if (tsconf.tx_type != HWTSTAMP_TX_OFF &&
> > +	    tsconf.tx_type != HWTSTAMP_TX_ON)
> >   		return -ERANGE;
> >   	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
> >   	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
> > @@ -2627,6 +2658,11 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> >   	else
> >   		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
> > +	if (!vi->has_tx_tstamp)
> > +		tsconf.tx_type = HWTSTAMP_TX_OFF;
> > +	else
> > +		vi->enable_tx_tstamp = tsconf.tx_type == HWTSTAMP_TX_ON;
> > +
> >   	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> >   		return -EFAULT;
> > @@ -2641,7 +2677,8 @@ static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> >   	tsconf.flags = 0;
> >   	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
> >   						  HWTSTAMP_FILTER_NONE;
> > -	tsconf.tx_type = HWTSTAMP_TX_OFF;
> > +	tsconf.tx_type = vi->enable_tx_tstamp ? HWTSTAMP_TX_ON :
> > +						HWTSTAMP_TX_OFF;
> >   	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> >   		return -EFAULT;
> > @@ -3178,6 +3215,12 @@ static int virtnet_probe(struct virtio_device *vdev)
> >   		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> >   	}
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP) &&
> > +	    !vring_use_dma_api(vdev)) {
> > +		vi->has_tx_tstamp = true;
> > +		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> > +	}
> > +
> >   	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
> >   	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> >   		vi->any_header_sg = true;
> > @@ -3369,7 +3412,7 @@ static struct virtio_device_id id_table[] = {
> >   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> > -	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
> > +	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
> >   static unsigned int features[] = {
> >   	VIRTNET_FEATURES,
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 71e16b53e9c1..cf5d5d1f9b14 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -238,7 +238,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
> >    * unconditionally on data path.
> >    */
> > -static bool vring_use_dma_api(struct virtio_device *vdev)
> > +bool vring_use_dma_api(struct virtio_device *vdev)
> >   {
> >   	if (!virtio_has_dma_quirk(vdev))
> >   		return true;
> > @@ -257,6 +257,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> >   	return false;
> >   }
> > +EXPORT_SYMBOL_GPL(vring_use_dma_api);
> >   size_t virtio_max_dma_size(struct virtio_device *vdev)
> >   {
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 55ea329fe72a..5289e2812e95 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -140,6 +140,7 @@ int virtio_device_freeze(struct virtio_device *dev);
> >   int virtio_device_restore(struct virtio_device *dev);
> >   #endif
> > +bool vring_use_dma_api(struct virtio_device *vdev);
> >   size_t virtio_max_dma_size(struct virtio_device *vdev);
> >   #define virtio_device_for_each_vq(vdev, vq) \
> > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> > index a5c84410cf92..b5d6f0c6cead 100644
> > --- a/include/uapi/linux/virtio_net.h
> > +++ b/include/uapi/linux/virtio_net.h
> > @@ -57,6 +57,7 @@
> >   					 * Steering */
> >   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> > +#define VIRTIO_NET_F_TX_TSTAMP	  54	/* Device sends TAI transmit time */
> >   #define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
> 
> 
> I wonder how much value to split into two features.
> 
> Thanks
> 
> 
> >   #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
> >   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Willem de Bruijn]

On Tue, Feb 9, 2021 at 11:39 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
> >
> > On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> > > From: Willem de Bruijn <willemb@google.com>
> > >
> > > Add optional PTP hardware tx timestamp offload for virtio-net.
> > >
> > > Accurate RTT measurement requires timestamps close to the wire.
> > > Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> > > equivalent to VIRTIO_NET_F_RX_TSTAMP.
> > >
> > > The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> > > returned on completion. If the feature is negotiated, the device
> > > either places the timestamp or clears the feature bit.
> > >
> > > The timestamp straddles (virtual) hardware domains. Like PTP, use
> > > international atomic time (CLOCK_TAI) as global clock base. The driver
> > > must sync with the device, e.g., through kvm-clock.
> > >
> > > Modify can_push to ensure that on tx completion the header, and thus
> > > timestamp, is in a predicatable location at skb_vnet_hdr.
> > >
> > > RFC: this implementation relies on the device writing to the buffer.
> > > That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
> > > The virtio changes should be a separate patch at the least.
> > >
> > > Tested: modified txtimestamp.c to with h/w timestamping:
> > >    -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
> > >    +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
> > >    + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
> > >
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > ---
> > >   drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
> > >   drivers/virtio/virtio_ring.c    |  3 +-
> > >   include/linux/virtio.h          |  1 +
> > >   include/uapi/linux/virtio_net.h |  1 +
> > >   4 files changed, 56 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index ac44c5efa0bc..fc8ecd3a333a 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -210,6 +210,12 @@ struct virtnet_info {
> > >     /* Device will pass rx timestamp. Requires has_rx_tstamp */
> > >     bool enable_rx_tstamp;
> > > +   /* Device can pass CLOCK_TAI transmit time to the driver */
> > > +   bool has_tx_tstamp;
> > > +
> > > +   /* Device will pass tx timestamp. Requires has_tx_tstamp */
> > > +   bool enable_tx_tstamp;
> > > +
> > >     /* Has control virtqueue */
> > >     bool has_cvq;
> > > @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > >     return stats.packets;
> > >   }
> > > +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
> > > +                                struct sk_buff *skb)
> > > +{
> > > +   const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> > > +   const struct virtnet_info *vi = sq->vq->vdev->priv;
> > > +   struct skb_shared_hwtstamps ts;
> > > +
> > > +   if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> > > +       vi->enable_tx_tstamp) {
> > > +           ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
> > > +           skb_tstamp_tx(skb, &ts);
> >
> >
> > This probably won't work since the buffer is read-only from the device. (See
> > virtqueue_add_outbuf()).
> >
> > Another issue that I vaguely remember that the virtio spec forbids out
> > buffer after in buffer.
>
> Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
> have this statement:
>
>         The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
>         ments.
>
>
> similarly
>
> Device Requirements: The Virtqueue Descriptor Table
>         A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
>         buffer.

Thanks. That's clear. So the clean solution would be to add a
device-writable descriptor after the existing device-readable ones.

And the device must be aware that this is to return the tstamp only.
In the example implementation of vhost, it has to exclude this last
descriptor from the msg->msg_iter iovec array with packet data
initialized at get_tx_bufs/init_iov_iter.

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Jason Wang]

On 2021/2/10 上午10:36, Willem de Bruijn wrote:
> On Tue, Feb 9, 2021 at 11:39 AM Michael S. Tsirkin<mst@redhat.com>  wrote:
>> On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
>>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
>>>> From: Willem de Bruijn<willemb@google.com>
>>>>
>>>> Add optional PTP hardware tx timestamp offload for virtio-net.
>>>>
>>>> Accurate RTT measurement requires timestamps close to the wire.
>>>> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
>>>> equivalent to VIRTIO_NET_F_RX_TSTAMP.
>>>>
>>>> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
>>>> returned on completion. If the feature is negotiated, the device
>>>> either places the timestamp or clears the feature bit.
>>>>
>>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
>>>> international atomic time (CLOCK_TAI) as global clock base. The driver
>>>> must sync with the device, e.g., through kvm-clock.
>>>>
>>>> Modify can_push to ensure that on tx completion the header, and thus
>>>> timestamp, is in a predicatable location at skb_vnet_hdr.
>>>>
>>>> RFC: this implementation relies on the device writing to the buffer.
>>>> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
>>>> The virtio changes should be a separate patch at the least.
>>>>
>>>> Tested: modified txtimestamp.c to with h/w timestamping:
>>>>     -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
>>>>     +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
>>>>     + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
>>>>
>>>> Signed-off-by: Willem de Bruijn<willemb@google.com>
>>>> ---
>>>>    drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
>>>>    drivers/virtio/virtio_ring.c    |  3 +-
>>>>    include/linux/virtio.h          |  1 +
>>>>    include/uapi/linux/virtio_net.h |  1 +
>>>>    4 files changed, 56 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index ac44c5efa0bc..fc8ecd3a333a 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -210,6 +210,12 @@ struct virtnet_info {
>>>>      /* Device will pass rx timestamp. Requires has_rx_tstamp */
>>>>      bool enable_rx_tstamp;
>>>> +   /* Device can pass CLOCK_TAI transmit time to the driver */
>>>> +   bool has_tx_tstamp;
>>>> +
>>>> +   /* Device will pass tx timestamp. Requires has_tx_tstamp */
>>>> +   bool enable_tx_tstamp;
>>>> +
>>>>      /* Has control virtqueue */
>>>>      bool has_cvq;
>>>> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>>>>      return stats.packets;
>>>>    }
>>>> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
>>>> +                                struct sk_buff *skb)
>>>> +{
>>>> +   const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
>>>> +   const struct virtnet_info *vi = sq->vq->vdev->priv;
>>>> +   struct skb_shared_hwtstamps ts;
>>>> +
>>>> +   if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
>>>> +       vi->enable_tx_tstamp) {
>>>> +           ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
>>>> +           skb_tstamp_tx(skb, &ts);
>>> This probably won't work since the buffer is read-only from the device. (See
>>> virtqueue_add_outbuf()).
>>>
>>> Another issue that I vaguely remember that the virtio spec forbids out
>>> buffer after in buffer.
>> Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
>> have this statement:
>>
>>          The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
>>          ments.
>>
>>
>> similarly
>>
>> Device Requirements: The Virtqueue Descriptor Table
>>          A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
>>          buffer.
> Thanks. That's clear. So the clean solution would be to add a
> device-writable descriptor after the existing device-readable ones.


I think so, but a question is the format for this tailer. I think it 
might be better to post a spec patch to discuss.

Thanks


>
> And the device must be aware that this is to return the tstamp only.
> In the example implementation of vhost, it has to exclude this last
> descriptor from the msg->msg_iter iovec array with packet data
> initialized at get_tx_bufs/init_iov_iter.
>

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Willem de Bruijn]

On Tue, Feb 9, 2021 at 11:15 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/2/10 上午10:36, Willem de Bruijn wrote:
> > On Tue, Feb 9, 2021 at 11:39 AM Michael S. Tsirkin<mst@redhat.com>  wrote:
> >> On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
> >>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
> >>>> From: Willem de Bruijn<willemb@google.com>
> >>>>
> >>>> Add optional PTP hardware tx timestamp offload for virtio-net.
> >>>>
> >>>> Accurate RTT measurement requires timestamps close to the wire.
> >>>> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> >>>> equivalent to VIRTIO_NET_F_RX_TSTAMP.
> >>>>
> >>>> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> >>>> returned on completion. If the feature is negotiated, the device
> >>>> either places the timestamp or clears the feature bit.
> >>>>
> >>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
> >>>> international atomic time (CLOCK_TAI) as global clock base. The driver
> >>>> must sync with the device, e.g., through kvm-clock.
> >>>>
> >>>> Modify can_push to ensure that on tx completion the header, and thus
> >>>> timestamp, is in a predicatable location at skb_vnet_hdr.
> >>>>
> >>>> RFC: this implementation relies on the device writing to the buffer.
> >>>> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
> >>>> The virtio changes should be a separate patch at the least.
> >>>>
> >>>> Tested: modified txtimestamp.c to with h/w timestamping:
> >>>>     -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
> >>>>     +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
> >>>>     + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
> >>>>
> >>>> Signed-off-by: Willem de Bruijn<willemb@google.com>
> >>>> ---
> >>>>    drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
> >>>>    drivers/virtio/virtio_ring.c    |  3 +-
> >>>>    include/linux/virtio.h          |  1 +
> >>>>    include/uapi/linux/virtio_net.h |  1 +
> >>>>    4 files changed, 56 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>> index ac44c5efa0bc..fc8ecd3a333a 100644
> >>>> --- a/drivers/net/virtio_net.c
> >>>> +++ b/drivers/net/virtio_net.c
> >>>> @@ -210,6 +210,12 @@ struct virtnet_info {
> >>>>      /* Device will pass rx timestamp. Requires has_rx_tstamp */
> >>>>      bool enable_rx_tstamp;
> >>>> +   /* Device can pass CLOCK_TAI transmit time to the driver */
> >>>> +   bool has_tx_tstamp;
> >>>> +
> >>>> +   /* Device will pass tx timestamp. Requires has_tx_tstamp */
> >>>> +   bool enable_tx_tstamp;
> >>>> +
> >>>>      /* Has control virtqueue */
> >>>>      bool has_cvq;
> >>>> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> >>>>      return stats.packets;
> >>>>    }
> >>>> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
> >>>> +                                struct sk_buff *skb)
> >>>> +{
> >>>> +   const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> >>>> +   const struct virtnet_info *vi = sq->vq->vdev->priv;
> >>>> +   struct skb_shared_hwtstamps ts;
> >>>> +
> >>>> +   if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> >>>> +       vi->enable_tx_tstamp) {
> >>>> +           ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
> >>>> +           skb_tstamp_tx(skb, &ts);
> >>> This probably won't work since the buffer is read-only from the device. (See
> >>> virtqueue_add_outbuf()).
> >>>
> >>> Another issue that I vaguely remember that the virtio spec forbids out
> >>> buffer after in buffer.
> >> Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
> >> have this statement:
> >>
> >>          The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
> >>          ments.
> >>
> >>
> >> similarly
> >>
> >> Device Requirements: The Virtqueue Descriptor Table
> >>          A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
> >>          buffer.
> > Thanks. That's clear. So the clean solution would be to add a
> > device-writable descriptor after the existing device-readable ones.
>
>
> I think so, but a question is the format for this tailer. I think it
> might be better to post a spec patch to discuss.

Okay I'll do that. I want to get something that works first, to make
sure that whatever I propose in spec is actually implementable.

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Jason Wang]

On 2021/2/10 上午12:38, Michael S. Tsirkin wrote:
> On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
>>> From: Willem de Bruijn<willemb@google.com>
>>>
>>> Add optional PTP hardware tx timestamp offload for virtio-net.
>>>
>>> Accurate RTT measurement requires timestamps close to the wire.
>>> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
>>> equivalent to VIRTIO_NET_F_RX_TSTAMP.
>>>
>>> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
>>> returned on completion. If the feature is negotiated, the device
>>> either places the timestamp or clears the feature bit.
>>>
>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
>>> international atomic time (CLOCK_TAI) as global clock base. The driver
>>> must sync with the device, e.g., through kvm-clock.
>>>
>>> Modify can_push to ensure that on tx completion the header, and thus
>>> timestamp, is in a predicatable location at skb_vnet_hdr.
>>>
>>> RFC: this implementation relies on the device writing to the buffer.
>>> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
>>> The virtio changes should be a separate patch at the least.
>>>
>>> Tested: modified txtimestamp.c to with h/w timestamping:
>>>     -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
>>>     +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
>>>     + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
>>>
>>> Signed-off-by: Willem de Bruijn<willemb@google.com>
>>> ---
>>>    drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
>>>    drivers/virtio/virtio_ring.c    |  3 +-
>>>    include/linux/virtio.h          |  1 +
>>>    include/uapi/linux/virtio_net.h |  1 +
>>>    4 files changed, 56 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index ac44c5efa0bc..fc8ecd3a333a 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -210,6 +210,12 @@ struct virtnet_info {
>>>    	/* Device will pass rx timestamp. Requires has_rx_tstamp */
>>>    	bool enable_rx_tstamp;
>>> +	/* Device can pass CLOCK_TAI transmit time to the driver */
>>> +	bool has_tx_tstamp;
>>> +
>>> +	/* Device will pass tx timestamp. Requires has_tx_tstamp */
>>> +	bool enable_tx_tstamp;
>>> +
>>>    	/* Has control virtqueue */
>>>    	bool has_cvq;
>>> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>>>    	return stats.packets;
>>>    }
>>> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
>>> +				     struct sk_buff *skb)
>>> +{
>>> +	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
>>> +	const struct virtnet_info *vi = sq->vq->vdev->priv;
>>> +	struct skb_shared_hwtstamps ts;
>>> +
>>> +	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
>>> +	    vi->enable_tx_tstamp) {
>>> +		ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
>>> +		skb_tstamp_tx(skb, &ts);
>> This probably won't work since the buffer is read-only from the device. (See
>> virtqueue_add_outbuf()).
>>
>> Another issue that I vaguely remember that the virtio spec forbids out
>> buffer after in buffer.
> Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
> have this statement:
>
> 	The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
> 	ments.
>
>
> similarly
>
> Device Requirements: The Virtqueue Descriptor Table
> 	A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
> 	buffer.
>
>

Exactly. But I wonder what's the rationale behinds those requirements?

Thanks

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Michael S. Tsirkin]

On Mon, Feb 08, 2021 at 01:55:57PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Add optional PTP hardware tx timestamp offload for virtio-net.
> 
> Accurate RTT measurement requires timestamps close to the wire.
> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> equivalent to VIRTIO_NET_F_RX_TSTAMP.
> 
> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> returned on completion. If the feature is negotiated, the device
> either places the timestamp or clears the feature bit.
> 
> The timestamp straddles (virtual) hardware domains. Like PTP, use
> international atomic time (CLOCK_TAI) as global clock base. The driver
> must sync with the device, e.g., through kvm-clock.
> 
> Modify can_push to ensure that on tx completion the header, and thus
> timestamp, is in a predicatable location at skb_vnet_hdr.
> 
> RFC: this implementation relies on the device writing to the buffer.
> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.

If you do something like this, please do it in the validate
callback and clear the features you aren't using.

> The virtio changes should be a separate patch at the least.
> 
> Tested: modified txtimestamp.c to with h/w timestamping:
>   -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
>   +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
>   + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
>  drivers/virtio/virtio_ring.c    |  3 +-
>  include/linux/virtio.h          |  1 +
>  include/uapi/linux/virtio_net.h |  1 +
>  4 files changed, 56 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ac44c5efa0bc..fc8ecd3a333a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -210,6 +210,12 @@ struct virtnet_info {
>  	/* Device will pass rx timestamp. Requires has_rx_tstamp */
>  	bool enable_rx_tstamp;
>  
> +	/* Device can pass CLOCK_TAI transmit time to the driver */
> +	bool has_tx_tstamp;
> +
> +	/* Device will pass tx timestamp. Requires has_tx_tstamp */
> +	bool enable_tx_tstamp;
> +
>  	/* Has control virtqueue */
>  	bool has_cvq;
>  
> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  	return stats.packets;
>  }
>  
> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
> +				     struct sk_buff *skb)
> +{
> +	const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
> +	const struct virtnet_info *vi = sq->vq->vdev->priv;
> +	struct skb_shared_hwtstamps ts;
> +
> +	if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
> +	    vi->enable_tx_tstamp) {
> +		ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
> +		skb_tstamp_tx(skb, &ts);
> +	}
> +}
> +
>  static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
>  {
>  	unsigned int len;
> @@ -1412,6 +1432,7 @@ static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
>  		if (likely(!is_xdp_frame(ptr))) {
>  			struct sk_buff *skb = ptr;
>  
> +			virtnet_record_tx_tstamp(sq, skb);
>  			pr_debug("Sent skb %p\n", skb);
>  
>  			bytes += skb->len;
> @@ -1558,7 +1579,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  	struct virtio_net_hdr_mrg_rxbuf *hdr;
>  	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
>  	struct virtnet_info *vi = sq->vq->vdev->priv;
> -	struct virtio_net_hdr_v1_hash *ht;
> +	struct virtio_net_hdr_hash_ts *ht;
>  	int num_sg;
>  	unsigned hdr_len = vi->hdr_len;
>  	bool can_push;
> @@ -1567,7 +1588,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  
>  	can_push = vi->any_header_sg &&
>  		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> -		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len &&
> +		!vi->enable_tx_tstamp;
>  	/* Even if we can, don't push here yet as this would skew
>  	 * csum_start offset below. */
>  	if (can_push)
> @@ -1588,10 +1610,12 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  		u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
>  					    VIRTIO_NET_HASH_REPORT_OTHER;
>  
> -		ht->hash_value = cpu_to_le32(skb->hash);
> -		ht->hash_report = cpu_to_le16(report);
> -		ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> +		ht->hash.value = cpu_to_le32(skb->hash);
> +		ht->hash.report = cpu_to_le16(report);
> +		ht->hash.flow_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
>  	}
> +	if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
> +		ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
>  
>  	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
>  	if (can_push) {
> @@ -2307,7 +2331,13 @@ static int virtnet_get_ts_info(struct net_device *dev,
>  		info->rx_filters = HWTSTAMP_FILTER_NONE;
>  	}
>  
> -	info->tx_types = HWTSTAMP_TX_OFF;
> +	if (vi->has_tx_tstamp) {
> +		info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
> +					 SOF_TIMESTAMPING_RAW_HARDWARE;
> +		info->tx_types = HWTSTAMP_TX_ON;
> +	} else {
> +		info->tx_types = HWTSTAMP_TX_OFF;
> +	}
>  
>  	return 0;
>  }
> @@ -2616,7 +2646,8 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  		return -EFAULT;
>  	if (tsconf.flags)
>  		return -EINVAL;
> -	if (tsconf.tx_type != HWTSTAMP_TX_OFF)
> +	if (tsconf.tx_type != HWTSTAMP_TX_OFF &&
> +	    tsconf.tx_type != HWTSTAMP_TX_ON)
>  		return -ERANGE;
>  	if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
>  	    tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
> @@ -2627,6 +2658,11 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	else
>  		vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
>  
> +	if (!vi->has_tx_tstamp)
> +		tsconf.tx_type = HWTSTAMP_TX_OFF;
> +	else
> +		vi->enable_tx_tstamp = tsconf.tx_type == HWTSTAMP_TX_ON;
> +
>  	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
>  		return -EFAULT;
>  
> @@ -2641,7 +2677,8 @@ static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	tsconf.flags = 0;
>  	tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
>  						  HWTSTAMP_FILTER_NONE;
> -	tsconf.tx_type = HWTSTAMP_TX_OFF;
> +	tsconf.tx_type = vi->enable_tx_tstamp ? HWTSTAMP_TX_ON :
> +						HWTSTAMP_TX_OFF;
>  
>  	if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
>  		return -EFAULT;
> @@ -3178,6 +3215,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
>  	}
>  
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP) &&
> +	    !vring_use_dma_api(vdev)) {
> +		vi->has_tx_tstamp = true;
> +		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> +	}
> +
>  	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
>  	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>  		vi->any_header_sg = true;
> @@ -3369,7 +3412,7 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>  	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>  	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
> +	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 71e16b53e9c1..cf5d5d1f9b14 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -238,7 +238,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
>   * unconditionally on data path.
>   */
>  
> -static bool vring_use_dma_api(struct virtio_device *vdev)
> +bool vring_use_dma_api(struct virtio_device *vdev)
>  {
>  	if (!virtio_has_dma_quirk(vdev))
>  		return true;
> @@ -257,6 +257,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>  
>  	return false;
>  }
> +EXPORT_SYMBOL_GPL(vring_use_dma_api);
>  
>  size_t virtio_max_dma_size(struct virtio_device *vdev)
>  {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 55ea329fe72a..5289e2812e95 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -140,6 +140,7 @@ int virtio_device_freeze(struct virtio_device *dev);
>  int virtio_device_restore(struct virtio_device *dev);
>  #endif
>  
> +bool vring_use_dma_api(struct virtio_device *vdev);
>  size_t virtio_max_dma_size(struct virtio_device *vdev);
>  
>  #define virtio_device_for_each_vq(vdev, vq) \
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index a5c84410cf92..b5d6f0c6cead 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,7 @@
>  					 * Steering */
>  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>  
> +#define VIRTIO_NET_F_TX_TSTAMP	  54	/* Device sends TAI transmit time */
>  #define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
>  #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
>  #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
> -- 
> 2.30.0.478.g8a0d178c01-goog

Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Willem de Bruijn]

On Tue, Feb 9, 2021 at 4:43 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Feb 08, 2021 at 01:55:57PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Add optional PTP hardware tx timestamp offload for virtio-net.
> >
> > Accurate RTT measurement requires timestamps close to the wire.
> > Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
> > equivalent to VIRTIO_NET_F_RX_TSTAMP.
> >
> > The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> > returned on completion. If the feature is negotiated, the device
> > either places the timestamp or clears the feature bit.
> >
> > The timestamp straddles (virtual) hardware domains. Like PTP, use
> > international atomic time (CLOCK_TAI) as global clock base. The driver
> > must sync with the device, e.g., through kvm-clock.
> >
> > Modify can_push to ensure that on tx completion the header, and thus
> > timestamp, is in a predicatable location at skb_vnet_hdr.
> >
> > RFC: this implementation relies on the device writing to the buffer.
> > That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
>
> If you do something like this, please do it in the validate
> callback and clear the features you aren't using.

Ah yes. Thanks for the tip. I'll do that ..

.. once I'm sure that this approach of using an outbuf for I/O is
actually allowed behavior. I'm not entirely convinced yet myself.
Jason also pointed out more specific concerns. I'll look into that
further.

[PATCH RFC v2 4/4] virtio-net: support future packet transmit time

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Add optional transmit time (SO_TXTIME) offload for virtio-net.

The Linux TCP/IP stack tries to avoid bursty transmission and network
congestion through pacing: computing an skb delivery time based on
congestion information. Userspace protocol implementations can achieve
the same with SO_TXTIME. This may also reduce scheduling jitter and
improve RTT estimation.

Pacing can be implemented in ETF or FQ qdiscs or offloaded to NIC
hardware. Allow virtio-net driver to offload for the same reasons.

The timestamp straddles (virtual) hardware domains. Like PTP, use
international atomic time (CLOCK_TAI) as global clock base. The driver
must sync with the device, e.g., through kvm-clock.

Changes RFC - RFCv2
  - rename from transmit timestamp to future packet transmit time
  - convert cpu_to_virtioXX to cpu_to_leXX

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 13 ++++++++++++-
 include/uapi/linux/virtio_net.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fc8ecd3a333a..c09d19b97f42 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -216,6 +216,9 @@ struct virtnet_info {
 	/* Device will pass tx timestamp. Requires has_tx_tstamp */
 	bool enable_tx_tstamp;
 
+	/* Driver will pass CLOCK_TAI delivery time to the device */
+	bool has_tx_time;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -1616,6 +1619,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	}
 	if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
 		ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
+	if (vi->has_tx_time && skb->tstamp)
+		ht->tstamp = cpu_to_le64(skb->tstamp);
 
 	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
 	if (can_push) {
@@ -3221,6 +3226,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
 	}
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TIME)) {
+		vi->has_tx_time = true;
+		vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
+	}
+
 	if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
 	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 		vi->any_header_sg = true;
@@ -3412,7 +3422,8 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
+	VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP, \
+	VIRTIO_NET_F_TX_TIME
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index b5d6f0c6cead..7ca99a2ee1a3 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,7 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#define VIRTIO_NET_F_TX_TIME	  53	/* Driver sets TAI delivery time */
 #define VIRTIO_NET_F_TX_TSTAMP	  54	/* Device sends TAI transmit time */
 #define VIRTIO_NET_F_RX_TSTAMP	  55	/* Device sends TAI receive time */
 #define VIRTIO_NET_F_TX_HASH	  56	/* Driver sends hash report */
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[Willem de Bruijn]

On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> RFCv2 for four new features to the virtio network device:
>
> 1. pass tx flow state to host, for routing + telemetry
> 2. pass rx tstamp to guest, for better RTT estimation
> 3. pass tx tstamp to guest, idem
> 3. pass tx delivery time to host, for accurate pacing
>
> All would introduce an extension to the virtio spec.
> Concurrently with code review I will write ballots to
> https://www.oasis-open.org/committees/ballots.php?wg_abbrev=virtio
>
> These changes are to the driver side. Evaluation additionally requires
> achanges to qemu and at least one back-end. I implemented preliminary
> support in Linux vhost-net. Both patches available through github at
>
> https://github.com/wdebruij/linux/tree/virtio-net-txhash-2
> https://github.com/wdebruij/qemu/tree/virtio-net-txhash-2
>
> Changes RFC -> RFCv2
>   - add transmit timestamp patch
>   - see individual patches for other changes
>
> Willem de Bruijn (4):
>   virtio-net: support transmit hash report
>   virtio-net: support receive timestamp
>   virtio-net: support transmit timestamp
>   virtio-net: support future packet transmit time

Seeing Yuri's patchset adding new features reminded me that I did not
follow-up on this patch series on the list.

The patches themselves are mostly in good shape. The last tx tstamp
issue can be resolved.

But the device implementation I target only supports legacy mode.
Below conversation that we had in one of the patches makes clear that
supporting this in legacy is not feasible. Nor is upgrading that
device in the short term. Until there is a device implementation that
implements these offloads, these features are a dead letter. Not moving
forward for now.

Somewhat related: is there a plan for when we run out of 64 feature bits?

> > > Actually, would it be possible to make new features available on
> > > legacy devices? There is nothing in the features bits precluding it.
> >
> > I think it won't be possible: you are using feature bit 55,
> > legacy devices have up to 32 feature bits. And of course the
> > header looks a bit differently for legacy, you would have to add special
> > code to handle that when mergeable buffers are off.
>
> I think I can make the latter work. I did start without a dependency
> on the v1 header initially.
>
> Feature bit array length I had not considered. Good point. Need to
> think about that. It would be very appealing if in particular the
> tx-hash feature could work in legacy mode.

Re: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[Jason Wang]

On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn <willemb@google.com> wrote:
>
> On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.com>
> >
> > RFCv2 for four new features to the virtio network device:
> >
> > 1. pass tx flow state to host, for routing + telemetry
> > 2. pass rx tstamp to guest, for better RTT estimation
> > 3. pass tx tstamp to guest, idem
> > 3. pass tx delivery time to host, for accurate pacing
> >
> > All would introduce an extension to the virtio spec.
> > Concurrently with code review I will write ballots to
> > https://www.oasis-open.org/committees/ballots.php?wg_abbrev=virtio
> >
> > These changes are to the driver side. Evaluation additionally requires
> > achanges to qemu and at least one back-end. I implemented preliminary
> > support in Linux vhost-net. Both patches available through github at
> >
> > https://github.com/wdebruij/linux/tree/virtio-net-txhash-2
> > https://github.com/wdebruij/qemu/tree/virtio-net-txhash-2
> >
> > Changes RFC -> RFCv2
> >   - add transmit timestamp patch
> >   - see individual patches for other changes
> >
> > Willem de Bruijn (4):
> >   virtio-net: support transmit hash report
> >   virtio-net: support receive timestamp
> >   virtio-net: support transmit timestamp
> >   virtio-net: support future packet transmit time
>
> Seeing Yuri's patchset adding new features reminded me that I did not
> follow-up on this patch series on the list.
>
> The patches themselves are mostly in good shape. The last tx tstamp
> issue can be resolved.
>
> But the device implementation I target only supports legacy mode.
> Below conversation that we had in one of the patches makes clear that
> supporting this in legacy is not feasible. Nor is upgrading that
> device in the short term. Until there is a device implementation that
> implements these offloads, these features are a dead letter. Not moving
> forward for now.
>
> Somewhat related: is there a plan for when we run out of 64 feature bits?

A quick thought: we need add (or reserve) a new feature bit to
indicate that we need more bits, and have transport specific
implementation of those extra bits negotiation. E.g for PCI, we can
introduce new fields in the capability.

Thanks

>
> > > > Actually, would it be possible to make new features available on
> > > > legacy devices? There is nothing in the features bits precluding it.
> > >
> > > I think it won't be possible: you are using feature bit 55,
> > > legacy devices have up to 32 feature bits. And of course the
> > > header looks a bit differently for legacy, you would have to add special
> > > code to handle that when mergeable buffers are off.
> >
> > I think I can make the latter work. I did start without a dependency
> > on the v1 header initially.
> >
> > Feature bit array length I had not considered. Good point. Need to
> > think about that. It would be very appealing if in particular the
> > tx-hash feature could work in legacy mode.
>

Re: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time

[Willem de Bruijn]

On Fri, May 14, 2021 at 3:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 14, 2021 at 6:50 AM Willem de Bruijn <willemb@google.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 1:56 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb@google.com>
> > >
> > > RFCv2 for four new features to the virtio network device:
> > >
> > > 1. pass tx flow state to host, for routing + telemetry
> > > 2. pass rx tstamp to guest, for better RTT estimation
> > > 3. pass tx tstamp to guest, idem
> > > 3. pass tx delivery time to host, for accurate pacing
> > >
> > > All would introduce an extension to the virtio spec.
> > > Concurrently with code review I will write ballots to
> > > https://www.oasis-open.org/committees/ballots.php?wg_abbrev=virtio
> > >
> > > These changes are to the driver side. Evaluation additionally requires
> > > achanges to qemu and at least one back-end. I implemented preliminary
> > > support in Linux vhost-net. Both patches available through github at
> > >
> > > https://github.com/wdebruij/linux/tree/virtio-net-txhash-2
> > > https://github.com/wdebruij/qemu/tree/virtio-net-txhash-2
> > >
> > > Changes RFC -> RFCv2
> > >   - add transmit timestamp patch
> > >   - see individual patches for other changes
> > >
> > > Willem de Bruijn (4):
> > >   virtio-net: support transmit hash report
> > >   virtio-net: support receive timestamp
> > >   virtio-net: support transmit timestamp
> > >   virtio-net: support future packet transmit time
> >
> > Seeing Yuri's patchset adding new features reminded me that I did not
> > follow-up on this patch series on the list.
> >
> > The patches themselves are mostly in good shape. The last tx tstamp
> > issue can be resolved.
> >
> > But the device implementation I target only supports legacy mode.
> > Below conversation that we had in one of the patches makes clear that
> > supporting this in legacy is not feasible. Nor is upgrading that
> > device in the short term. Until there is a device implementation that
> > implements these offloads, these features are a dead letter. Not moving
> > forward for now.
> >
> > Somewhat related: is there a plan for when we run out of 64 feature bits?
>
> A quick thought: we need add (or reserve) a new feature bit to
> indicate that we need more bits, and have transport specific
> implementation of those extra bits negotiation. E.g for PCI, we can
> introduce new fields in the capability.

Thanks Jason. Yes, that makes sense to me.

The difference from 32 to 64 bit between virtio_pci_legacy.c and
virtio_pci_modern.c is a good example:

  static u64 vp_get_features(struct virtio_device *vdev)
  {
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);

        /* When someone needs more than 32 feature bits, we'll need to
         * steal a bit to indicate that the rest are somewhere else. */
        return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES);
 }

  u64 vp_modern_get_features(struct virtio_pci_modern_device *mdev)
  {
        struct virtio_pci_common_cfg __iomem *cfg = mdev->common;

        u64 features;

        vp_iowrite32(0, &cfg->device_feature_select);
        features = vp_ioread32(&cfg->device_feature);
        vp_iowrite32(1, &cfg->device_feature_select);
        features |= ((u64)vp_ioread32(&cfg->device_feature) << 32);

        return features;
  }

device_feature_select is a 32-bit field, of which only values 0 and 1
are defined so far, per the virtio 1.1 spec:

"
device_feature_select
The driver uses this to select which feature bits device_feature
shows. Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature
Bits 32 to 63, etc.
"

That leaves plenty of room for expansion, at least for pci devices.

>
> >
> > > > > Actually, would it be possible to make new features available on
> > > > > legacy devices? There is nothing in the features bits precluding it.
> > > >
> > > > I think it won't be possible: you are using feature bit 55,
> > > > legacy devices have up to 32 feature bits. And of course the
> > > > header looks a bit differently for legacy, you would have to add special
> > > > code to handle that when mergeable buffers are off.
> > >
> > > I think I can make the latter work. I did start without a dependency
> > > on the v1 header initially.
> > >
> > > Feature bit array length I had not considered. Good point. Need to
> > > think about that. It would be very appealing if in particular the
> > > tx-hash feature could work in legacy mode.
> >
>