Re: [PATCH v12] virtio-net: support inner header hash

[Parav Pandit <parav@nvidia.com>]

On 4/3/2023 12:58 AM, Heng Qi wrote:

> To achieve this, the device can calculate a suitable hash based on the inner headers
> of this flow, for example using the Toeplitz combined with a symmetric hash key.
I am not sure you need symmetric hash key. Toeplitz with symmetric 
hashing without the symmetric key is possible too.

So just mentioning it as a 'combined with symmetric hashing' is enough.

>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>           u8 rss_max_key_size;
>           le16 rss_max_indirection_table_length;
>           le32 supported_hash_types;
> +        le32 supported_tunnel_hash_types;
>   };
Given that a set command is added via cvq, it make sense to also do 
symetrric work to get it via a cvq.
This is also similar to the latest work for notification coalescing for 
VQ where get and set done using single channel = cvq.

Granted that RSS and other fields are done differently, but it was bit 
in the past.

With that no need to define two fields at two different places in config 
area and also in cvq.

Just the new opcode is needed for GET and things will be fine.

> +If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the device supports inner header hash and
> +the driver can configure the inner header hash calculation for encapsulated packets \ref{sec:Device Types / Network Device / Device OperatiHn / Processing of Incoming Packets / Hash calculation for incoming packets / Tunnel/Encapsulated packet}
> +by issuing the command VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG from the class VIRTIO_NET_CTRL_MQ.
> +The command sets \field{hash_tunnel_types} in the structure virtio_net_hash_tunnel_config.
> +
> +struct virtio_net_hash_tunnel_config {
> +    le32 hash_tunnel_types;
> +};
> +
VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG_SET
and
VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG_GET

> +Filed \field{hash_tunnel_types} contains a bitmask of supported hash tunnel types as
> +defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash tunnel types}.
> +
> +\subparagraph{Tunnel/Encapsulated packet}
> +\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Tunnel/Encapsulated packet}
> +
> +A tunnel packet is encapsulated from the original packet based on the tunneling
> +protocol (only a single level of encapsulation is currently supported). The
> +encapsulated packet contains an outer header and an inner header, and the device
> +calculates the hash over either the inner header or the outer header.
> +
> +If VIRTIO_NET_F_HASH_TUNNEL is negotiated and a received encapsulated packet's
> +outer header matches one of the supported \field{hash_tunnel_types}, the hash
> +of the inner header is calculated.
s/one of the supported/one of the configured/
Because support comes from GET or config space area; out of which a 
smaller or equal subset of tunnel types are configured.

> +\devicenormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> +
> +The device MUST calculate the outer header hash if the received encapsulated packet has an encapsulation type not in \field{supported_tunnel_hash_types}.
> +
Since the configured set can be smaller, a better reword is:
The device MUST calculate the hash from the outer header if the received 
encapsulated packet type is not matching from hash_tunnel_types.

> +The device MUST respond to the VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG command with VIRTIO_NET_ERR if the device
> +received an unrecognized or unsupported VIRTIO_NET_HASH_TUNNEL_TYPE_ flag.
> +
> +Upon reset, the device MUST initialize \field{hash_tunnel_type} to 0.
> +
> +\drivernormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> +
> +The driver MUST have negotiated the feature VIRTIO_NET_F_HASH_TUNNEL when issuing the command VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG.
> +
> +The driver MUST NOT set any VIRTIO_NET_HASH_TUNNEL_TYPE_ flags that are not supported by the device.
> +
>   \paragraph{Hash reporting for incoming packets}
>   \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash reporting for incoming packets}
>   
> @@ -1308,6 +1450,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>    #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET        0 (for automatic receive steering)
>    #define VIRTIO_NET_CTRL_MQ_RSS_CONFIG          1 (for configurable receive steering)
>    #define VIRTIO_NET_CTRL_MQ_HASH_CONFIG         2 (for configurable hash calculation)
> + #define VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG       3 (for configurable inner header hash)
MQ_TUNNEL_CONFIG is not mutually exclusive with HASH or RSS config.
Hence adding it here is incorrect.

See the text below.
"If more than one multiqueue mode is negotiated, the resulting device 
configuration is defined by the last command
sent by the driver."

So MQ_TUNNEL config without RSS or HASH will be an undefined behavior, 
as it doesn't what kind of hashing to be done.

So please tunnel GET and SET commands outside of VIRTIO_NET_CTRL_MQ.
This way it can work side by side with rss, hash and auto modes.

I am sorry for the late response.
I had a day off last week and other active discussions as you notice.