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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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 are
> 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.
> Currently, timestamping is handled the same for all supported filters and
> therefore handled the same for all received packets.
> 
> XDP packets are not supported and timestamping is skipped for the XDP path.
> 
> 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 v2:
>   - update filter handling
>   - move tstamp into virtio_net_common_hdr
>   - remove new struct virtio_net_hdr_v1_hash_tunnel_ts
>   Changes to v1:
>   - 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        | 127 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_net.h |   1 +
>  2 files changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..ef6238cb336b9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -416,6 +416,12 @@ struct virtnet_info {
>  	u32 rss_hash_types_supported;
>  	u32 rss_hash_types_saved;
>  
> +	/* Device passes time stamps to the driver */
> +	bool has_tstamp;
> +	bool hwts_rx_en;
> +
> +	struct kernel_hwtstamp_config tstamp_config;
> +
>  	/* Has control virtqueue */
>  	bool has_cvq;
>  
> @@ -499,6 +505,8 @@ struct virtio_net_common_hdr {
>  		struct virtio_net_hdr_v1_hash hash_v1_hdr;
>  		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
>  	};
> +
> +	__le16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
>  };

What is the alignment of the structs in the union and thus the padding
here?

Moving away from introducing a new variant of the struct with every
field is the right approach.

I don't think virtio_net_common_hdr was intended to be extended in
this way. But it might be the simplest approach.