On Mon, Dec 28, 2020 at 7:55 PM Michael S. Tsirkin [off-list ref] wrote:

On Mon, Dec 28, 2020 at 02:30:31PM -0500, Willem de Bruijn wrote:

quoted

On Mon, Dec 28, 2020 at 12:29 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Dec 28, 2020 at 11:22:32AM -0500, Willem de Bruijn wrote:

quoted

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

Add optional PTP hardware 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 host may
pass receive timestamps for all or some packets. A timestamp is valid
if non-zero.

The timestamp straddles (virtual) hardware domains. Like PTP, use
international atomic time (CLOCK_TAI) as global clock base. It is
guest responsibility to sync with host, e.g., through kvm-clock.

Signed-off-by: Willem de Bruijn <willemb@google.com>

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index f6881b5b77ee..0ffe2eeebd4a 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    /* Host sends TAI receive time */
 #define VIRTIO_NET_F_TX_HASH   56    /* Guest sends hash report */
 #define VIRTIO_NET_F_HASH_REPORT  57 /* Supports hash report */
 #define VIRTIO_NET_F_RSS       60    /* Supports RSS RX steering */

@@ -182,6 +183,17 @@ struct virtio_net_hdr_v1_hash {
      };
 };

+struct virtio_net_hdr_v12 {
+     struct virtio_net_hdr_v1 hdr;
+     struct {
+             __le32 value;
+             __le16 report;
+             __le16 flow_state;
+     } hash;
+     __virtio32 reserved;
+     __virtio64 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

So it looks like VIRTIO_NET_F_RX_TSTAMP should depend on both
VIRTIO_NET_F_RX_TSTAMP and VIRTIO_NET_F_HASH_REPORT then?

Do you mean VIRTIO_NET_F_TX_TSTAMP depends on VIRTIO_NET_F_RX_TSTAMP?

I think if either is enabled we need to enable the extended layout.
Regardless of whether TX_HASH or HASH_REPORT are enabled. If they are
not, then those fields are ignored.

I mean do we waste the 8 bytes for hash if TSTAMP is set but HASH is not?
If TSTAMP depends on HASH then point is moot.

True, but the two features really are independent.

I did consider using configurable metadata layout depending on
features negotiated. If there are tons of optional extensions, that
makes sense. But it is more complex and parsing error prone. With a
handful of options each of a few bytes, that did not seem worth the
cost to me at this point.

And importantly, such a mode can always be added later as a separate
VIRTIO_NET_F_PACKED_HEADER option.

If anything, perhaps if we increase the virtio_net_hdr_* allocation,
we should allocate some additional reserved space now? As each new
size introduces quite a bit of boilerplate. Also, e.g., in qemu just
to pass the sizes between virtio-net driver and vhost-net device.