Re: [PATCH RFC v4 2/6] virtio-net: support receive timestamp

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On 2026-11-01 at 10:04 -05, Willem de Bruijn <willemdebruijn.kernel@gmail.com> 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 <s.trumtrar@pengutronix.de>
> > 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    |