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

From: "Michael S. Tsirkin" <[email-masked]>
Date: Tue, 9 Feb 2021 11:38:54 -0500

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 <[email-masked]>
> > 
> > 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 <[email-masked]>
> > ---
> >   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 */