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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 04, 2026 at 12:55:59PM -0500, Willem de Bruijn wrote:
> Jason Wang wrote:
> > On Mon, Feb 2, 2026 at 5:06 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Steffen Trumtrar wrote:
> > > > Add optional hardware rx timestamp offload for virtio-net.
> > > >
> > > > Introduce virtio feature VIRTIO_NET_F_TSTAMP. If negotiated, the
> > > > virtio-net header is expanded with room for a timestamp.
> > > >
> > > > To get and set the hwtstamp the functions ndo_hwtstamp_set/get need
> > > > to be implemented. This allows filtering the packets and only time stamp
> > > > the packets where the filter matches. This way, the timestamping can
> > > > be en/disabled at runtime.
> > > >
> > > > Tested:
> > > >   guest: ./timestamping eth0 \
> > > >           SOF_TIMESTAMPING_RAW_HARDWARE \
> > > >           SOF_TIMESTAMPING_RX_HARDWARE
> > > >   host: nc -4 -u 192.168.1.1 319
> > > >
> > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > >
> > > > --
> > > >   Changes to last version:
> > > >   - rework series to use flow filters
> > > >   - add new struct virtio_net_hdr_v1_hash_tunnel_ts
> > > >   - original work done by: Willem de Bruijn <willemb@google.com>
> > > > ---
> > > >  drivers/net/virtio_net.c        | 136 ++++++++++++++++++++++++++++++++++++----
> > > >  include/uapi/linux/virtio_net.h |   9 +++
> > > >  2 files changed, 133 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 1bb3aeca66c6e..4e8d9b20c1b34 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -429,6 +429,9 @@ struct virtnet_info {
> > > >       struct virtio_net_rss_config_trailer rss_trailer;
> > > >       u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> > > >
> > > > +     /* Device passes time stamps from the driver */
> > > > +     bool has_tstamp;
> > > > +
> > > >       /* Has control virtqueue */
> > > >       bool has_cvq;
> > > >
> > > > @@ -475,6 +478,8 @@ struct virtnet_info {
> > > >
> > > >       struct control_buf *ctrl;
> > > >
> > > > +     struct kernel_hwtstamp_config tstamp_config;
> > > > +
> > > >       /* Ethtool settings */
> > > >       u8 duplex;
> > > >       u32 speed;
> > > > @@ -511,6 +516,7 @@ struct virtio_net_common_hdr {
> > > >               struct virtio_net_hdr_mrg_rxbuf mrg_hdr;
> > > >               struct virtio_net_hdr_v1_hash hash_v1_hdr;
> > > >               struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
> > > > +             struct virtio_net_hdr_v1_hash_tunnel_ts ts_hdr;
> > >
> > > Jason, Michael: creating a new struct for every field is not very
> > > elegant. Is it time to find a more forward looking approach to
> > > expanding with new fields? Like a TLV, or how netlink structs like
> > > tcp_info are extended with support for legacy users that only use
> > > a truncated struct?
> > 
> > I fully agree, we need somebody to work on this.
> 
> Great. I'll take a look when I have some time.
> 
> The current approach infers struct virtio_net_hdr_$VARIANT size from
> negotiated features:
> 
> - virtio_net: virtio_has_feature
> - vhost_net: virtio_features_test_bit
> - tun: tun_vnet_parse_size
> 
> Tuntap however also explicitly negotiates length with TUNSETVNETHDRSZ.
> 
> If we create a struct virtio_net_hdr_ext that can be continuously
> extended going forward, the host and guest will have to negotiate the
> length they both understand, and agree to the min of the two.
> 
> We could use that to implicitly negotiate supported features. E.g.,
> if len is sizeof(virtio_net_hdr_v1_hash_tunnel) then these features
> are supported:
> 
> VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
> VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO
> VIRTIO_NET_F_HASH_REPORT
> etc.
> 
> But that would mean that all features up to the length have to be
> supported, no gaps. I think that is probably not preferable? So then
> they still need to negotiate features, as well as length. Does virtio
> have a mechanism to negotiate not boolean feature enable/disable but
> integers such as struct length?
> 
> Else the change will probably be simpler: keep existing negotiation,
> only change the code to once define a struct and keep adding fields.
> Rather than defining new structs with each feature and updating many
> function arguments and variables to switch to the new struct type.

Right I am not sure why we keep doing this. Let's add a VLA at
the end of the struct and declare fields can be added at will.


> Again, for that see as example struct tcp_info, which is UAPI, but has
> been extended many times.


-- 
MST