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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-JLcuaRckKGJSt9Oi-_7W2=1t9FLR6=Thdh5krh6+L9sw@mail.gmail.com>
Date:   Wed, 10 Feb 2021 09:40:44 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        virtualization@...ts.linux-foundation.org,
        Network Development <netdev@...r.kernel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

On Tue, Feb 9, 2021 at 11:15 PM Jason Wang <jasowang@...hat.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@...hat.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@...gle.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@...gle.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.