Re: [PATCH] virtio-net: define feature of per-packet RSS hash delivery

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

On Tue, Dec 03, 2019 at 08:32:12PM +0200, Yuri Benditovich wrote:
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/66
> Conditional extending of virtio header structure to deliver
> packet's hash and hash type used for calculation.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>

Functionally this looks mostly good. But we need to change the
way it's documented slightly.

> ---
>  content.tex | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 01be7df..44975e4 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2811,6 +2811,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> +\item[VIRTIO_NET_F_HASH_REPORT(58)] Device can report per-packet hash value
> +    and a type of calculated hash
> +
>  \item[VIRTIO_NET_F_RSS(60)] Device supports RSS (receive-side scaling)
>      with Toeplitz hash calculation and configurable hash parameters for receive steering
>  
> @@ -2844,6 +2847,8 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
>  \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
>  \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_HASH_REPORT] Requires VIRTIO_NET_F_RSS.
> +
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3071,6 +3076,8 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
>          le16 csum_start;
>          le16 csum_offset;
>          le16 num_buffers;
> +        le32 hash_value; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> +        le16 hash_type; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  };
>  \end{lstlisting}
>  
> @@ -3330,6 +3337,15 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>    set: if so, device has validated the packet checksum.
>    In case of multiple encapsulated protocols, one level of checksums
>    has been validated.
> +\item If VIRTIO_NET_F_HASH_REPORT was negotiated and a device has calculated
> +  a hash for the packet, \field{hash_value} contains calculated hash value and
> +  \field{hash_type} contains exact hash type.
> +
> +  If the hash was not calculated, \field{hash_type} contains zero.
> +
> +  For defined hash types and their meaning, see \ref{sec:Device Types / Network Device / Device configuration layout / RSS}.
> +
> +  For the procedure of hash calculation, see \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS hash types}.
>  \end{enumerate}
>  
>  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
> @@ -3909,6 +3925,14 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item Else skip IPv6 extension headers and calculate the hash as defined above for IPv6 packet without extension headers
>  \end{itemize}
>  
> +If VIRTIO_NET_F_HASH_REPORT was negotiated, the device reports calculated hash information in fields of virtio_net_hdr as follows:
> +
> +Exact hash type is populated in \field{hash_type}
> +
> +Hash value is populated in \field{hash_value}
> +
> +If, due to any reason, the device did not calculate the hash, it sets \field{hash_type} to zero.
> +
>  \drivernormative{\subparagraph}{Setting RSS parameters}{Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) }
>  
>  A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.


This is understandably a minimal change, but imho messes things up:
hash calculation is no longer part of RSS so really should be moved
from out of there.
So we'd have a chapter for hash calculation, defining hash types
etc.

Then both RSS and the new chapter refer to that.

Also, I am guessing supported hash types should be valid
in config space with the new feature - isn't that right?

> -- 
> 2.17.2