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

[Jason Wang <jasowang@redhat.com>]

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