On 2026-11-01 at 10:04 -05, Willem de Bruijn <[email protected]> wrote:
> Steffen Trumtrar wrote: > > Add optional hardware rx timestamp offload for virtio-net. > > > > Introduce the virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the > > virtio-net header grows by room for a 64 bit TAI timestamp (header > > size 40 bytes), stored in struct virtio_net_common_hdr. The layout is > > cumulative: the field is at the same offset whether or not the > > features before it were negotiated. The fixed layout needs > > VIRTIO_F_VERSION_1, so the feature is cleared without it. > > > > Implement ndo_hwtstamp_get/set so the timestamping can be enabled > > and disabled at runtime with SIOCSHWTSTAMP. The driver cannot filter > > what the device timestamps, so any rx filter other than NONE becomes > > ALL. The timestamp is read before the packet is handed to the XDP > > program or the stack, and attached to the skb with skb_hwtstamps(). > > It is a raw TAI value; there is no PHC. > > > > The header size is negotiated per device and also applies to > > transmitted packets. On transmit, zero the header behind the fields > > the driver fills in, so the device sees a zero tstamp and no stale > > skb->cb or headroom bytes. > > > > On receive in big packet mode, the header is placed in front of the > > data in the first page. Grow padded_vnet_hdr to 48 bytes so the > > 40 byte header still fits there. > > > > Tested in a guest with a patched QEMU: > > guest: ./timestamping eth0 \ > > SOF_TIMESTAMPING_RAW_HARDWARE \ > > SOF_TIMESTAMPING_RX_HARDWARE > > host: nc -4 -u 192.168.1.1 319 > > > > Based on work by Willem de Bruijn. > > > > Signed-off-by: Steffen Trumtrar <[email protected]> > > Assisted-by: Claude:claude-opus-5-5 > > --- > > drivers/net/virtio_net.c | 136 > > +++++++++++++++++++++++++++++++++++----- > > include/linux/virtio_net.h | 10 +++ > > include/uapi/linux/virtio_net.h | 1 + > > 3 files changed, 131 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 17d3e32eed9d6..23af10c613b48 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -20,6 +20,7 @@ > > #include <linux/filter.h> > > #include <linux/kernel.h> > > #include <linux/dim.h> > > +#include <linux/unaligned.h> > > #include <net/route.h> > > #include <net/xdp.h> > > #include <net/net_failover.h> > > @@ -416,6 +417,12 @@ struct virtnet_info { > > u32 rss_hash_types_supported; > > u32 rss_hash_types_saved; > > > > + /* Device passes time stamps to the driver */ > > + bool has_rx_tstamp; > > + bool hwts_rx_en; > > nit: cryptic name. rx_tstamp_supported and rx_tstamp_enabled or so? Sure, I'll rename them. > > + > > + struct kernel_hwtstamp_config tstamp_config; > > + > > /* Has control virtqueue */ > > bool has_cvq; > > > +static int virtnet_get_ts_info(struct net_device *dev, > > + struct kernel_ethtool_ts_info *info) > > +{ > > + struct virtnet_info *vi = netdev_priv(dev); > > + > > + /* setup default software timestamp */ > > + ethtool_op_get_ts_info(dev, info); > > + > > + if (vi->has_rx_tstamp) { > > + info->so_timestamping |= > > + SOF_TIMESTAMPING_RX_HARDWARE | > > + SOF_TIMESTAMPING_RAW_HARDWARE; > > + > > + info->tx_types = BIT(HWTSTAMP_TX_OFF); > > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | > > + BIT(HWTSTAMP_FILTER_ALL); > > + } > > `ethtool_op_get_ts_info()` leaves `info->phc_index = -1`, which breaks > Ethtool > Netlink `ETHTOOL_MSG_TSCONFIG_GET` and `ETHTOOL_MSG_TSCONFIG_SET` > (`ethtool --show-hwtstamp-cfg` / `--set-hwtstamp-cfg`): > `tsconfig_prepare_data()` > in `net/ethtool/tsconfig.c` rejects `ts_info.phc_index == -1` with > `-ENODEV`. > > Either `net/ethtool/tsconfig.c` must be taught to allow PHC-less > `ndo_hwtstamp_*` > drivers, or the device needs an associated PHC (e.g. `virtio_rtc`). > Right. (explanation mostly for myself to understand the problem) The PHC-less case looks easy to allow in tsconfig: tsconfig_reply_size() and tsconfig_fill_reply() already leave out ETHTOOL_A_TSCONFIG_HWTSTAMP_PROVIDER when the index is -1, only tsconfig_prepare_data() refuses it with -ENODEV. For TSCONFIG_SET it is worse than for GET: ethnl_set_tsconfig() calls dev_set_hwtstamp_phylib(), so the driver's ndo_hwtstamp_set() has already applied the config, and only then tsconfig_send_reply() fails in tsconfig_prepare_data(). Userspace gets -ENODEV although timestamping is now on. virtio-net is not the only one there: ngbe/txgbe (libwx) report hardware timestamping with phc_index -1 if CONFIG_PTP_1588_CLOCK=n, and dpaa does the same without a ptimer-handle. I can send that as a separate patch. Which clock the timestamps are in is a separate question, see my answer to the cover letter. > > > @@ -7250,7 +7353,8 @@ static struct virtio_device_id id_table[] = { > > VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \ > > VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \ > > VIRTIO_NET_F_VQ_NOTF_COAL, \ > > - VIRTIO_NET_F_GUEST_HDRLEN, VIRTIO_NET_F_DEVICE_STATS > > + VIRTIO_NET_F_GUEST_HDRLEN, VIRTIO_NET_F_DEVICE_STATS, \ > > + VIRTIO_NET_F_RX_TSTAMP > > `VIRTNET_FEATURES` expands into `features_legacy[]` as well as > `features[]`. > Legacy virtio only supports 32 feature bits (`0..31`) and never has > `VIRTIO_F_VERSION_1`. Move `VIRTIO_NET_F_RX_TSTAMP` (bit 49) into > `features[]` > alongside `VIRTIO_NET_F_*_UDP_TUNNEL_GSO*`. > Oops, yes. Will move it to features[]. Steffen -- Pengutronix e.K. | Dipl.-Inform. Steffen Trumtrar | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |
