Message-ID: <CACGkMEsP5bv2j5Fx7f3jL=F8oyzA2h_BQZyBZyhwy7DzQzB-SA@mail.gmail.com>
Date: Thu, 5 Feb 2026 10:52:22 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
	Steffen Trumtrar <s.trumtrar@...gutronix.de>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Richard Cochran <richardcochran@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	Eugenio Pérez <eperezma@...hat.com>, 
	virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v2 2/2] virtio-net: support receive timestamp

On Thu, Feb 5, 2026 at 2:44 AM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> 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@...il.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@...gutronix.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@...gle.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?

I think not.

> >
> > 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.
>
>

Yes.

Thanks

> --
> MST
>

Powered by blists - more mailing lists