[virtio-dev] [PATCH v19] virtio-net: support inner header hash

[virtio-dev] [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

1. Currently, a received encapsulated packet has an outer and an inner header, but
the virtio device is unable to calculate the hash for the inner header. The same
flow can traverse through different tunnels, resulting in the encapsulated
packets being spread across multiple receive queues (refer to the figure below).
However, in certain scenarios, we may need to direct these encapsulated packets of
the same flow to a single receive queue. This facilitates the processing
of the flow by the same CPU to improve performance (warm caches, less locking, etc.).

               client1                    client2
                  |        +-------+         |
                  +------->|tunnels|<--------+
                           +-------+
                              |  |
                              v  v
                      +-----------------+
                      | monitoring host |
                      +-----------------+

To achieve this, the device can calculate a symmetric hash based on the inner headers
of the same flow.

2. For legacy systems, they may lack entropy fields which modern protocols have in
the outer header, resulting in multiple flows with the same outer header but
different inner headers being directed to the same receive queue. This results in
poor receive performance.

To address this limitation, inner header hash can be used to enable the device to advertise
the capability to calculate the hash for the inner packet, regaining better receive performance.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
v18->v19:
	1. Have a single structure instead of two. @Michael S . Tsirkin
	2. Some small rewrites. @Michael S . Tsirkin
	3. Rebase to master.

v17->v18:
	1. Some rewording suggestions from Michael (Thanks!).
	2. Use 0 to disable inner header hash and remove
	   VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
v16->v17:
	1. Some small rewrites. @Parav Pandit
	2. Add Parav's Reviewed-by tag (Thanks!).

v15->v16:
	1. Remove the hash_option. In order to delimit the inner header hash and RSS
	   configuration, the ability to configure the outer src udp port hash is given
	   to RSS. This is orthogonal to inner header hash, which will be done in the
	   RSS capability extension topic (considered as an RSS extension together
	   with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
	2. Fix a 'field' typo. @Parav Pandit

v14->v15:
	1. Add tunnel hash option suggested by @Michael S . Tsirkin
	2. Adjust some descriptions.

v13->v14:
	1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
	2. Rebase to master branch.
	3. Some minor modifications.

v12->v13:
	1. Add a GET command for hash_tunnel_types. @Parav Pandit
	2. Add tunneling protocol explanation. @Jason Wang
	3. Add comments on some usage scenarios for inner hash.

v11->v12:
	1. Add a command VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG.
	2. Refine the commit log. @Michael S . Tsirkin
	3. Add some tunnel types.

v10->v11:
	1. Revise commit log for clarity for readers.
	2. Some modifications to avoid undefined terms. @Parav Pandit
	3. Change VIRTIO_NET_F_HASH_TUNNEL dependency. @Parav Pandit
	4. Add the normative statements. @Parav Pandit

v9->v10:
	1. Removed hash_report_tunnel related information. @Parav Pandit
	2. Re-describe the limitations of QoS for tunneling.
	3. Some clarification.

v8->v9:
	1. Merge hash_report_tunnel_types into hash_report. @Parav Pandit
	2. Add tunnel security section. @Michael S . Tsirkin
	3. Add VIRTIO_NET_F_HASH_REPORT_TUNNEL.
	4. Fix some typos.
	5. Add more tunnel types. @Michael S . Tsirkin

v7->v8:
	1. Add supported_hash_tunnel_types. @Jason Wang, @Parav Pandit
	2. Change hash_report_tunnel to hash_report_tunnel_types. @Parav Pandit
	3. Removed re-definition for inner packet hashing. @Parav Pandit
	4. Fix some typos. @Michael S . Tsirkin
	5. Clarify some sentences. @Michael S . Tsirkin

v6->v7:
	1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
	2. Fix some syntax issues. @Michael S. Tsirkin

v5->v6:
	1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
	2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
	3. Move the links to introduction section. @Michael S. Tsirkin
	4. Clarify some sentences. @Michael S. Tsirkin

v4->v5:
	1. Clarify some paragraphs. @Cornelia Huck
	2. Fix the u8 type. @Cornelia Huck

v3->v4:
	1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
	2. Make things clearer. @Jason Wang @Michael S. Tsirkin
	3. Keep the possibility to use inner hash for automatic receive steering. @Jason Wang
	4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. many times. @Michael S. Tsirkin

v2->v3:
	1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
	2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason Wang, @Michael S. Tsirkin

v1->v2:
	1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
	2. Clarify some paragraphs. @Jason Wang
	3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri Benditovich

 device-types/net/description.tex        | 149 ++++++++++++++++++++++++
 device-types/net/device-conformance.tex |   1 +
 device-types/net/driver-conformance.tex |   1 +
 introduction.tex                        |  39 +++++++
 4 files changed, 190 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 3030222..4a6f5a2 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -88,6 +88,8 @@ \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_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
+
 \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
 
 \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
@@ -147,6 +149,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \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_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
 \end{description}
 
 \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -869,6 +872,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 If the feature VIRTIO_NET_F_RSS was negotiated:
 \begin{itemize}
 \item The device uses \field{hash_types} of the virtio_net_rss_config structure as 'Enabled hash types' bitmask.
+\item If additionally the feature VIRTIO_NET_F_HASH_TUNNEL was negotiated, the device uses \field{enabled_tunnel_types} of the
+      virtnet_hash_tunnel structure as 'Encapsulation types enabled for inner header hash' bitmask.
 \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_rss_config structure (see
 \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / Setting RSS parameters}).
 \end{itemize}
@@ -876,6 +881,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 If the feature VIRTIO_NET_F_RSS was not negotiated:
 \begin{itemize}
 \item The device uses \field{hash_types} of the virtio_net_hash_config structure as 'Enabled hash types' bitmask.
+\item If additionally the feature VIRTIO_NET_F_HASH_TUNNEL was negotiated, the device uses \field{enabled_tunnel_types} of the
+      virtnet_hash_tunnel structure as 'Encapsulation types enabled for inner header hash' bitmask.
 \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_hash_config structure (see
 \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}).
 \end{itemize}
@@ -889,6 +896,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
  \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}.
 \end{itemize}
 
+The per-packet hash calculation can depend on the IP packet type. See
+\hyperref[intro:IP]{[IP]}, \hyperref[intro:UDP]{[UDP]} and \hyperref[intro:TCP]{[TCP]}.
+
 \subparagraph{Supported/enabled hash types}
 \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash types}
 Hash types applicable for IPv4 packets:
@@ -1001,6 +1011,145 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 (see \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / IPv6 packets without extension header}).
 \end{itemize}
 
+\paragraph{Inner Header Hash}
+\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Inner Header Hash}
+
+If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the driver can send commands VIRTIO_NET_CTRL_HASH_TUNNEL_SET
+and VIRTIO_NET_CTRL_HASH_TUNNEL_GET to configure the calculation of the inner header hash.
+
+struct virtnet_hash_tunnel {
+    le32 supported_tunnel_types;
+    le32 enabled_tunnel_types;
+};
+
+#define VIRTIO_NET_CTRL_HASH_TUNNEL 7
+ #define VIRTIO_NET_CTRL_HASH_TUNNEL_SET 0
+ #define VIRTIO_NET_CTRL_HASH_TUNNEL_GET 1
+
+The class VIRTIO_NET_CTRL_HASH_TUNNEL has the following commands:
+\begin{itemize}
+\item VIRTIO_NET_CTRL_HASH_TUNNEL_SET: set \field{enabled_tunnel_types} for the device using the
+      virtnet_hash_tunnel structure, which is read-only for the device.
+      Field \field{supported_tunnel_types} is reserved and it is ignored by the device.
+      Field \field{enabled_tunnel_types} contains the bitmask of encapsulation types to enable for inner header hash.
+
+\item VIRTIO_NET_CTRL_HASH_TUNNEL_GET: get \field{enabled_tunnel_types} and \field{supported_tunnel_types} from
+      the device using the virtnet_hash_tunnel structure, which is write-only for the device.
+      Field \field{supported_tunnel_types} contains the bitmask of encapsulation types supported by the device for inner header hash.
+      Field \field{enabled_tunnel_types} contains the bitmask of encapsulation types enabled in a previous
+      successful call to VIRTIO_NET_CTRL_HASH_TUNNEL_SET.
+\end{itemize}
+
+Encapsulation types are defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets /
+Hash calculation for incoming packets / Encapsulation types supported/enabled for inner header hash}.
+
+\subparagraph{Encapsulated packet}
+\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Encapsulated packet}
+
+Multiple tunneling protocols allow encapsulating an inner, payload packet in an outer, encapsulated packet.
+The encapsulated packet thus 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
+encapsulation types enabled in \field{enabled_tunnel_types}, then the device uses the inner header for hash
+calculations (only a single level of encapsulation is currently supported).
+
+If VIRTIO_NET_F_HASH_TUNNEL is negotiated and a received packet's (outer) header does not match any encapsulation
+types enabled in \field{enabled_tunnel_types}, then the device uses the outer header for hash calculations.
+
+Initially (before the driver sends any VIRTIO_NET_CTRL_HASH_TUNNEL_SET command) all encapsulation types are
+disabled (the value of \field{enabled_tunnel_types} is 0) for inner header hash.
+
+Encapsulation types supported/enabled for inner header hash:
+\begin{itemize}
+    \item The outer header of the following encapsulation types does not contain the transport protocol:
+        \begin{enumerate}
+	    \item \hyperref[intro:ipip]{[IPIP]}: the outer header is over IPv4 and the inner header is over IPv4.
+	    \item \hyperref[intro:nvgre]{[NVGRE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+	    \item \hyperref[intro:gre_rfc2784]{[GRE_rfc2784]}: the outer header is over IPv4 and the inner header is over IPv4.
+	    \item \hyperref[intro:gre_rfc2890]{[GRE_rfc2890]}: the outer header is over IPv4 and the inner header is over IPv4.
+	    \item \hyperref[intro:gre_rfc7676]{[GRE_rfc7676]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+        \end{enumerate}
+
+    \item The outer header of the following encapsulation types uses UDP as the transport protocol:
+        \begin{enumerate}
+	    \item \hyperref[intro:vxlan]{[VXLAN]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+	    \item \hyperref[intro:geneve]{[GENEVE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+	    \item \hyperref[intro:vxlan_gpe]{[VXLAN-GPE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+	    \item \hyperref[intro:gre_in_udp_rfc8086]{[GRE-in-UDP]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
+        \end{enumerate}
+\end{itemize}
+
+\subparagraph{Encapsulation types supported/enabled for inner header hash}
+\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets /
+Hash calculation for incoming packets / Encapsulation types supported/enabled for inner header hash}
+
+Encapsulation types applicable for inner header hash:
+\begin{lstlisting}
+The \hyperref[intro:gre_rfc2784]{[GRE_rfc2784]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_2784    (1 << 0)
+
+The \hyperref[intro:gre_rfc2890]{[GRE_rfc2890]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_2890    (1 << 1)
+
+The \hyperref[intro:gre_rfc7676]{[GRE_rfc7676]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_7676    (1 << 2)
+
+The \hyperref[intro:gre_in_udp_rfc8086]{[GRE-in-UDP]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_UDP     (1 << 3)
+
+The \hyperref[intro:vxlan]{[VXLAN]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_VXLAN       (1 << 4)
+
+The \hyperref[intro:vxlan_gpe]{[VXLAN-GPE]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_VXLAN_GPE   (1 << 5)
+
+The \hyperref[intro:geneve]{[GENEVE]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_GENEVE      (1 << 6)
+
+The \hyperref[intro:ipip]{[IPIP]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_IPIP        (1 << 7)
+
+The \hyperref[intro:nvgre]{[NVGRE]} encapsulation type:
+#define VIRTIO_NET_HASH_TUNNEL_TYPE_NVGRE       (1 << 8)
+\end{lstlisting}
+
+\subparagraph{Advice}
+Example uses of inner header hash:
+\begin{itemize}
+\item Legacy tunneling protocols, lacking outer header entropy, can use RSS with inner header hash to
+      distribute flows with identical outer but different inner headers across various queues, improving performance.
+\item Identify an inner flow distributed across multiple outer tunnels.
+\end{itemize}
+
+As using inner header hash completely discards the outer header entropy, care must be taken
+if the inner header is controlled by an adversary, as the adversary can then intentionally create
+configurations with insufficient entropy.
+
+Besides disabling inner header hash, mitigations would depend on how the hash is used. When the hash
+use is limited to RSS queue selection, inner header hash may have quality of service (QoS) limitations.
+
+\devicenormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
+
+The device MUST ignore \field{supported_tunnel_types} for any received VIRTIO_NET_CTRL_HASH_TUNNEL_SET command.
+
+If the (outer) header of the received packet does not match any encapsulation type enabled
+in \field{enabled_tunnel_types}, the device MUST calculate the hash on the outer header.
+
+If the device receives any bits in \field{enabled_tunnel_types} which are not set in \field{supported_tunnel_types},
+it MUST respond to the VIRTIO_NET_CTRL_HASH_TUNNEL_SET command with VIRTIO_NET_ERR.
+
+If \field{enabled_tunnel_types} is set to 0 or upon device reset, the device MUST disable inner header hash for all encapsulation types.
+
+\drivernormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
+
+The driver MUST have negotiated the VIRTIO_NET_F_HASH_TUNNEL feature when issuing
+commands VIRTIO_NET_CTRL_HASH_TUNNEL_SET and VIRTIO_NET_CTRL_HASH_TUNNEL_GET.
+
+The driver MUST ignore the values received from the VIRTIO_NET_CTRL_HASH_TUNNEL_GET command if the device responds with VIRTIO_NET_ERR.
+
+The driver MUST NOT set any bits in \field{enabled_tunnel_types} which is not set in \field{supported_tunnel_types}.
+
 \paragraph{Hash reporting for incoming packets}
 \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash reporting for incoming packets}
 
diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
index 54f6783..f88f48b 100644
--- a/device-types/net/device-conformance.tex
+++ b/device-types/net/device-conformance.tex
@@ -14,4 +14,5 @@
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \end{itemize}
diff --git a/device-types/net/driver-conformance.tex b/device-types/net/driver-conformance.tex
index 97d0cc1..9d853d9 100644
--- a/device-types/net/driver-conformance.tex
+++ b/device-types/net/driver-conformance.tex
@@ -14,4 +14,5 @@
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Offloads State Configuration / Setting Offloads State}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) }
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
+\item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \end{itemize}
diff --git a/introduction.tex b/introduction.tex
index b7155bf..81f07a4 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -102,6 +102,45 @@ \section{Normative References}\label{sec:Normative References}
     Standards for Efficient Cryptography Group(SECG), ``SEC1: Elliptic Cureve Cryptography'', Version 1.0, September 2000.
 	\newline\url{https://www.secg.org/sec1-v2.pdf}\\
 
+	\phantomsection\label{intro:gre_rfc2784}\textbf{[GRE_rfc2784]} &
+    Generic Routing Encapsulation. This protocol is only specified for IPv4 and used as either the payload or delivery protocol.
+	\newline\url{https://datatracker.ietf.org/doc/rfc2784/}\\
+	\phantomsection\label{intro:gre_rfc2890}\textbf{[GRE_rfc2890]} &
+    Key and Sequence Number Extensions to GRE \ref{intro:gre_rfc2784}. This protocol describes extensions by which two fields, Key and
+    Sequence Number, can be optionally carried in the GRE Header \ref{intro:gre_rfc2784}.
+	\newline\url{https://www.rfc-editor.org/rfc/rfc2890}\\
+	\phantomsection\label{intro:gre_rfc7676}\textbf{[GRE_rfc7676]} &
+    IPv6 Support for Generic Routing Encapsulation (GRE). This protocol is specified for IPv6 and used as either the payload or
+    delivery protocol. Note that this does not change the GRE header format or any behaviors specified by RFC 2784 or RFC 2890.
+	\newline\url{https://datatracker.ietf.org/doc/rfc7676/}\\
+	\phantomsection\label{intro:gre_in_udp_rfc8086}\textbf{[GRE-in-UDP]} &
+    GRE-in-UDP Encapsulation. This specifies a method of encapsulating network protocol packets within GRE and UDP headers.
+    This protocol is specified for IPv4 and IPv6, and used as either the payload or delivery protocol.
+	\newline\url{https://www.rfc-editor.org/rfc/rfc8086}\\
+	\phantomsection\label{intro:vxlan}\textbf{[VXLAN]} &
+    Virtual eXtensible Local Area Network.
+	\newline\url{https://datatracker.ietf.org/doc/rfc7348/}\\
+	\phantomsection\label{intro:vxlan-gpe}\textbf{[VXLAN-GPE]} &
+    Generic Protocol Extension for VXLAN. This protocol describes extending Virtual eXtensible Local Area Network (VXLAN) via changes to the VXLAN header.
+	\newline\url{https://www.ietf.org/archive/id/draft-ietf-nvo3-vxlan-gpe-12.txt}\\
+	\phantomsection\label{intro:geneve}\textbf{[GENEVE]} &
+    Generic Network Virtualization Encapsulation.
+	\newline\url{https://datatracker.ietf.org/doc/rfc8926/}\\
+	\phantomsection\label{intro:ipip}\textbf{[IPIP]} &
+    IP Encapsulation within IP.
+	\newline\url{https://www.rfc-editor.org/rfc/rfc2003}\\
+	\phantomsection\label{intro:nvgre}\textbf{[NVGRE]} &
+    NVGRE: Network Virtualization Using Generic Routing Encapsulation
+	\newline\url{https://www.rfc-editor.org/rfc/rfc7637.html}\\
+	\phantomsection\label{intro:IP}\textbf{[IP]} &
+    INTERNET PROTOCOL
+	\newline\url{https://www.rfc-editor.org/rfc/rfc791}\\
+	\phantomsection\label{intro:UDP}\textbf{[UDP]} &
+    User Datagram Protocol
+	\newline\url{https://www.rfc-editor.org/rfc/rfc768}\\
+	\phantomsection\label{intro:TCP}\textbf{[TCP]} &
+    TRANSMISSION CONTROL PROTOCOL
+	\newline\url{https://www.rfc-editor.org/rfc/rfc793}\\
 \end{longtable}
 
 \section{Non-Normative References}
-- 
2.19.1.6.gb485710b


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jason Wang]

On Wed, Jun 28, 2023 at 12:35 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> 1. Currently, a received encapsulated packet has an outer and an inner header, but
> the virtio device is unable to calculate the hash for the inner header. The same
> flow can traverse through different tunnels, resulting in the encapsulated
> packets being spread across multiple receive queues (refer to the figure below).
> However, in certain scenarios, we may need to direct these encapsulated packets of
> the same flow to a single receive queue. This facilitates the processing
> of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
>
>                client1                    client2
>                   |        +-------+         |
>                   +------->|tunnels|<--------+
>                            +-------+
>                               |  |
>                               v  v
>                       +-----------------+
>                       | monitoring host |
>                       +-----------------+
>
> To achieve this, the device can calculate a symmetric hash based on the inner headers
> of the same flow.
>
> 2. For legacy systems, they may lack entropy fields which modern protocols have in
> the outer header, resulting in multiple flows with the same outer header but
> different inner headers being directed to the same receive queue. This results in
> poor receive performance.
>
> To address this limitation, inner header hash can be used to enable the device to advertise
> the capability to calculate the hash for the inner packet, regaining better receive performance.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---
> v18->v19:
>         1. Have a single structure instead of two. @Michael S . Tsirkin
>         2. Some small rewrites. @Michael S . Tsirkin
>         3. Rebase to master.
>
> v17->v18:
>         1. Some rewording suggestions from Michael (Thanks!).
>         2. Use 0 to disable inner header hash and remove
>            VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> v16->v17:
>         1. Some small rewrites. @Parav Pandit
>         2. Add Parav's Reviewed-by tag (Thanks!).
>
> v15->v16:
>         1. Remove the hash_option. In order to delimit the inner header hash and RSS
>            configuration, the ability to configure the outer src udp port hash is given
>            to RSS. This is orthogonal to inner header hash, which will be done in the
>            RSS capability extension topic (considered as an RSS extension together
>            with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
>         2. Fix a 'field' typo. @Parav Pandit
>
> v14->v15:
>         1. Add tunnel hash option suggested by @Michael S . Tsirkin
>         2. Adjust some descriptions.
>
> v13->v14:
>         1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit

I may miss some discussions, but this complicates the provisioning a lot.

Having it in the config space, then a type agnostic provisioning
through config space + feature bits just works fine.

If we move it only via cvq, we need device specific provisioning interface.

Thanks


>         2. Rebase to master branch.
>         3. Some minor modifications.
>
> v12->v13:
>         1. Add a GET command for hash_tunnel_types. @Parav Pandit
>         2. Add tunneling protocol explanation. @Jason Wang
>         3. Add comments on some usage scenarios for inner hash.
>
> v11->v12:
>         1. Add a command VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG.
>         2. Refine the commit log. @Michael S . Tsirkin
>         3. Add some tunnel types.
>
> v10->v11:
>         1. Revise commit log for clarity for readers.
>         2. Some modifications to avoid undefined terms. @Parav Pandit
>         3. Change VIRTIO_NET_F_HASH_TUNNEL dependency. @Parav Pandit
>         4. Add the normative statements. @Parav Pandit
>
> v9->v10:
>         1. Removed hash_report_tunnel related information. @Parav Pandit
>         2. Re-describe the limitations of QoS for tunneling.
>         3. Some clarification.
>
> v8->v9:
>         1. Merge hash_report_tunnel_types into hash_report. @Parav Pandit
>         2. Add tunnel security section. @Michael S . Tsirkin
>         3. Add VIRTIO_NET_F_HASH_REPORT_TUNNEL.
>         4. Fix some typos.
>         5. Add more tunnel types. @Michael S . Tsirkin
>
> v7->v8:
>         1. Add supported_hash_tunnel_types. @Jason Wang, @Parav Pandit
>         2. Change hash_report_tunnel to hash_report_tunnel_types. @Parav Pandit
>         3. Removed re-definition for inner packet hashing. @Parav Pandit
>         4. Fix some typos. @Michael S . Tsirkin
>         5. Clarify some sentences. @Michael S . Tsirkin
>
> v6->v7:
>         1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
>         2. Fix some syntax issues. @Michael S. Tsirkin
>
> v5->v6:
>         1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
>         2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
>         3. Move the links to introduction section. @Michael S. Tsirkin
>         4. Clarify some sentences. @Michael S. Tsirkin
>
> v4->v5:
>         1. Clarify some paragraphs. @Cornelia Huck
>         2. Fix the u8 type. @Cornelia Huck
>
> v3->v4:
>         1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
>         2. Make things clearer. @Jason Wang @Michael S. Tsirkin
>         3. Keep the possibility to use inner hash for automatic receive steering. @Jason Wang
>         4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. many times. @Michael S. Tsirkin
>
> v2->v3:
>         1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
>         2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason Wang, @Michael S. Tsirkin
>
> v1->v2:
>         1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
>         2. Clarify some paragraphs. @Jason Wang
>         3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri Benditovich
>
>  device-types/net/description.tex        | 149 ++++++++++++++++++++++++
>  device-types/net/device-conformance.tex |   1 +
>  device-types/net/driver-conformance.tex |   1 +
>  introduction.tex                        |  39 +++++++
>  4 files changed, 190 insertions(+)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 3030222..4a6f5a2 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,6 +88,8 @@ \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_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> +
>  \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
>
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> @@ -147,6 +149,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \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_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>  \end{description}
>
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -869,6 +872,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  If the feature VIRTIO_NET_F_RSS was negotiated:
>  \begin{itemize}
>  \item The device uses \field{hash_types} of the virtio_net_rss_config structure as 'Enabled hash types' bitmask.
> +\item If additionally the feature VIRTIO_NET_F_HASH_TUNNEL was negotiated, the device uses \field{enabled_tunnel_types} of the
> +      virtnet_hash_tunnel structure as 'Encapsulation types enabled for inner header hash' bitmask.
>  \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_rss_config structure (see
>  \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / Setting RSS parameters}).
>  \end{itemize}
> @@ -876,6 +881,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  If the feature VIRTIO_NET_F_RSS was not negotiated:
>  \begin{itemize}
>  \item The device uses \field{hash_types} of the virtio_net_hash_config structure as 'Enabled hash types' bitmask.
> +\item If additionally the feature VIRTIO_NET_F_HASH_TUNNEL was negotiated, the device uses \field{enabled_tunnel_types} of the
> +      virtnet_hash_tunnel structure as 'Encapsulation types enabled for inner header hash' bitmask.
>  \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_hash_config structure (see
>  \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}).
>  \end{itemize}
> @@ -889,6 +896,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}.
>  \end{itemize}
>
> +The per-packet hash calculation can depend on the IP packet type. See
> +\hyperref[intro:IP]{[IP]}, \hyperref[intro:UDP]{[UDP]} and \hyperref[intro:TCP]{[TCP]}.
> +
>  \subparagraph{Supported/enabled hash types}
>  \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash types}
>  Hash types applicable for IPv4 packets:
> @@ -1001,6 +1011,145 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  (see \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / IPv6 packets without extension header}).
>  \end{itemize}
>
> +\paragraph{Inner Header Hash}
> +\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Inner Header Hash}
> +
> +If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the driver can send commands VIRTIO_NET_CTRL_HASH_TUNNEL_SET
> +and VIRTIO_NET_CTRL_HASH_TUNNEL_GET to configure the calculation of the inner header hash.
> +
> +struct virtnet_hash_tunnel {
> +    le32 supported_tunnel_types;
> +    le32 enabled_tunnel_types;
> +};
> +
> +#define VIRTIO_NET_CTRL_HASH_TUNNEL 7
> + #define VIRTIO_NET_CTRL_HASH_TUNNEL_SET 0
> + #define VIRTIO_NET_CTRL_HASH_TUNNEL_GET 1
> +
> +The class VIRTIO_NET_CTRL_HASH_TUNNEL has the following commands:
> +\begin{itemize}
> +\item VIRTIO_NET_CTRL_HASH_TUNNEL_SET: set \field{enabled_tunnel_types} for the device using the
> +      virtnet_hash_tunnel structure, which is read-only for the device.
> +      Field \field{supported_tunnel_types} is reserved and it is ignored by the device.
> +      Field \field{enabled_tunnel_types} contains the bitmask of encapsulation types to enable for inner header hash.
> +
> +\item VIRTIO_NET_CTRL_HASH_TUNNEL_GET: get \field{enabled_tunnel_types} and \field{supported_tunnel_types} from
> +      the device using the virtnet_hash_tunnel structure, which is write-only for the device.
> +      Field \field{supported_tunnel_types} contains the bitmask of encapsulation types supported by the device for inner header hash.
> +      Field \field{enabled_tunnel_types} contains the bitmask of encapsulation types enabled in a previous
> +      successful call to VIRTIO_NET_CTRL_HASH_TUNNEL_SET.
> +\end{itemize}
> +
> +Encapsulation types are defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets /
> +Hash calculation for incoming packets / Encapsulation types supported/enabled for inner header hash}.
> +
> +\subparagraph{Encapsulated packet}
> +\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Encapsulated packet}
> +
> +Multiple tunneling protocols allow encapsulating an inner, payload packet in an outer, encapsulated packet.
> +The encapsulated packet thus 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
> +encapsulation types enabled in \field{enabled_tunnel_types}, then the device uses the inner header for hash
> +calculations (only a single level of encapsulation is currently supported).
> +
> +If VIRTIO_NET_F_HASH_TUNNEL is negotiated and a received packet's (outer) header does not match any encapsulation
> +types enabled in \field{enabled_tunnel_types}, then the device uses the outer header for hash calculations.
> +
> +Initially (before the driver sends any VIRTIO_NET_CTRL_HASH_TUNNEL_SET command) all encapsulation types are
> +disabled (the value of \field{enabled_tunnel_types} is 0) for inner header hash.
> +
> +Encapsulation types supported/enabled for inner header hash:
> +\begin{itemize}
> +    \item The outer header of the following encapsulation types does not contain the transport protocol:
> +        \begin{enumerate}
> +           \item \hyperref[intro:ipip]{[IPIP]}: the outer header is over IPv4 and the inner header is over IPv4.
> +           \item \hyperref[intro:nvgre]{[NVGRE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +           \item \hyperref[intro:gre_rfc2784]{[GRE_rfc2784]}: the outer header is over IPv4 and the inner header is over IPv4.
> +           \item \hyperref[intro:gre_rfc2890]{[GRE_rfc2890]}: the outer header is over IPv4 and the inner header is over IPv4.
> +           \item \hyperref[intro:gre_rfc7676]{[GRE_rfc7676]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +        \end{enumerate}
> +
> +    \item The outer header of the following encapsulation types uses UDP as the transport protocol:
> +        \begin{enumerate}
> +           \item \hyperref[intro:vxlan]{[VXLAN]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +           \item \hyperref[intro:geneve]{[GENEVE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +           \item \hyperref[intro:vxlan_gpe]{[VXLAN-GPE]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +           \item \hyperref[intro:gre_in_udp_rfc8086]{[GRE-in-UDP]}: the outer header is over IPv4/IPv6 and the inner header is over IPv4/IPv6.
> +        \end{enumerate}
> +\end{itemize}
> +
> +\subparagraph{Encapsulation types supported/enabled for inner header hash}
> +\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets /
> +Hash calculation for incoming packets / Encapsulation types supported/enabled for inner header hash}
> +
> +Encapsulation types applicable for inner header hash:
> +\begin{lstlisting}
> +The \hyperref[intro:gre_rfc2784]{[GRE_rfc2784]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_2784    (1 << 0)
> +
> +The \hyperref[intro:gre_rfc2890]{[GRE_rfc2890]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_2890    (1 << 1)
> +
> +The \hyperref[intro:gre_rfc7676]{[GRE_rfc7676]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_7676    (1 << 2)
> +
> +The \hyperref[intro:gre_in_udp_rfc8086]{[GRE-in-UDP]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE_UDP     (1 << 3)
> +
> +The \hyperref[intro:vxlan]{[VXLAN]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_VXLAN       (1 << 4)
> +
> +The \hyperref[intro:vxlan_gpe]{[VXLAN-GPE]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_VXLAN_GPE   (1 << 5)
> +
> +The \hyperref[intro:geneve]{[GENEVE]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GENEVE      (1 << 6)
> +
> +The \hyperref[intro:ipip]{[IPIP]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_IPIP        (1 << 7)
> +
> +The \hyperref[intro:nvgre]{[NVGRE]} encapsulation type:
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_NVGRE       (1 << 8)
> +\end{lstlisting}
> +
> +\subparagraph{Advice}
> +Example uses of inner header hash:
> +\begin{itemize}
> +\item Legacy tunneling protocols, lacking outer header entropy, can use RSS with inner header hash to
> +      distribute flows with identical outer but different inner headers across various queues, improving performance.
> +\item Identify an inner flow distributed across multiple outer tunnels.
> +\end{itemize}
> +
> +As using inner header hash completely discards the outer header entropy, care must be taken
> +if the inner header is controlled by an adversary, as the adversary can then intentionally create
> +configurations with insufficient entropy.
> +
> +Besides disabling inner header hash, mitigations would depend on how the hash is used. When the hash
> +use is limited to RSS queue selection, inner header hash may have quality of service (QoS) limitations.
> +
> +\devicenormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> +
> +The device MUST ignore \field{supported_tunnel_types} for any received VIRTIO_NET_CTRL_HASH_TUNNEL_SET command.
> +
> +If the (outer) header of the received packet does not match any encapsulation type enabled
> +in \field{enabled_tunnel_types}, the device MUST calculate the hash on the outer header.
> +
> +If the device receives any bits in \field{enabled_tunnel_types} which are not set in \field{supported_tunnel_types},
> +it MUST respond to the VIRTIO_NET_CTRL_HASH_TUNNEL_SET command with VIRTIO_NET_ERR.
> +
> +If \field{enabled_tunnel_types} is set to 0 or upon device reset, the device MUST disable inner header hash for all encapsulation types.
> +
> +\drivernormative{\subparagraph}{Inner Header Hash}{Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> +
> +The driver MUST have negotiated the VIRTIO_NET_F_HASH_TUNNEL feature when issuing
> +commands VIRTIO_NET_CTRL_HASH_TUNNEL_SET and VIRTIO_NET_CTRL_HASH_TUNNEL_GET.
> +
> +The driver MUST ignore the values received from the VIRTIO_NET_CTRL_HASH_TUNNEL_GET command if the device responds with VIRTIO_NET_ERR.
> +
> +The driver MUST NOT set any bits in \field{enabled_tunnel_types} which is not set in \field{supported_tunnel_types}.
> +
>  \paragraph{Hash reporting for incoming packets}
>  \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash reporting for incoming packets}
>
> diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
> index 54f6783..f88f48b 100644
> --- a/device-types/net/device-conformance.tex
> +++ b/device-types/net/device-conformance.tex
> @@ -14,4 +14,5 @@
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
>  \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
> +\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
>  \end{itemize}
> diff --git a/device-types/net/driver-conformance.tex b/device-types/net/driver-conformance.tex
> index 97d0cc1..9d853d9 100644
> --- a/device-types/net/driver-conformance.tex
> +++ b/device-types/net/driver-conformance.tex
> @@ -14,4 +14,5 @@
>  \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Offloads State Configuration / Setting Offloads State}
>  \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) }
>  \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
> +\item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
>  \end{itemize}
> diff --git a/introduction.tex b/introduction.tex
> index b7155bf..81f07a4 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -102,6 +102,45 @@ \section{Normative References}\label{sec:Normative References}
>      Standards for Efficient Cryptography Group(SECG), ``SEC1: Elliptic Cureve Cryptography'', Version 1.0, September 2000.
>         \newline\url{https://www.secg.org/sec1-v2.pdf}\\
>
> +       \phantomsection\label{intro:gre_rfc2784}\textbf{[GRE_rfc2784]} &
> +    Generic Routing Encapsulation. This protocol is only specified for IPv4 and used as either the payload or delivery protocol.
> +       \newline\url{https://datatracker.ietf.org/doc/rfc2784/}\\
> +       \phantomsection\label{intro:gre_rfc2890}\textbf{[GRE_rfc2890]} &
> +    Key and Sequence Number Extensions to GRE \ref{intro:gre_rfc2784}. This protocol describes extensions by which two fields, Key and
> +    Sequence Number, can be optionally carried in the GRE Header \ref{intro:gre_rfc2784}.
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc2890}\\
> +       \phantomsection\label{intro:gre_rfc7676}\textbf{[GRE_rfc7676]} &
> +    IPv6 Support for Generic Routing Encapsulation (GRE). This protocol is specified for IPv6 and used as either the payload or
> +    delivery protocol. Note that this does not change the GRE header format or any behaviors specified by RFC 2784 or RFC 2890.
> +       \newline\url{https://datatracker.ietf.org/doc/rfc7676/}\\
> +       \phantomsection\label{intro:gre_in_udp_rfc8086}\textbf{[GRE-in-UDP]} &
> +    GRE-in-UDP Encapsulation. This specifies a method of encapsulating network protocol packets within GRE and UDP headers.
> +    This protocol is specified for IPv4 and IPv6, and used as either the payload or delivery protocol.
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc8086}\\
> +       \phantomsection\label{intro:vxlan}\textbf{[VXLAN]} &
> +    Virtual eXtensible Local Area Network.
> +       \newline\url{https://datatracker.ietf.org/doc/rfc7348/}\\
> +       \phantomsection\label{intro:vxlan-gpe}\textbf{[VXLAN-GPE]} &
> +    Generic Protocol Extension for VXLAN. This protocol describes extending Virtual eXtensible Local Area Network (VXLAN) via changes to the VXLAN header.
> +       \newline\url{https://www.ietf.org/archive/id/draft-ietf-nvo3-vxlan-gpe-12.txt}\\
> +       \phantomsection\label{intro:geneve}\textbf{[GENEVE]} &
> +    Generic Network Virtualization Encapsulation.
> +       \newline\url{https://datatracker.ietf.org/doc/rfc8926/}\\
> +       \phantomsection\label{intro:ipip}\textbf{[IPIP]} &
> +    IP Encapsulation within IP.
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc2003}\\
> +       \phantomsection\label{intro:nvgre}\textbf{[NVGRE]} &
> +    NVGRE: Network Virtualization Using Generic Routing Encapsulation
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc7637.html}\\
> +       \phantomsection\label{intro:IP}\textbf{[IP]} &
> +    INTERNET PROTOCOL
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc791}\\
> +       \phantomsection\label{intro:UDP}\textbf{[UDP]} &
> +    User Datagram Protocol
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc768}\\
> +       \phantomsection\label{intro:TCP}\textbf{[TCP]} &
> +    TRANSMISSION CONTROL PROTOCOL
> +       \newline\url{https://www.rfc-editor.org/rfc/rfc793}\\
>  \end{longtable}
>
>  \section{Non-Normative References}
> --
> 2.19.1.6.gb485710b
>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, June 27, 2023 11:46 PM
> 
> Having it in the config space, then a type agnostic provisioning through config
> space + feature bits just works fine.
>
Provisioning is far simpler thing to do in device specific way than asking device to store this value in onchip area which is rarely accessed.
There are many fields of many features for 1.4 are discussed including this one. All of these capabilities just cannot be stored in config space.
 
> If we move it only via cvq, we need device specific provisioning interface.

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

[Jason Wang]

On Wed, Jun 28, 2023 at 12:23 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, June 27, 2023 11:46 PM
> >
> > Having it in the config space, then a type agnostic provisioning through config
> > space + feature bits just works fine.
> >
> Provisioning is far simpler thing to do in device specific way than asking device to store this value in onchip area which is rarely accessed.

Are you suggesting to not place any new fields in the config space?

struct virtio_net_config {
        u8 mac[6];
        le16 status;
        le16 max_virtqueue_pairs;
        le16 mtu;
        le32 speed;
        u8 duplex;
        u8 rss_max_key_size;
        le16 rss_max_indirection_table_length;
        le32 supported_hash_types;
};

Which of the above do you think can be accessed frequently and which
part of the spec says it must be stored in the onchip area?

Thanks

> There are many fields of many features for 1.4 are discussed including this one. All of these capabilities just cannot be stored in config space.
>
> > If we move it only via cvq, we need device specific provisioning interface.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 28, 2023 1:38 AM

[..]
> > Provisioning is far simpler thing to do in device specific way than asking device
> to store this value in onchip area which is rarely accessed.
> 
> Are you suggesting to not place any new fields in the config space?
> 
Yes.

> struct virtio_net_config {
>         u8 mac[6];
>         le16 status;
>         le16 max_virtqueue_pairs;
>         le16 mtu;
>         le32 speed;
>         u8 duplex;
>         u8 rss_max_key_size;
>         le16 rss_max_indirection_table_length;
>         le32 supported_hash_types;
> };
> 
> Which of the above do you think can be accessed frequently and which part of
> the spec says it must be stored in the onchip area?
> 
Most are not accessed frequently.
The fact that they are in MMIO a device needs to place in a memory with tight latency budget.
Spec is not going to talk on onchip area, it is the reflection of spec that forces certain inefficient implementation .

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

[Jason Wang]

On Thu, Jun 29, 2023 at 12:00 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 28, 2023 1:38 AM
>
> [..]
> > > Provisioning is far simpler thing to do in device specific way than asking device
> > to store this value in onchip area which is rarely accessed.
> >
> > Are you suggesting to not place any new fields in the config space?
> >
> Yes.

It's not the fault of config space itself, but the way to access the
config space, more below.

>
> > struct virtio_net_config {
> >         u8 mac[6];
> >         le16 status;
> >         le16 max_virtqueue_pairs;
> >         le16 mtu;
> >         le32 speed;
> >         u8 duplex;
> >         u8 rss_max_key_size;
> >         le16 rss_max_indirection_table_length;
> >         le32 supported_hash_types;
> > };
> >
> > Which of the above do you think can be accessed frequently and which part of
> > the spec says it must be stored in the onchip area?
> >
> Most are not accessed frequently.
> The fact that they are in MMIO a device needs to place in a memory with tight latency budget.

This really depends on the implementation and vendor architectures.
For example,

1) MSI BAR might require much more resources than a simple device
configuration space
2) I was told my some vendors that the virtqueue is much more valuable than MMIO
3) We can introduce new ways to access the device configuration space

> Spec is not going to talk on onchip area, it is the reflection of spec that forces certain inefficient implementation .

Exactly, it's implementation specific, so config space is fine, we
just need to invent new methods to access them that fit for your
hardware.

Thanks


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] Re: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Jason Wang
> Sent: Wednesday, June 28, 2023 11:18 PM
> 
> It's not the fault of config space itself, but the way to access the config space,
> more below.
>
Sure and it is linked to old and new entries into it. I explained in this thread with discussion with Michael.
Please refer it.
 
> >
> > > struct virtio_net_config {
> > >         u8 mac[6];
> > >         le16 status;
> > >         le16 max_virtqueue_pairs;
> > >         le16 mtu;
> > >         le32 speed;
> > >         u8 duplex;
> > >         u8 rss_max_key_size;
> > >         le16 rss_max_indirection_table_length;
> > >         le32 supported_hash_types;
> > > };
> > >
> > > Which of the above do you think can be accessed frequently and which
> > > part of the spec says it must be stored in the onchip area?
> > >
> > Most are not accessed frequently.
> > The fact that they are in MMIO a device needs to place in a memory with tight
> latency budget.
> 
> This really depends on the implementation and vendor architectures.
> For example,
> 
> 1) MSI BAR might require much more resources than a simple device
> configuration space
Sure, that is also getting solved in parallel outside of virtio spec.
So MSI BAR saving is not replacement, it is different area that is getting optimized.

> 2) I was told my some vendors that the virtqueue is much more valuable than
> MMIO
Not sure I understand "valuable".

> 3) We can introduce new ways to access the device configuration space
>
Yes, new way for existing config space and extended (new fields) of config space.
I explained the reason to differentiate this few times in thread to Michael, so I will omit repeating here.
 
> > Spec is not going to talk on onchip area, it is the reflection of spec that forces
> certain inefficient implementation .
> 
> Exactly, it's implementation specific, so config space is fine, we just need to
> invent new methods to access them that fit for your hardware.

Yes. Lets work on it in the new thread.

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 04:23:09AM +0000, Parav Pandit wrote:
> 
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, June 27, 2023 11:46 PM
> > 
> > Having it in the config space, then a type agnostic provisioning through config
> > space + feature bits just works fine.
> >
> Provisioning is far simpler thing to do in device specific way than asking device to store this value in onchip area which is rarely accessed.

I thought a lot about this over the weekend. I just do not see this
working out, sorry.  Two things is never easier than one. We already
need config space with provisioning.  In fact e.g. on Linux we want
drivers to keep doing virtio_cread() and all the DMA tricks should be
hidden behind the scenes. You don't like it that config space is
on-chip? Build an interface for it not to be on chip. There is simply
no downside to that.

I came back after a small break and I still see this discussion
that keeps playing out as a big distraction.
In particular where are you doing to store the things that are for DMA?
In system RAM? We don't *have* anywhere in system RAM to store this,
we will need an interface to allocate system RAM and pass it to device
for this idea to work. So in 1.3 where we won't have this, there is
much less of an excuse to use a vq.




> There are many fields of many features for 1.4 are discussed including this one. All of these capabilities just cannot be stored in config space.

config space is synchronous interface, vq is asynchronous.  Setup
is just too messy to do asynchronously. So yes, config space.


Please let people just focus on fixing config space instead of
temporary cvq hacks.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 6:27 AM
> 
> On Wed, Jun 28, 2023 at 04:23:09AM +0000, Parav Pandit wrote:
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, June 27, 2023 11:46 PM
> > >
> > > Having it in the config space, then a type agnostic provisioning
> > > through config space + feature bits just works fine.
> > >
> > Provisioning is far simpler thing to do in device specific way than asking device
> to store this value in onchip area which is rarely accessed.
> 
> I thought a lot about this over the weekend. I just do not see this working out,
> sorry.  Two things is never easier than one. 

You proposed two things and making it mandatory.
I am glad that you realized to do using one interface.

In different context, you proposed two interface for driver notification on PF and/or VF BAR in separate thread for flexibility. I asked for one.
Somehow you promoted flexibility (and also complexity that no vendor wanted), and here you lean towards single interface...

So more explanation of the rationale will help to understand.

I didn't propose two interfaces. Let me explain again.

I proposed, 
a. all new fields to go via vq (single interface)
b. all existing fields to stay in cfg space (single interface)
c. if one wants to optimize optionally existing fields can utilize (same flexibility how you asked for PF and VF notification).


> We already need config space with provisioning.  
Provisioning does provision features, config space and control parameters, msix vectors and more.
Provisioning does _not_ need config space.

> In fact e.g. on Linux we want drivers to keep doing virtio_cread()
> and all the DMA tricks should be hidden behind the scenes. You don't like it that
> config space is on-chip? Build an interface for it not to be on chip. There is
> simply no downside to that.
> 
This is the ask to build software interface between driver and device to not demand on-chip config space more importantly _predictably_.

> I came back after a small break and I still see this discussion that keeps playing
> out as a big distraction.
> In particular where are you doing to store the things that are for DMA?
> In system RAM? We don't *have* anywhere in system RAM to store this, we will
> need an interface to allocate system RAM and pass it to device for this idea to
> work. So in 1.3 where we won't have this, there is much less of an excuse to use
> a vq.
To store what?
CVQ is already there to do GET and SET operation.

> 
> 
> 
> 
> > There are many fields of many features for 1.4 are discussed including this
> one. All of these capabilities just cannot be stored in config space.
> 
> config space is synchronous interface, vq is asynchronous.  Setup is just too
> messy to do asynchronously. So yes, config space.
> 
Huh, I disagree. The whole virtio spec has got the VQs all over and you say that VQ is messy.
Doesn't make any sense at all.

> 
> Please let people just focus on fixing config space instead of temporary cvq
> hacks.

The ask is to have predictable sizing for existing defined config space field and not keep infinitely growing by two interfaces.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 04:18:02PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 6:27 AM
> > 
> > On Wed, Jun 28, 2023 at 04:23:09AM +0000, Parav Pandit wrote:
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, June 27, 2023 11:46 PM
> > > >
> > > > Having it in the config space, then a type agnostic provisioning
> > > > through config space + feature bits just works fine.
> > > >
> > > Provisioning is far simpler thing to do in device specific way than asking device
> > to store this value in onchip area which is rarely accessed.
> > 
> > I thought a lot about this over the weekend. I just do not see this working out,
> > sorry.  Two things is never easier than one. 
> 
> You proposed two things and making it mandatory.
> I am glad that you realized to do using one interface.
> 
> In different context, you proposed two interface for driver notification on PF and/or VF BAR in separate thread for flexibility. I asked for one.
> Somehow you promoted flexibility (and also complexity that no vendor wanted), and here you lean towards single interface...
> 
> So more explanation of the rationale will help to understand.

OK I will try.

Actually, my preblem is exactly if there's no one single
interface that can do everything.


Two interfaces that do the same might seem redundant but
there could be good reasons for this, in this case legacy.

See the difference?

> I didn't propose two interfaces. Let me explain again.
> 
> I proposed, 
> a. all new fields to go via vq (single interface)
> b. all existing fields to stay in cfg space (single interface)
> c. if one wants to optimize optionally existing fields can utilize (same flexibility how you asked for PF and VF notification).
> 
> 
> > We already need config space with provisioning.  
> Provisioning does provision features, config space and control parameters, msix vectors and more.
> Provisioning does _not_ need config space.

I mean it needs to provision it.

> > In fact e.g. on Linux we want drivers to keep doing virtio_cread()
> > and all the DMA tricks should be hidden behind the scenes. You don't like it that
> > config space is on-chip? Build an interface for it not to be on chip. There is
> > simply no downside to that.
> > 
> This is the ask to build software interface between driver and device to not demand on-chip config space more importantly _predictably_.

Absolutely. So we would say that driver SHOULD use the DMA interface
if it is present.

> > I came back after a small break and I still see this discussion that keeps playing
> > out as a big distraction.
> > In particular where are you doing to store the things that are for DMA?
> > In system RAM? We don't *have* anywhere in system RAM to store this, we will
> > need an interface to allocate system RAM and pass it to device for this idea to
> > work. So in 1.3 where we won't have this, there is much less of an excuse to use
> > a vq.
> To store what?
> CVQ is already there to do GET and SET operation.

To store whatever value CVQ is requesting. Where is it going to
come from?


> > 
> > 
> > 
> > 
> > > There are many fields of many features for 1.4 are discussed including this
> > one. All of these capabilities just cannot be stored in config space.
> > 
> > config space is synchronous interface, vq is asynchronous.  Setup is just too
> > messy to do asynchronously. So yes, config space.
> > 
> Huh, I disagree. The whole virtio spec has got the VQs all over and you say that VQ is messy.
> Doesn't make any sense at all.

Asynchronous is just harder for software.  Look at the hoops virtio net
has to jump through to do control path over cvq and weep. Compare to
single liner virtio_cread.

I happen to maintain Linux drivers too, so I care.

> > 
> > Please let people just focus on fixing config space instead of temporary cvq
> > hacks.
> 
> The ask is to have predictable sizing for existing defined config space field and not keep infinitely growing by two interfaces.

I don't know what "predictable sizing" is or why does it matter.

I get the value of reduced MMIO register map.
I get that the only way to get that appears to be to pass values
around using DMA.

I don't think we need to throw away config space - just use DMA to access it.

And "existing defined" here seems to be at a completely random point in time.
If someone wants to have fields in MMIO, I see no reason not to.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 12:46 PM

> Actually, my preblem is exactly if there's no one single interface that can do
> everything.
>
Extended config space reading via single DMA interface is just enough.

> 
> Two interfaces that do the same might seem redundant but there could be good
> reasons for this, in this case legacy.
>
But legacy should not force it to newer interface. More below.
 
> See the difference?
> 

> > I didn't propose two interfaces. Let me explain again.
> >
> > I proposed,
> > a. all new fields to go via vq (single interface) b. all existing
> > fields to stay in cfg space (single interface) c. if one wants to
> > optimize optionally existing fields can utilize (same flexibility how you asked
> for PF and VF notification).
> >
> >
> > > We already need config space with provisioning.
> > Provisioning does provision features, config space and control parameters,
> msix vectors and more.
> > Provisioning does _not_ need config space.
> 
> I mean it needs to provision it.
> 
No matter there is config space of dma, a device to be provisioned.

> > > In fact e.g. on Linux we want drivers to keep doing virtio_cread()
> > > and all the DMA tricks should be hidden behind the scenes. You don't
> > > like it that config space is on-chip? Build an interface for it not
> > > to be on chip. There is simply no downside to that.
> > >
> > This is the ask to build software interface between driver and device to not
> demand on-chip config space more importantly _predictably_.
> 
> Absolutely. So we would say that driver SHOULD use the DMA interface if it is
> present.
> 
I am asking for driver MUST use the DMA interface for _extended_ config space.
Why?
For two reasons.

1. Because than device is 100% sure that it does not fulfill MMIO needs
2. Single interface to access extended config space, which is what you asked for.
3. Single driver code because there is single way to get it.


> > > I came back after a small break and I still see this discussion that
> > > keeps playing out as a big distraction.
> > > In particular where are you doing to store the things that are for DMA?
> > > In system RAM? We don't *have* anywhere in system RAM to store this,
> > > we will need an interface to allocate system RAM and pass it to
> > > device for this idea to work. So in 1.3 where we won't have this,
> > > there is much less of an excuse to use a vq.
> > To store what?
> > CVQ is already there to do GET and SET operation.
> 
> To store whatever value CVQ is requesting. Where is it going to come from?
>
It is going to come from the device memory that does not have hard requirements of MMIO accessibility.
For example slow flash memory.
 
> Asynchronous is just harder for software.  Look at the hoops virtio net has to
> jump through to do control path over cvq and weep. Compare to single liner
> virtio_cread.
> 
> I happen to maintain Linux drivers too, so I care.
> 
It is harder for first time if no such construct exists; and it is clearly not the case.
This patch is already adding CVQ get and set command.
Async behavior already built into the virtio spec.
Extending it for extended config space just make device more cheaper to build.
Software cost is paid only once at device init time just like CVQ, AQ and more.

> > >
> > > Please let people just focus on fixing config space instead of
> > > temporary cvq hacks.
> >
> > The ask is to have predictable sizing for existing defined config space field and
> not keep infinitely growing by two interfaces.
> 
> I don't know what "predictable sizing" is or why does it matter.
> 
Because when device has two ways to access config space, it always have to account and build the interface that, hey some driver will not use DMA.
Hence have it always in the MMIO accessible area.

> I get the value of reduced MMIO register map.
> I get that the only way to get that appears to be to pass values around using
> DMA.
>
Ok. great.
 
> I don't think we need to throw away config space - just use DMA to access it.
> 
> And "existing defined" here seems to be at a completely random point in time.
> If someone wants to have fields in MMIO, I see no reason not to.

This demands two ways of access and that conflicts with your point of desire to do single way.
I proposed single way, extended config space via DMA interface, that is really as simple as that.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 05:06:40PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 12:46 PM
> 
> > Actually, my preblem is exactly if there's no one single interface that can do
> > everything.
> >
> Extended config space reading via single DMA interface is just enough.
> 
> > 
> > Two interfaces that do the same might seem redundant but there could be good
> > reasons for this, in this case legacy.
> >
> But legacy should not force it to newer interface. More below.
>  
> > See the difference?
> > 
> 
> > > I didn't propose two interfaces. Let me explain again.
> > >
> > > I proposed,
> > > a. all new fields to go via vq (single interface) b. all existing
> > > fields to stay in cfg space (single interface) c. if one wants to
> > > optimize optionally existing fields can utilize (same flexibility how you asked
> > for PF and VF notification).
> > >
> > >
> > > > We already need config space with provisioning.
> > > Provisioning does provision features, config space and control parameters,
> > msix vectors and more.
> > > Provisioning does _not_ need config space.
> > 
> > I mean it needs to provision it.
> > 
> No matter there is config space of dma, a device to be provisioned.
> 
> > > > In fact e.g. on Linux we want drivers to keep doing virtio_cread()
> > > > and all the DMA tricks should be hidden behind the scenes. You don't
> > > > like it that config space is on-chip? Build an interface for it not
> > > > to be on chip. There is simply no downside to that.
> > > >
> > > This is the ask to build software interface between driver and device to not
> > demand on-chip config space more importantly _predictably_.
> > 
> > Absolutely. So we would say that driver SHOULD use the DMA interface if it is
> > present.
> > 
> I am asking for driver MUST use the DMA interface for _extended_ config space.
> Why?
> For two reasons.
> 
> 1. Because than device is 100% sure that it does not fulfill MMIO needs
> 2. Single interface to access extended config space, which is what you asked for.
> 3. Single driver code because there is single way to get it.
> 

Right. But I think there's a better way.
Just say that any feature in MMIO MUST be also in DMA.
So any modern driver will have no reason at all to use MMIO - DMA
is a superset.


> > > > I came back after a small break and I still see this discussion that
> > > > keeps playing out as a big distraction.
> > > > In particular where are you doing to store the things that are for DMA?
> > > > In system RAM? We don't *have* anywhere in system RAM to store this,
> > > > we will need an interface to allocate system RAM and pass it to
> > > > device for this idea to work. So in 1.3 where we won't have this,
> > > > there is much less of an excuse to use a vq.
> > > To store what?
> > > CVQ is already there to do GET and SET operation.
> > 
> > To store whatever value CVQ is requesting. Where is it going to come from?
> >
> It is going to come from the device memory that does not have hard requirements of MMIO accessibility.
> For example slow flash memory.
>  
> > Asynchronous is just harder for software.  Look at the hoops virtio net has to
> > jump through to do control path over cvq and weep. Compare to single liner
> > virtio_cread.
> > 
> > I happen to maintain Linux drivers too, so I care.
> > 
> It is harder for first time if no such construct exists; and it is clearly not the case.
> This patch is already adding CVQ get and set command.

I expect get to mostly go unused.

> Async behavior already built into the virtio spec.
> Extending it for extended config space just make device more cheaper to build.
> Software cost is paid only once at device init time just like CVQ, AQ and more.
> 
> > > >
> > > > Please let people just focus on fixing config space instead of
> > > > temporary cvq hacks.
> > >
> > > The ask is to have predictable sizing for existing defined config space field and
> > not keep infinitely growing by two interfaces.
> > 
> > I don't know what "predictable sizing" is or why does it matter.
> > 
> Because when device has two ways to access config space, it always have to account and build the interface that, hey some driver will not use DMA.
> Hence have it always in the MMIO accessible area.

If we say that drivers should use DMA, they will.  If we additionally
explain that some features might not be in MMIO no sane driver will use
MMIO if it does not have to.
Or if it does then it has violated the spec and will get less features?




> > I get the value of reduced MMIO register map.
> > I get that the only way to get that appears to be to pass values around using
> > DMA.
> >
> Ok. great.
>  
> > I don't think we need to throw away config space - just use DMA to access it.
> > 
> > And "existing defined" here seems to be at a completely random point in time.
> > If someone wants to have fields in MMIO, I see no reason not to.
> 
> This demands two ways of access and that conflicts with your point of desire to do single way.
> I proposed single way, extended config space via DMA interface, that is really as simple as that.


My desire is to have a single way that works for everything.
We have legacy so we will have legacy ways too, these
might not work for everything.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 1:16 PM

> > 1. Because than device is 100% sure that it does not fulfill MMIO
> > needs 2. Single interface to access extended config space, which is what you
> asked for.
> > 3. Single driver code because there is single way to get it.
> >
> 
> Right. But I think there's a better way.
> Just say that any feature in MMIO MUST be also in DMA.
> So any modern driver will have no reason at all to use MMIO - DMA is a
> superset.
>
How does that relax the need of MMIO for the device?

> If we say that drivers should use DMA, they will.  If we additionally explain that
> some features might not be in MMIO no sane driver will use MMIO if it does not
> have to.
> Or if it does then it has violated the spec and will get less features?
> 
So than why to have it in MMIO anyway?
Why not say driver must use dma?

> > This demands two ways of access and that conflicts with your point of desire
> to do single way.
> > I proposed single way, extended config space via DMA interface, that is really
> as simple as that.
> 
> 
> My desire is to have a single way that works for everything.
> We have legacy so we will have legacy ways too, these might not work for
> everything.
Here is what can work well,

A device tells the offset of the extended config space to the driver.
Any field starting from that offset must be accessed via a DMA interface.
This way, driver must support DMA and all new features come from there.

If device chose to use MMIO, say sw, it can still do it using MMIO.
 

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 05:06:40PM +0000, Parav Pandit wrote:
> > > >
> > > > Please let people just focus on fixing config space instead of
> > > > temporary cvq hacks.
> > >
> > > The ask is to have predictable sizing for existing defined config space field and
> > not keep infinitely growing by two interfaces.
> > 
> > I don't know what "predictable sizing" is or why does it matter.
> > 
> Because when device has two ways to access config space, it always have to account and build the interface that, hey some driver will not use DMA.
> Hence have it always in the MMIO accessible area.

Maybe I get it. You want to use the new features as a carrot to force
drivers to implement DMA? You suspect they will ignore the spec
requirement just because things seem to work?

There's some logic here, for sure. you just might be right.

However, surely we can discuss this small tweak in 1.4 timeframe?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 1:24 PM


> > Because when device has two ways to access config space, it always have to
> account and build the interface that, hey some driver will not use DMA.
> > Hence have it always in the MMIO accessible area.
> 
> Maybe I get it. You want to use the new features as a carrot to force drivers to
> implement DMA? You suspect they will ignore the spec requirement just
> because things seem to work?
>
Right because it is not a must normative.
 
> There's some logic here, for sure. you just might be right.
> 
> However, surely we can discuss this small tweak in 1.4 timeframe?

Sure, if we prefer the DMA approach I don't have a problem in adding temporary one field to config space.

I propose to add a line to the spec " Device Configuration Space" section, something like,

Note: Any new device configuration space fields additional MUST consider accessing such fields via a DMA interface.

And this will guide the new patches of what to do instead of last moment rush.



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 05:38:29PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 1:24 PM
> 
> 
> > > Because when device has two ways to access config space, it always have to
> > account and build the interface that, hey some driver will not use DMA.
> > > Hence have it always in the MMIO accessible area.
> > 
> > Maybe I get it. You want to use the new features as a carrot to force drivers to
> > implement DMA? You suspect they will ignore the spec requirement just
> > because things seem to work?
> >
> Right because it is not a must normative.

Well SHOULD also does not mean "ok to just ignore".

	This word, or the adjective "RECOMMENDED", mean that there
	   may exist valid reasons in particular circumstances to ignore a
	   particular item, but the full implications must be understood and
	   carefully weighed before choosing a different course.



> > There's some logic here, for sure. you just might be right.
> > 
> > However, surely we can discuss this small tweak in 1.4 timeframe?
> 
> Sure, if we prefer the DMA approach I don't have a problem in adding temporary one field to config space.
> 
> I propose to add a line to the spec " Device Configuration Space" section, something like,
> 
> Note: Any new device configuration space fields additional MUST consider accessing such fields via a DMA interface.
> 
> And this will guide the new patches of what to do instead of last moment rush.

Yea, except again I'd probably make it a SHOULD: e.g. I can see how
switching to MMIO might be an option for qemu helping us debug DMA
issues.

The time to discuss this detail would be around when proposal for the DMA
access to config space is on list though: I feel this SHOULD vs MUST
is a small enough detail.

Going back to inner hash. If we move supported_tunnels back to config space,
do you feel we still need GET or just drop it? I note we do not
have GET for either hash or rss config.

And if we no longer have GET is there still a reason for a separate
command as opposed to a field in virtio_net_hash_config?
I know this was done in v11 but there it was misaligned.
We went with a command because we needed it for supported_tunnels
but now that is no longer the case and
there are reserved words in virtio_net_hash_config ...

Let me know how you feel it about that, not critical for me.


-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, June 28, 2023 3:45 PM
> > > Maybe I get it. You want to use the new features as a carrot to
> > > force drivers to implement DMA? You suspect they will ignore the
> > > spec requirement just because things seem to work?
> > >
> > Right because it is not a must normative.
> 
> Well SHOULD also does not mean "ok to just ignore".
> 
> 	This word, or the adjective "RECOMMENDED", mean that there
> 	   may exist valid reasons in particular circumstances to ignore a
> 	   particular item, but the full implications must be understood and
> 	   carefully weighed before choosing a different course.
>
RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
So rather a good design is device tells the starting offset for the extended config space.
And extended config space MUST be accessed using a DMA.
With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
This also gives the ability to maintain current config as MMIO for backward compatibility.
 
> 
> 
> > > There's some logic here, for sure. you just might be right.
> > >
> > > However, surely we can discuss this small tweak in 1.4 timeframe?
> >
> > Sure, if we prefer the DMA approach I don't have a problem in adding
> temporary one field to config space.
> >
> > I propose to add a line to the spec " Device Configuration Space"
> > section, something like,
> >
> > Note: Any new device configuration space fields additional MUST consider
> accessing such fields via a DMA interface.
> >
> > And this will guide the new patches of what to do instead of last moment
> rush.
> 
> Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
> MMIO might be an option for qemu helping us debug DMA issues.
>
There are too many queues whose debugging is needed and MMIO likely not the way to debug.
 
> The time to discuss this detail would be around when proposal for the DMA
> access to config space is on list though: I feel this SHOULD vs MUST is a small
> enough detail.
>
From implementation POV it is certainly critical and good step forward to optimize virtio interface.
 
> Going back to inner hash. If we move supported_tunnels back to config space,
> do you feel we still need GET or just drop it? I note we do not have GET for
> either hash or rss config.
>
For hash and rss config, debugging is missing. :)
Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
 
> And if we no longer have GET is there still a reason for a separate command as
> opposed to a field in virtio_net_hash_config?
> I know this was done in v11 but there it was misaligned.
> We went with a command because we needed it for supported_tunnels but
> now that is no longer the case and there are reserved words in
> virtio_net_hash_config ...
> 
> Let me know how you feel it about that, not critical for me.

struct virtio_net_hash_config reserved is fine.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/6/29 上午9:56, Parav Pandit 写道:
>
>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Wednesday, June 28, 2023 3:45 PM
>>>> Maybe I get it. You want to use the new features as a carrot to
>>>> force drivers to implement DMA? You suspect they will ignore the
>>>> spec requirement just because things seem to work?
>>>>
>>> Right because it is not a must normative.
>> Well SHOULD also does not mean "ok to just ignore".
>>
>> 	This word, or the adjective "RECOMMENDED", mean that there
>> 	   may exist valid reasons in particular circumstances to ignore a
>> 	   particular item, but the full implications must be understood and
>> 	   carefully weighed before choosing a different course.
>>
> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> So rather a good design is device tells the starting offset for the extended config space.
> And extended config space MUST be accessed using a DMA.
> With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
> This also gives the ability to maintain current config as MMIO for backward compatibility.
>   
>>
>>>> There's some logic here, for sure. you just might be right.
>>>>
>>>> However, surely we can discuss this small tweak in 1.4 timeframe?
>>> Sure, if we prefer the DMA approach I don't have a problem in adding
>> temporary one field to config space.
>>> I propose to add a line to the spec " Device Configuration Space"
>>> section, something like,
>>>
>>> Note: Any new device configuration space fields additional MUST consider
>> accessing such fields via a DMA interface.
>>> And this will guide the new patches of what to do instead of last moment
>> rush.
>>
>> Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
>> MMIO might be an option for qemu helping us debug DMA issues.
>>
> There are too many queues whose debugging is needed and MMIO likely not the way to debug.
>   
>> The time to discuss this detail would be around when proposal for the DMA
>> access to config space is on list though: I feel this SHOULD vs MUST is a small
>> enough detail.
>>
>  From implementation POV it is certainly critical and good step forward to optimize virtio interface.
>   
>> Going back to inner hash. If we move supported_tunnels back to config space,
>> do you feel we still need GET or just drop it? I note we do not have GET for
>> either hash or rss config.
>>
> For hash and rss config, debugging is missing. :)
> Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.

Great! Glad to hear this!

>   
>> And if we no longer have GET is there still a reason for a separate command as
>> opposed to a field in virtio_net_hash_config?
>> I know this was done in v11 but there it was misaligned.
>> We went with a command because we needed it for supported_tunnels but
>> now that is no longer the case and there are reserved words in
>> virtio_net_hash_config ...
>>
>> Let me know how you feel it about that, not critical for me.
> struct virtio_net_hash_config reserved is fine.

+1.

Inner header hash is orthogonal to RSS, and it's fine to have its own 
structure and commands.
There is no need to send additional RSS fields when we configure inner 
header hash.

Thanks.



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 10:05:09AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/6/29 上午9:56, Parav Pandit 写道:
> > 
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > > Maybe I get it. You want to use the new features as a carrot to
> > > > > force drivers to implement DMA? You suspect they will ignore the
> > > > > spec requirement just because things seem to work?
> > > > > 
> > > > Right because it is not a must normative.
> > > Well SHOULD also does not mean "ok to just ignore".
> > > 
> > > 	This word, or the adjective "RECOMMENDED", mean that there
> > > 	   may exist valid reasons in particular circumstances to ignore a
> > > 	   particular item, but the full implications must be understood and
> > > 	   carefully weighed before choosing a different course.
> > > 
> > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> > So rather a good design is device tells the starting offset for the extended config space.
> > And extended config space MUST be accessed using a DMA.
> > With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
> > This also gives the ability to maintain current config as MMIO for backward compatibility.
> > > 
> > > > > There's some logic here, for sure. you just might be right.
> > > > > 
> > > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > > temporary one field to config space.
> > > > I propose to add a line to the spec " Device Configuration Space"
> > > > section, something like,
> > > > 
> > > > Note: Any new device configuration space fields additional MUST consider
> > > accessing such fields via a DMA interface.
> > > > And this will guide the new patches of what to do instead of last moment
> > > rush.
> > > 
> > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
> > > MMIO might be an option for qemu helping us debug DMA issues.
> > > 
> > There are too many queues whose debugging is needed and MMIO likely not the way to debug.
> > > The time to discuss this detail would be around when proposal for the DMA
> > > access to config space is on list though: I feel this SHOULD vs MUST is a small
> > > enough detail.
> > > 
> >  From implementation POV it is certainly critical and good step forward to optimize virtio interface.
> > > Going back to inner hash. If we move supported_tunnels back to config space,
> > > do you feel we still need GET or just drop it? I note we do not have GET for
> > > either hash or rss config.
> > > 
> > For hash and rss config, debugging is missing. :)
> > Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
> 
> Great! Glad to hear this!
> 
> > > And if we no longer have GET is there still a reason for a separate command as
> > > opposed to a field in virtio_net_hash_config?
> > > I know this was done in v11 but there it was misaligned.
> > > We went with a command because we needed it for supported_tunnels but
> > > now that is no longer the case and there are reserved words in
> > > virtio_net_hash_config ...
> > > 
> > > Let me know how you feel it about that, not critical for me.
> > struct virtio_net_hash_config reserved is fine.
> 
> +1.
> 
> Inner header hash is orthogonal to RSS, and it's fine to have its own
> structure and commands.
> There is no need to send additional RSS fields when we configure inner
> header hash.
> 
> Thanks.

Not RSS, hash calculations. It's not critical, but I note that
practically you said you will enable this with symmetric hash
so it makes sense to me to send this in the same command
with the key.

Not critical though if there's opposition.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/6/29 下午7:48, Michael S. Tsirkin 写道:
> On Thu, Jun 29, 2023 at 10:05:09AM +0800, Heng Qi wrote:
>>
>> 在 2023/6/29 上午9:56, Parav Pandit 写道:
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Wednesday, June 28, 2023 3:45 PM
>>>>>> Maybe I get it. You want to use the new features as a carrot to
>>>>>> force drivers to implement DMA? You suspect they will ignore the
>>>>>> spec requirement just because things seem to work?
>>>>>>
>>>>> Right because it is not a must normative.
>>>> Well SHOULD also does not mean "ok to just ignore".
>>>>
>>>> 	This word, or the adjective "RECOMMENDED", mean that there
>>>> 	   may exist valid reasons in particular circumstances to ignore a
>>>> 	   particular item, but the full implications must be understood and
>>>> 	   carefully weighed before choosing a different course.
>>>>
>>> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
>>> So rather a good design is device tells the starting offset for the extended config space.
>>> And extended config space MUST be accessed using a DMA.
>>> With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
>>> This also gives the ability to maintain current config as MMIO for backward compatibility.
>>>>>> There's some logic here, for sure. you just might be right.
>>>>>>
>>>>>> However, surely we can discuss this small tweak in 1.4 timeframe?
>>>>> Sure, if we prefer the DMA approach I don't have a problem in adding
>>>> temporary one field to config space.
>>>>> I propose to add a line to the spec " Device Configuration Space"
>>>>> section, something like,
>>>>>
>>>>> Note: Any new device configuration space fields additional MUST consider
>>>> accessing such fields via a DMA interface.
>>>>> And this will guide the new patches of what to do instead of last moment
>>>> rush.
>>>>
>>>> Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
>>>> MMIO might be an option for qemu helping us debug DMA issues.
>>>>
>>> There are too many queues whose debugging is needed and MMIO likely not the way to debug.
>>>> The time to discuss this detail would be around when proposal for the DMA
>>>> access to config space is on list though: I feel this SHOULD vs MUST is a small
>>>> enough detail.
>>>>
>>>   From implementation POV it is certainly critical and good step forward to optimize virtio interface.
>>>> Going back to inner hash. If we move supported_tunnels back to config space,
>>>> do you feel we still need GET or just drop it? I note we do not have GET for
>>>> either hash or rss config.
>>>>
>>> For hash and rss config, debugging is missing. :)
>>> Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
>> Great! Glad to hear this!
>>
>>>> And if we no longer have GET is there still a reason for a separate command as
>>>> opposed to a field in virtio_net_hash_config?
>>>> I know this was done in v11 but there it was misaligned.
>>>> We went with a command because we needed it for supported_tunnels but
>>>> now that is no longer the case and there are reserved words in
>>>> virtio_net_hash_config ...
>>>>
>>>> Let me know how you feel it about that, not critical for me.
>>> struct virtio_net_hash_config reserved is fine.
>> +1.
>>
>> Inner header hash is orthogonal to RSS, and it's fine to have its own
>> structure and commands.
>> There is no need to send additional RSS fields when we configure inner
>> header hash.
>>
>> Thanks.
> Not RSS, hash calculations. It's not critical, but I note that
> practically you said you will enable this with symmetric hash
> so it makes sense to me to send this in the same command

This works for me.

Thanks.

> with the key.
>
> Not critical though if there's opposition.
>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, June 29, 2023 7:48 AM


> > > struct virtio_net_hash_config reserved is fine.
> >
> > +1.
> >
> > Inner header hash is orthogonal to RSS, and it's fine to have its own
> > structure and commands.
> > There is no need to send additional RSS fields when we configure inner
> > header hash.
> >
> > Thanks.
> 
> Not RSS, hash calculations. It's not critical, but I note that practically you said
> you will enable this with symmetric hash so it makes sense to me to send this in
> the same command with the key.
> 

In the v19, we have,

+\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.

So it is done along with rss, so in same struct as rss config is fine.

Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, June 29, 2023 7:48 AM
> 
> 
> > > > struct virtio_net_hash_config reserved is fine.
> > >
> > > +1.
> > >
> > > Inner header hash is orthogonal to RSS, and it's fine to have its own
> > > structure and commands.
> > > There is no need to send additional RSS fields when we configure inner
> > > header hash.
> > >
> > > Thanks.
> > 
> > Not RSS, hash calculations. It's not critical, but I note that practically you said
> > you will enable this with symmetric hash so it makes sense to me to send this in
> > the same command with the key.
> > 
> 
> In the v19, we have,
> 
> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> 
> So it is done along with rss, so in same struct as rss config is fine.

Do you mean having both virtio_net_rss_config and virtio_net_hash_config
have enabled_hash_types?
Like this:

struct virtio_net_rss_config {
     le32 hash_types;
     le16 indirection_table_mask;
     struct rss_rq_id unclassified_queue;
     struct rss_rq_id indirection_table[indirection_table_length];
     le16 max_tx_vq;
     u8 hash_key_length;
     u8 hash_key_data[hash_key_length];
+    le32 enabled_tunnel_types;	
};

struct virtio_net_hash_config {
     le32 hash_types;
-    le16 reserved[4];
+    le32 enabled_tunnel_types;
+    le16 reserved[2];
     u8 hash_key_length;
     u8 hash_key_data[hash_key_length];
};


If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types in virtio_net_rss_config
will follow the variable length field and cause misalignment.

If we let the inner header hash reuse the virtio_net_hash_config structure, it can work, but the only disadvantage
is that the configuration of the inner header hash and *RSS*(not hash calculations) becomes somewhat coupled.
Just imagine:
If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT,
1. then if we only want to configure the inner header hash (such as enabled_tunnel_types), it is good for us to send
virtio_net_hash_config alone;
2. but then if we want to configure the inner header hash and RSS (such as indirection table), we need to send all
virtio_net_rss_config and virtio_net_hash_config once, because virtio_net_rss_config now does not carry enabled_tunnel_types
due to misalignment.

So, I think the following structure will make it clearer to configure inner header hash and RSS/hash calculation.
But in any case, if we still propose to reuse virtio_net_hash_config proposal, I am ok, no objection:

1. The supported_tunnel_types are placed in the device config space;

2.
Reserve the following structure:

      struct virtnet_hash_tunnel {
le32 enabled_tunnel_types;
      };

3. Reserve the SET command for enabled_tunnel_types and remove the GET
command for enabled_tunnel_types.

[1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html

Thanks a lot!

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Thursday, June 29, 2023 8:54 PM
> 
> On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Thursday, June 29, 2023 7:48 AM
> >
> >
> > > > > struct virtio_net_hash_config reserved is fine.
> > > >
> > > > +1.
> > > >
> > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > own structure and commands.
> > > > There is no need to send additional RSS fields when we configure
> > > > inner header hash.
> > > >
> > > > Thanks.
> > >
> > > Not RSS, hash calculations. It's not critical, but I note that
> > > practically you said you will enable this with symmetric hash so it
> > > makes sense to me to send this in the same command with the key.
> > >
> >
> > In the v19, we have,
> >
> > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> >
> > So it is done along with rss, so in same struct as rss config is fine.
> 
> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> have enabled_hash_types?
> Like this:
> 
> struct virtio_net_rss_config {
>      le32 hash_types;
>      le16 indirection_table_mask;
>      struct rss_rq_id unclassified_queue;
>      struct rss_rq_id indirection_table[indirection_table_length];
>      le16 max_tx_vq;
>      u8 hash_key_length;
>      u8 hash_key_data[hash_key_length];
> +    le32 enabled_tunnel_types;
> };
> 
> struct virtio_net_hash_config {
>      le32 hash_types;
> -    le16 reserved[4];
> +    le32 enabled_tunnel_types;
> +    le16 reserved[2];
>      u8 hash_key_length;
>      u8 hash_key_data[hash_key_length];
> };
> 
> 
> If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> in virtio_net_rss_config will follow the variable length field and cause
> misalignment.
> 
> If we let the inner header hash reuse the virtio_net_hash_config structure, it
> can work, but the only disadvantage is that the configuration of the inner
> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> Just imagine:
> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> then if we only want to configure the inner header hash (such as
> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> 2. but then if we want to configure the inner header hash and RSS (such as
> indirection table), we need to send all virtio_net_rss_config and
> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> enabled_tunnel_types due to misalignment.
> 
> So, I think the following structure will make it clearer to configure inner header
> hash and RSS/hash calculation.
> But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> am ok, no objection:
> 
> 1. The supported_tunnel_types are placed in the device config space;
>
Yes. I forgot the variable length part.
 
The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.

Given these two disadvantages, I also prefer independent SET command the way you have it.
 
> 2.
> Reserve the following structure:
> 
>       struct virtnet_hash_tunnel {
> le32 enabled_tunnel_types;
>       };
> 
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> command for enabled_tunnel_types.
> 
> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> 
> Thanks a lot!

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/6/30 上午9:36, Parav Pandit 写道:
>
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Thursday, June 29, 2023 8:54 PM
>>
>> On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
>>>
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Thursday, June 29, 2023 7:48 AM
>>>
>>>>>> struct virtio_net_hash_config reserved is fine.
>>>>> +1.
>>>>>
>>>>> Inner header hash is orthogonal to RSS, and it's fine to have its
>>>>> own structure and commands.
>>>>> There is no need to send additional RSS fields when we configure
>>>>> inner header hash.
>>>>>
>>>>> Thanks.
>>>> Not RSS, hash calculations. It's not critical, but I note that
>>>> practically you said you will enable this with symmetric hash so it
>>>> makes sense to me to send this in the same command with the key.
>>>>
>>> In the v19, we have,
>>>
>>> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
>> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>>> So it is done along with rss, so in same struct as rss config is fine.
>> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
>> have enabled_hash_types?
>> Like this:
>>
>> struct virtio_net_rss_config {
>>       le32 hash_types;
>>       le16 indirection_table_mask;
>>       struct rss_rq_id unclassified_queue;
>>       struct rss_rq_id indirection_table[indirection_table_length];
>>       le16 max_tx_vq;
>>       u8 hash_key_length;
>>       u8 hash_key_data[hash_key_length];
>> +    le32 enabled_tunnel_types;
>> };
>>
>> struct virtio_net_hash_config {
>>       le32 hash_types;
>> -    le16 reserved[4];
>> +    le32 enabled_tunnel_types;
>> +    le16 reserved[2];
>>       u8 hash_key_length;
>>       u8 hash_key_data[hash_key_length];
>> };
>>
>>
>> If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
>> in virtio_net_rss_config will follow the variable length field and cause
>> misalignment.
>>
>> If we let the inner header hash reuse the virtio_net_hash_config structure, it
>> can work, but the only disadvantage is that the configuration of the inner
>> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
>> Just imagine:
>> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
>> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
>> then if we only want to configure the inner header hash (such as
>> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
>> 2. but then if we want to configure the inner header hash and RSS (such as
>> indirection table), we need to send all virtio_net_rss_config and
>> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
>> enabled_tunnel_types due to misalignment.
>>
>> So, I think the following structure will make it clearer to configure inner header
>> hash and RSS/hash calculation.
>> But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
>> am ok, no objection:
>>
>> 1. The supported_tunnel_types are placed in the device config space;
>>
> Yes. I forgot the variable length part.
>   
> The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
>
> Given these two disadvantages, I also prefer independent SET command the way you have it.

OK, let's wait for Michael's input again.

Thanks.

>   
>> 2.
>> Reserve the following structure:
>>
>>        struct virtnet_hash_tunnel {
>> le32 enabled_tunnel_types;
>>        };
>>
>> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
>> command for enabled_tunnel_types.
>>
>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
>>
>> Thanks a lot!


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > 
> > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > Sent: Thursday, June 29, 2023 8:54 PM
> > > 
> > > On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> > > > 
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > 
> > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > +1.
> > > > > > 
> > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > own structure and commands.
> > > > > > There is no need to send additional RSS fields when we configure
> > > > > > inner header hash.
> > > > > > 
> > > > > > Thanks.
> > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > practically you said you will enable this with symmetric hash so it
> > > > > makes sense to me to send this in the same command with the key.
> > > > > 
> > > > In the v19, we have,
> > > > 
> > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > So it is done along with rss, so in same struct as rss config is fine.
> > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > have enabled_hash_types?
> > > Like this:
> > > 
> > > struct virtio_net_rss_config {
> > >       le32 hash_types;
> > >       le16 indirection_table_mask;
> > >       struct rss_rq_id unclassified_queue;
> > >       struct rss_rq_id indirection_table[indirection_table_length];
> > >       le16 max_tx_vq;
> > >       u8 hash_key_length;
> > >       u8 hash_key_data[hash_key_length];
> > > +    le32 enabled_tunnel_types;
> > > };
> > > 
> > > struct virtio_net_hash_config {
> > >       le32 hash_types;
> > > -    le16 reserved[4];
> > > +    le32 enabled_tunnel_types;
> > > +    le16 reserved[2];
> > >       u8 hash_key_length;
> > >       u8 hash_key_data[hash_key_length];
> > > };

Oh, I forgot that rss and hash had identical structures.
And we want to keep that I think.

But now it's not clear to me: does the same enabled_tunnel_types
apply to both hash calculation and rss?
I note we normally have separate configs for hash and rss.
So to keep it consistent what should we do? two set commands?
Or does enabled_tunnel_types apply to both rss and hash?

We should have reserved more space. We can still do it if you like:

struct virtio_net_rss_tunnel_config {
      le32 enabled_tunnel_types;
      le16 reserved[6];
      struct virtio_net_rss_config hash;
};

struct virtio_net_hash_tunnel_config {
      le32 enabled_tunnel_types;
      le16 reserved[6];
      struct virtio_net_hash_config hash;
};

?





> > > 
> > > 
> > > If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> > > in virtio_net_rss_config will follow the variable length field and cause
> > > misalignment.
> > > 
> > > If we let the inner header hash reuse the virtio_net_hash_config structure, it
> > > can work, but the only disadvantage is that the configuration of the inner
> > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > Just imagine:
> > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > then if we only want to configure the inner header hash (such as
> > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > indirection table), we need to send all virtio_net_rss_config and
> > > virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> > > enabled_tunnel_types due to misalignment.
> > > 
> > > So, I think the following structure will make it clearer to configure inner header
> > > hash and RSS/hash calculation.
> > > But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> > > am ok, no objection:
> > > 
> > > 1. The supported_tunnel_types are placed in the device config space;
> > > 
> > Yes. I forgot the variable length part.
> > The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.

Or it could be an advantage since one normally wants to configure a
symmetric key with this. Further device can just use the new config
with no need to check what the old one was. I'd call it a wash.

> > Given these two disadvantages, I also prefer independent SET command the way you have it.
> 
> OK, let's wait for Michael's input again.
> 
> Thanks.


This part is not critical to me, but now I understand we need two sets of SET commands.


> > > 2.
> > > Reserve the following structure:
> > > 
> > >        struct virtnet_hash_tunnel {
> > > le32 enabled_tunnel_types;
> > >        };
> > > 
> > > 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> > > command for enabled_tunnel_types.
> > > 
> > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> > > 
> > > Thanks a lot!


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
>>
>> 在 2023/6/30 上午9:36, Parav Pandit 写道:
>>>> From: Heng Qi <hengqi@linux.alibaba.com>
>>>> Sent: Thursday, June 29, 2023 8:54 PM
>>>>
>>>> On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
>>>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Sent: Thursday, June 29, 2023 7:48 AM
>>>>>>>> struct virtio_net_hash_config reserved is fine.
>>>>>>> +1.
>>>>>>>
>>>>>>> Inner header hash is orthogonal to RSS, and it's fine to have its
>>>>>>> own structure and commands.
>>>>>>> There is no need to send additional RSS fields when we configure
>>>>>>> inner header hash.
>>>>>>>
>>>>>>> Thanks.
>>>>>> Not RSS, hash calculations. It's not critical, but I note that
>>>>>> practically you said you will enable this with symmetric hash so it
>>>>>> makes sense to me to send this in the same command with the key.
>>>>>>
>>>>> In the v19, we have,
>>>>>
>>>>> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
>>>> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>>>>> So it is done along with rss, so in same struct as rss config is fine.
>>>> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
>>>> have enabled_hash_types?
>>>> Like this:
>>>>
>>>> struct virtio_net_rss_config {
>>>>        le32 hash_types;
>>>>        le16 indirection_table_mask;
>>>>        struct rss_rq_id unclassified_queue;
>>>>        struct rss_rq_id indirection_table[indirection_table_length];
>>>>        le16 max_tx_vq;
>>>>        u8 hash_key_length;
>>>>        u8 hash_key_data[hash_key_length];
>>>> +    le32 enabled_tunnel_types;
>>>> };
>>>>
>>>> struct virtio_net_hash_config {
>>>>        le32 hash_types;
>>>> -    le16 reserved[4];
>>>> +    le32 enabled_tunnel_types;
>>>> +    le16 reserved[2];
>>>>        u8 hash_key_length;
>>>>        u8 hash_key_data[hash_key_length];
>>>> };
> Oh, I forgot that rss and hash had identical structures.
> And we want to keep that I think.
>
> But now it's not clear to me: does the same enabled_tunnel_types
> apply to both hash calculation and rss?

Yes. What I'm trying to say is that making the inner header hash and 
RSS/hash calculation clear delimitation will make everything easier.
The inner header hash just configures enabled_tunnel_types.
The RSS/hash calculation is configured as before without modification.

> I note we normally have separate configs for hash and rss.
> So to keep it consistent what should we do? two set commands?

As I explained above, like outer udp port hash/symmetric toeplitz hash, 
these fall under the umbrella of RSS/hash calculation.
Let's keep the inner header hash simple.

> Or does enabled_tunnel_types apply to both rss and hash?

Certainly. See:

     +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ 
along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.

>
> We should have reserved more space. We can still do it if you like:
>
> struct virtio_net_rss_tunnel_config {
>        le32 enabled_tunnel_types;
>        le16 reserved[6];
>        struct virtio_net_rss_config hash;
> };
>
> struct virtio_net_hash_tunnel_config {
>        le32 enabled_tunnel_types;
>        le16 reserved[6];
>        struct virtio_net_hash_config hash;
> };
>
> ?
>
>
>
>
>
>>>>
>>>> If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
>>>> in virtio_net_rss_config will follow the variable length field and cause
>>>> misalignment.
>>>>
>>>> If we let the inner header hash reuse the virtio_net_hash_config structure, it
>>>> can work, but the only disadvantage is that the configuration of the inner
>>>> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
>>>> Just imagine:
>>>> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
>>>> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
>>>> then if we only want to configure the inner header hash (such as
>>>> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
>>>> 2. but then if we want to configure the inner header hash and RSS (such as
>>>> indirection table), we need to send all virtio_net_rss_config and
>>>> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
>>>> enabled_tunnel_types due to misalignment.
>>>>
>>>> So, I think the following structure will make it clearer to configure inner header
>>>> hash and RSS/hash calculation.
>>>> But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
>>>> am ok, no objection:
>>>>
>>>> 1. The supported_tunnel_types are placed in the device config space;
>>>>
>>> Yes. I forgot the variable length part.
>>> The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
> Or it could be an advantage since one normally wants to configure a
> symmetric key with this. Further device can just use the new config

When we want to configure the hash key, he continues to use the previous 
rss/hash calculation interface. This is ok.

Thanks.

> with no need to check what the old one was. I'd call it a wash.
>
>>> Given these two disadvantages, I also prefer independent SET command the way you have it.
>> OK, let's wait for Michael's input again.
>>
>> Thanks.
>
> This part is not critical to me, but now I understand we need two sets of SET commands.
>
>
>>>> 2.
>>>> Reserve the following structure:
>>>>
>>>>         struct virtnet_hash_tunnel {
>>>> le32 enabled_tunnel_types;
>>>>         };
>>>>
>>>> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
>>>> command for enabled_tunnel_types.
>>>>
>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
>>>>
>>>> Thanks a lot!
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > 
> > > > > On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > +1.
> > > > > > > > 
> > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > > > own structure and commands.
> > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > inner header hash.
> > > > > > > > 
> > > > > > > > Thanks.
> > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > practically you said you will enable this with symmetric hash so it
> > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > > 
> > > > > > In the v19, we have,
> > > > > > 
> > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > So it is done along with rss, so in same struct as rss config is fine.
> > > > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > > > have enabled_hash_types?
> > > > > Like this:
> > > > > 
> > > > > struct virtio_net_rss_config {
> > > > >        le32 hash_types;
> > > > >        le16 indirection_table_mask;
> > > > >        struct rss_rq_id unclassified_queue;
> > > > >        struct rss_rq_id indirection_table[indirection_table_length];
> > > > >        le16 max_tx_vq;
> > > > >        u8 hash_key_length;
> > > > >        u8 hash_key_data[hash_key_length];
> > > > > +    le32 enabled_tunnel_types;
> > > > > };
> > > > > 
> > > > > struct virtio_net_hash_config {
> > > > >        le32 hash_types;
> > > > > -    le16 reserved[4];
> > > > > +    le32 enabled_tunnel_types;
> > > > > +    le16 reserved[2];
> > > > >        u8 hash_key_length;
> > > > >        u8 hash_key_data[hash_key_length];
> > > > > };
> > Oh, I forgot that rss and hash had identical structures.
> > And we want to keep that I think.
> > 
> > But now it's not clear to me: does the same enabled_tunnel_types
> > apply to both hash calculation and rss?
> 
> Yes. What I'm trying to say is that making the inner header hash and
> RSS/hash calculation clear delimitation will make everything easier.
> The inner header hash just configures enabled_tunnel_types.
> The RSS/hash calculation is configured as before without modification.
> 
> > I note we normally have separate configs for hash and rss.
> > So to keep it consistent what should we do? two set commands?
> 
> As I explained above, like outer udp port hash/symmetric toeplitz hash,
> these fall under the umbrella of RSS/hash calculation.

Yes but note that symmetric toeplitz hash has to be configured
separately for RSS and for hashing.

> Let's keep the inner header hash simple.
> 
> > Or does enabled_tunnel_types apply to both rss and hash?
> 
> Certainly. See:
> 
>     +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.


It does not really say that.



> > 
> > We should have reserved more space. We can still do it if you like:
> > 
> > struct virtio_net_rss_tunnel_config {
> >        le32 enabled_tunnel_types;
> >        le16 reserved[6];
> >        struct virtio_net_rss_config hash;
> > };
> > 
> > struct virtio_net_hash_tunnel_config {
> >        le32 enabled_tunnel_types;
> >        le16 reserved[6];
> >        struct virtio_net_hash_config hash;
> > };
> > 
> > ?
> > 
> > 
> > 
> > 
> > 
> > > > > 
> > > > > If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> > > > > in virtio_net_rss_config will follow the variable length field and cause
> > > > > misalignment.
> > > > > 
> > > > > If we let the inner header hash reuse the virtio_net_hash_config structure, it
> > > > > can work, but the only disadvantage is that the configuration of the inner
> > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > Just imagine:
> > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > then if we only want to configure the inner header hash (such as
> > > > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> > > > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> > > > > enabled_tunnel_types due to misalignment.
> > > > > 
> > > > > So, I think the following structure will make it clearer to configure inner header
> > > > > hash and RSS/hash calculation.
> > > > > But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> > > > > am ok, no objection:
> > > > > 
> > > > > 1. The supported_tunnel_types are placed in the device config space;
> > > > > 
> > > > Yes. I forgot the variable length part.
> > > > The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
> > Or it could be an advantage since one normally wants to configure a
> > symmetric key with this. Further device can just use the new config
> 
> When we want to configure the hash key, he continues to use the previous
> rss/hash calculation interface. This is ok.
> 
> Thanks.

I don't understand this sentence. My point is simply that
to use the tunnel key has to be symmetric. So two commands
will be required: one to set tunnel types, one to
set the key.


> > with no need to check what the old one was. I'd call it a wash.
> > 
> > > > Given these two disadvantages, I also prefer independent SET command the way you have it.
> > > OK, let's wait for Michael's input again.
> > > 
> > > Thanks.
> > 
> > This part is not critical to me, but now I understand we need two sets of SET commands.
> > 
> > 
> > > > > 2.
> > > > > Reserve the following structure:
> > > > > 
> > > > >         struct virtnet_hash_tunnel {
> > > > > le32 enabled_tunnel_types;
> > > > >         };
> > > > > 
> > > > > 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> > > > > command for enabled_tunnel_types.
> > > > > 
> > > > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> > > > > 
> > > > > Thanks a lot!
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
>>
>> 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
>>> On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
>>>> 在 2023/6/30 上午9:36, Parav Pandit 写道:
>>>>>> From: Heng Qi <hengqi@linux.alibaba.com>
>>>>>> Sent: Thursday, June 29, 2023 8:54 PM
>>>>>>
>>>>>> On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
>>>>>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>> Sent: Thursday, June 29, 2023 7:48 AM
>>>>>>>>>> struct virtio_net_hash_config reserved is fine.
>>>>>>>>> +1.
>>>>>>>>>
>>>>>>>>> Inner header hash is orthogonal to RSS, and it's fine to have its
>>>>>>>>> own structure and commands.
>>>>>>>>> There is no need to send additional RSS fields when we configure
>>>>>>>>> inner header hash.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>> Not RSS, hash calculations. It's not critical, but I note that
>>>>>>>> practically you said you will enable this with symmetric hash so it
>>>>>>>> makes sense to me to send this in the same command with the key.
>>>>>>>>
>>>>>>> In the v19, we have,
>>>>>>>
>>>>>>> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
>>>>>> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>>>>>>> So it is done along with rss, so in same struct as rss config is fine.
>>>>>> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
>>>>>> have enabled_hash_types?
>>>>>> Like this:
>>>>>>
>>>>>> struct virtio_net_rss_config {
>>>>>>         le32 hash_types;
>>>>>>         le16 indirection_table_mask;
>>>>>>         struct rss_rq_id unclassified_queue;
>>>>>>         struct rss_rq_id indirection_table[indirection_table_length];
>>>>>>         le16 max_tx_vq;
>>>>>>         u8 hash_key_length;
>>>>>>         u8 hash_key_data[hash_key_length];
>>>>>> +    le32 enabled_tunnel_types;
>>>>>> };
>>>>>>
>>>>>> struct virtio_net_hash_config {
>>>>>>         le32 hash_types;
>>>>>> -    le16 reserved[4];
>>>>>> +    le32 enabled_tunnel_types;
>>>>>> +    le16 reserved[2];
>>>>>>         u8 hash_key_length;
>>>>>>         u8 hash_key_data[hash_key_length];
>>>>>> };
>>> Oh, I forgot that rss and hash had identical structures.
>>> And we want to keep that I think.
>>>
>>> But now it's not clear to me: does the same enabled_tunnel_types
>>> apply to both hash calculation and rss?
>> Yes. What I'm trying to say is that making the inner header hash and
>> RSS/hash calculation clear delimitation will make everything easier.
>> The inner header hash just configures enabled_tunnel_types.
>> The RSS/hash calculation is configured as before without modification.
>>
>>> I note we normally have separate configs for hash and rss.
>>> So to keep it consistent what should we do? two set commands?
>> As I explained above, like outer udp port hash/symmetric toeplitz hash,
>> these fall under the umbrella of RSS/hash calculation.
> Yes but note that symmetric toeplitz hash has to be configured
> separately for RSS and for hashing.

Yes, this requires a field like \field{mode}, with different values 
corresponding to different hashing algorithms, such as toeplitz or 
symmetric toeplitz.
The outer port hash belongs to RSS/hash calculation.

>
>> Let's keep the inner header hash simple.
>>
>>> Or does enabled_tunnel_types apply to both rss and hash?
>> Certainly. See:
>>
>>      +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
>> with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>
> It does not really say that.
Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command 
is applied to hash and RSS. Yes.
When one wants to configure inner header hash separately, use 
VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types 
separately.
When one wants to configure both inner header hash and RSS/hash, use 
VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with 
VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
The inner header hash is decoupled from RSS/hash, and no extra fields 
will be sent every configuration.

>
>
>
>>> We should have reserved more space. We can still do it if you like:
>>>
>>> struct virtio_net_rss_tunnel_config {
>>>         le32 enabled_tunnel_types;
>>>         le16 reserved[6];
>>>         struct virtio_net_rss_config hash;
>>> };
>>>
>>> struct virtio_net_hash_tunnel_config {
>>>         le32 enabled_tunnel_types;
>>>         le16 reserved[6];
>>>         struct virtio_net_hash_config hash;
>>> };
>>>
>>> ?
>>>
>>>
>>>
>>>
>>>
>>>>>> If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
>>>>>> in virtio_net_rss_config will follow the variable length field and cause
>>>>>> misalignment.
>>>>>>
>>>>>> If we let the inner header hash reuse the virtio_net_hash_config structure, it
>>>>>> can work, but the only disadvantage is that the configuration of the inner
>>>>>> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
>>>>>> Just imagine:
>>>>>> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
>>>>>> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
>>>>>> then if we only want to configure the inner header hash (such as
>>>>>> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
>>>>>> 2. but then if we want to configure the inner header hash and RSS (such as
>>>>>> indirection table), we need to send all virtio_net_rss_config and
>>>>>> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
>>>>>> enabled_tunnel_types due to misalignment.
>>>>>>
>>>>>> So, I think the following structure will make it clearer to configure inner header
>>>>>> hash and RSS/hash calculation.
>>>>>> But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
>>>>>> am ok, no objection:
>>>>>>
>>>>>> 1. The supported_tunnel_types are placed in the device config space;
>>>>>>
>>>>> Yes. I forgot the variable length part.
>>>>> The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
>>> Or it could be an advantage since one normally wants to configure a
>>> symmetric key with this. Further device can just use the new config
>> When we want to configure the hash key, he continues to use the previous
>> rss/hash calculation interface. This is ok.
>>
>> Thanks.
> I don't understand this sentence. My point is simply that
> to use the tunnel key has to be symmetric. So two commands
> will be required: one to set tunnel types, one to
> set the key.

Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET 
and the other
should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.

Thanks.

>
>>> with no need to check what the old one was. I'd call it a wash.
>>>
>>>>> Given these two disadvantages, I also prefer independent SET command the way you have it.
>>>> OK, let's wait for Michael's input again.
>>>>
>>>> Thanks.
>>> This part is not critical to me, but now I understand we need two sets of SET commands.
>>>
>>>
>>>>>> 2.
>>>>>> Reserve the following structure:
>>>>>>
>>>>>>          struct virtnet_hash_tunnel {
>>>>>> le32 enabled_tunnel_types;
>>>>>>          };
>>>>>>
>>>>>> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
>>>>>> command for enabled_tunnel_types.
>>>>>>
>>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
>>>>>>
>>>>>> Thanks a lot!
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > > 
> > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> > > > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > +1.
> > > > > > > > > > 
> > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > > > > > own structure and commands.
> > > > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > > > inner header hash.
> > > > > > > > > > 
> > > > > > > > > > Thanks.
> > > > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > > > practically you said you will enable this with symmetric hash so it
> > > > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > > > > 
> > > > > > > > In the v19, we have,
> > > > > > > > 
> > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > So it is done along with rss, so in same struct as rss config is fine.
> > > > > > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > > > > > have enabled_hash_types?
> > > > > > > Like this:
> > > > > > > 
> > > > > > > struct virtio_net_rss_config {
> > > > > > >         le32 hash_types;
> > > > > > >         le16 indirection_table_mask;
> > > > > > >         struct rss_rq_id unclassified_queue;
> > > > > > >         struct rss_rq_id indirection_table[indirection_table_length];
> > > > > > >         le16 max_tx_vq;
> > > > > > >         u8 hash_key_length;
> > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > };
> > > > > > > 
> > > > > > > struct virtio_net_hash_config {
> > > > > > >         le32 hash_types;
> > > > > > > -    le16 reserved[4];
> > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > +    le16 reserved[2];
> > > > > > >         u8 hash_key_length;
> > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > };
> > > > Oh, I forgot that rss and hash had identical structures.
> > > > And we want to keep that I think.
> > > > 
> > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > apply to both hash calculation and rss?
> > > Yes. What I'm trying to say is that making the inner header hash and
> > > RSS/hash calculation clear delimitation will make everything easier.
> > > The inner header hash just configures enabled_tunnel_types.
> > > The RSS/hash calculation is configured as before without modification.
> > > 
> > > > I note we normally have separate configs for hash and rss.
> > > > So to keep it consistent what should we do? two set commands?
> > > As I explained above, like outer udp port hash/symmetric toeplitz hash,
> > > these fall under the umbrella of RSS/hash calculation.
> > Yes but note that symmetric toeplitz hash has to be configured
> > separately for RSS and for hashing.
> 
> Yes, this requires a field like \field{mode}, with different values
> corresponding to different hashing algorithms, such as toeplitz or symmetric
> toeplitz.
> The outer port hash belongs to RSS/hash calculation.

So there will be need for more fields.
To me this implies extending with struct virtio_net_rss_tunnel_config
is a better idea since we then have some reserved space to
put "mode" down the road (in the reserved[6] space below).

No?

> > 
> > > Let's keep the inner header hash simple.
> > > 
> > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > Certainly. See:
> > > 
> > >      +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> > > with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > 
> > It does not really say that.
> Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
> applied to hash and RSS. Yes.
> When one wants to configure inner header hash separately, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
> separately.
> When one wants to configure both inner header hash and RSS/hash, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> The inner header hash is decoupled from RSS/hash, and no extra fields will
> be sent every configuration.

But why is tunnel so different? Rest of things can be configured
for hash and for rss separately.


For example I can configure hash for IPv4 and IPv6 but RSS only for
IPv6. But for some reason, if I configure tunnel for GRE it will
affect both hash and RSS. Why? It seems quite possible to me that I
was offload for hash for GRE but don't want to use it
for RSS to avoid RX underrun issues we discussed.


> > 
> > 
> > 
> > > > We should have reserved more space. We can still do it if you like:
> > > > 
> > > > struct virtio_net_rss_tunnel_config {
> > > >         le32 enabled_tunnel_types;
> > > >         le16 reserved[6];
> > > >         struct virtio_net_rss_config hash;
> > > > };
> > > > 
> > > > struct virtio_net_hash_tunnel_config {
> > > >         le32 enabled_tunnel_types;
> > > >         le16 reserved[6];
> > > >         struct virtio_net_hash_config hash;
> > > > };
> > > > 
> > > > ?
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > > > > If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> > > > > > > in virtio_net_rss_config will follow the variable length field and cause
> > > > > > > misalignment.
> > > > > > > 
> > > > > > > If we let the inner header hash reuse the virtio_net_hash_config structure, it
> > > > > > > can work, but the only disadvantage is that the configuration of the inner
> > > > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > > > Just imagine:
> > > > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > > > then if we only want to configure the inner header hash (such as
> > > > > > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> > > > > > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > > > virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> > > > > > > enabled_tunnel_types due to misalignment.
> > > > > > > 
> > > > > > > So, I think the following structure will make it clearer to configure inner header
> > > > > > > hash and RSS/hash calculation.
> > > > > > > But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> > > > > > > am ok, no objection:
> > > > > > > 
> > > > > > > 1. The supported_tunnel_types are placed in the device config space;
> > > > > > > 
> > > > > > Yes. I forgot the variable length part.
> > > > > > The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
> > > > Or it could be an advantage since one normally wants to configure a
> > > > symmetric key with this. Further device can just use the new config
> > > When we want to configure the hash key, he continues to use the previous
> > > rss/hash calculation interface. This is ok.
> > > 
> > > Thanks.
> > I don't understand this sentence. My point is simply that
> > to use the tunnel key has to be symmetric. So two commands
> > will be required: one to set tunnel types, one to
> > set the key.
> 
> Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET and
> the other
> should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> 
> Thanks.

Well that is still 1 command for tunnels that affects both rss and
hashing. Why not 2 commands for rss and for hashing?


> > 
> > > > with no need to check what the old one was. I'd call it a wash.
> > > > 
> > > > > > Given these two disadvantages, I also prefer independent SET command the way you have it.
> > > > > OK, let's wait for Michael's input again.
> > > > > 
> > > > > Thanks.
> > > > This part is not critical to me, but now I understand we need two sets of SET commands.
> > > > 
> > > > 
> > > > > > > 2.
> > > > > > > Reserve the following structure:
> > > > > > > 
> > > > > > >          struct virtnet_hash_tunnel {
> > > > > > > le32 enabled_tunnel_types;
> > > > > > >          };
> > > > > > > 
> > > > > > > 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> > > > > > > command for enabled_tunnel_types.
> > > > > > > 
> > > > > > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> > > > > > > 
> > > > > > > Thanks a lot!
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> > 
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> > 
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

On Fri, Jun 30, 2023 at 10:52:25AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
> > 
> > 
> > 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > > > 
> > > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > > > 
> > > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> > > > > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > > +1.
> > > > > > > > > > > 
> > > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > > > > > > own structure and commands.
> > > > > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > > > > inner header hash.
> > > > > > > > > > > 
> > > > > > > > > > > Thanks.
> > > > > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > > > > practically you said you will enable this with symmetric hash so it
> > > > > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > > > > > 
> > > > > > > > > In the v19, we have,
> > > > > > > > > 
> > > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > > So it is done along with rss, so in same struct as rss config is fine.
> > > > > > > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > > > > > > have enabled_hash_types?
> > > > > > > > Like this:
> > > > > > > > 
> > > > > > > > struct virtio_net_rss_config {
> > > > > > > >         le32 hash_types;
> > > > > > > >         le16 indirection_table_mask;
> > > > > > > >         struct rss_rq_id unclassified_queue;
> > > > > > > >         struct rss_rq_id indirection_table[indirection_table_length];
> > > > > > > >         le16 max_tx_vq;
> > > > > > > >         u8 hash_key_length;
> > > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > > };
> > > > > > > > 
> > > > > > > > struct virtio_net_hash_config {
> > > > > > > >         le32 hash_types;
> > > > > > > > -    le16 reserved[4];
> > > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > > +    le16 reserved[2];
> > > > > > > >         u8 hash_key_length;
> > > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > > };
> > > > > Oh, I forgot that rss and hash had identical structures.
> > > > > And we want to keep that I think.
> > > > > 
> > > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > > apply to both hash calculation and rss?
> > > > Yes. What I'm trying to say is that making the inner header hash and
> > > > RSS/hash calculation clear delimitation will make everything easier.
> > > > The inner header hash just configures enabled_tunnel_types.
> > > > The RSS/hash calculation is configured as before without modification.
> > > > 
> > > > > I note we normally have separate configs for hash and rss.
> > > > > So to keep it consistent what should we do? two set commands?
> > > > As I explained above, like outer udp port hash/symmetric toeplitz hash,
> > > > these fall under the umbrella of RSS/hash calculation.
> > > Yes but note that symmetric toeplitz hash has to be configured
> > > separately for RSS and for hashing.
> > 
> > Yes, this requires a field like \field{mode}, with different values
> > corresponding to different hashing algorithms, such as toeplitz or symmetric
> > toeplitz.
> > The outer port hash belongs to RSS/hash calculation.
> 
> So there will be need for more fields.

Yes, the mode field is outside of RSS/hash, as it should be at a higher
level than RSS/hash.

> To me this implies extending with struct virtio_net_rss_tunnel_config
> is a better idea since we then have some reserved space to
> put "mode" down the road (in the reserved[6] space below).
> 
> No?
> 
> > > 
> > > > Let's keep the inner header hash simple.
> > > > 
> > > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > > Certainly. See:
> > > > 
> > > >      +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> > > > with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > 
> > > It does not really say that.
> > Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
> > applied to hash and RSS. Yes.
> > When one wants to configure inner header hash separately, use
> > VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
> > separately.
> > When one wants to configure both inner header hash and RSS/hash, use
> > VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
> > VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> > The inner header hash is decoupled from RSS/hash, and no extra fields will
> > be sent every configuration.
> 
> But why is tunnel so different? Rest of things can be configured
> for hash and for rss separately.
> 

It dawned on me where we were not aligned. Please see more.

> 
> For example I can configure hash for IPv4 and IPv6 but RSS only for
> IPv6.

Can we have this behavior?
I don't think so, RSS and Hash have the same device configuration, which
is determined by the *last* *device-received* command of the VIRTIO_NET_CTRL_MQ class.
See the spec:
"If more than one multiqueue mode is negotiated, the resulting device
configuration is defined by the last command sent by the driver."


> But for some reason, if I configure tunnel for GRE it will
> affect both hash and RSS. Why? It seems quite possible to me that I
> was offload for hash for GRE but don't want to use it
> for RSS to avoid RX underrun issues we discussed.

This makes sense. So the scenario you are referring to should be:
The configuration of RSS/HASH is IPV6, but we have configured
GRE_rfc2784 (both inner and outer headers are IPv4). When the
device receives a GRE_rfc2784 packet, which matches the GRE_rfc2784
encapsulation type, and then the device need to calculate the hash
based on RSS/Hash, but it turns out that the configured IPv6 of RSS/hash
does not match the inner IPv4. Then nothing happened.  ?

If yes, I agree to virtio_net_rss_tunnel_config and virtio_net_hash_tunnel_config.

> 
> 
> > > 
> > > 
> > > 
> > > > > We should have reserved more space. We can still do it if you like:
> > > > > 
> > > > > struct virtio_net_rss_tunnel_config {
> > > > >         le32 enabled_tunnel_types;
> > > > >         le16 reserved[6];
> > > > >         struct virtio_net_rss_config hash;
> > > > > };
> > > > > 
> > > > > struct virtio_net_hash_tunnel_config {
> > > > >         le32 enabled_tunnel_types;
> > > > >         le16 reserved[6];
> > > > >         struct virtio_net_hash_config hash;
> > > > > };
> > > > > 
> > > > > ?
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > > > If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> > > > > > > > in virtio_net_rss_config will follow the variable length field and cause
> > > > > > > > misalignment.
> > > > > > > > 
> > > > > > > > If we let the inner header hash reuse the virtio_net_hash_config structure, it
> > > > > > > > can work, but the only disadvantage is that the configuration of the inner
> > > > > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > > > > Just imagine:
> > > > > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > > > > then if we only want to configure the inner header hash (such as
> > > > > > > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> > > > > > > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > > > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > > > > virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> > > > > > > > enabled_tunnel_types due to misalignment.
> > > > > > > > 
> > > > > > > > So, I think the following structure will make it clearer to configure inner header
> > > > > > > > hash and RSS/hash calculation.
> > > > > > > > But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> > > > > > > > am ok, no objection:
> > > > > > > > 
> > > > > > > > 1. The supported_tunnel_types are placed in the device config space;
> > > > > > > > 
> > > > > > > Yes. I forgot the variable length part.
> > > > > > > The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
> > > > > Or it could be an advantage since one normally wants to configure a
> > > > > symmetric key with this. Further device can just use the new config
> > > > When we want to configure the hash key, he continues to use the previous
> > > > rss/hash calculation interface. This is ok.
> > > > 
> > > > Thanks.
> > > I don't understand this sentence. My point is simply that
> > > to use the tunnel key has to be symmetric. So two commands
> > > will be required: one to set tunnel types, one to
> > > set the key.
> > 
> > Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET and
> > the other
> > should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> > 
> > Thanks.
> 
> Well that is still 1 command for tunnels that affects both rss and
> hashing. Why not 2 commands for rss and for hashing?
> 

I think I get you now.

If the above is correct, then I think we should have the following:
VIRTIO_NET_CTRL_TUNNEL_RSS -> virtio_net_rss_tunnel_config
VIRTIO_NET_CTRL_TUNNEL_HASH -> virtio_net_hash_tunnel_config

?

Thanks!

> 
> > > 
> > > > > with no need to check what the old one was. I'd call it a wash.
> > > > > 
> > > > > > > Given these two disadvantages, I also prefer independent SET command the way you have it.
> > > > > > OK, let's wait for Michael's input again.
> > > > > > 
> > > > > > Thanks.
> > > > > This part is not critical to me, but now I understand we need two sets of SET commands.
> > > > > 
> > > > > 
> > > > > > > > 2.
> > > > > > > > Reserve the following structure:
> > > > > > > > 
> > > > > > > >          struct virtnet_hash_tunnel {
> > > > > > > > le32 enabled_tunnel_types;
> > > > > > > >          };
> > > > > > > > 
> > > > > > > > 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> > > > > > > > command for enabled_tunnel_types.
> > > > > > > > 
> > > > > > > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> > > > > > > > 
> > > > > > > > Thanks a lot!
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > 
> > > This publicly archived list offers a means to provide input to the
> > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > 
> > > In order to verify user consent to the Feedback License terms and
> > > to minimize spam in the list archive, subscription is required
> > > before posting.
> > > 
> > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > List help: virtio-comment-help@lists.oasis-open.org
> > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > Committee: https://www.oasis-open.org/committees/virtio/
> > > Join OASIS: https://www.oasis-open.org/join/

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Sat, Jul 01, 2023 at 12:09:09AM +0800, Heng Qi wrote:
> On Fri, Jun 30, 2023 at 10:52:25AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
> > > 
> > > 
> > > 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > > > > 
> > > > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > > > > 
> > > > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
> > > > > > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > > > +1.
> > > > > > > > > > > > 
> > > > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have its
> > > > > > > > > > > > own structure and commands.
> > > > > > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > > > > > inner header hash.
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks.
> > > > > > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > > > > > practically you said you will enable this with symmetric hash so it
> > > > > > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > > > > > > 
> > > > > > > > > > In the v19, we have,
> > > > > > > > > > 
> > > > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > > > So it is done along with rss, so in same struct as rss config is fine.
> > > > > > > > > Do you mean having both virtio_net_rss_config and virtio_net_hash_config
> > > > > > > > > have enabled_hash_types?
> > > > > > > > > Like this:
> > > > > > > > > 
> > > > > > > > > struct virtio_net_rss_config {
> > > > > > > > >         le32 hash_types;
> > > > > > > > >         le16 indirection_table_mask;
> > > > > > > > >         struct rss_rq_id unclassified_queue;
> > > > > > > > >         struct rss_rq_id indirection_table[indirection_table_length];
> > > > > > > > >         le16 max_tx_vq;
> > > > > > > > >         u8 hash_key_length;
> > > > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > struct virtio_net_hash_config {
> > > > > > > > >         le32 hash_types;
> > > > > > > > > -    le16 reserved[4];
> > > > > > > > > +    le32 enabled_tunnel_types;
> > > > > > > > > +    le16 reserved[2];
> > > > > > > > >         u8 hash_key_length;
> > > > > > > > >         u8 hash_key_data[hash_key_length];
> > > > > > > > > };
> > > > > > Oh, I forgot that rss and hash had identical structures.
> > > > > > And we want to keep that I think.
> > > > > > 
> > > > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > > > apply to both hash calculation and rss?
> > > > > Yes. What I'm trying to say is that making the inner header hash and
> > > > > RSS/hash calculation clear delimitation will make everything easier.
> > > > > The inner header hash just configures enabled_tunnel_types.
> > > > > The RSS/hash calculation is configured as before without modification.
> > > > > 
> > > > > > I note we normally have separate configs for hash and rss.
> > > > > > So to keep it consistent what should we do? two set commands?
> > > > > As I explained above, like outer udp port hash/symmetric toeplitz hash,
> > > > > these fall under the umbrella of RSS/hash calculation.
> > > > Yes but note that symmetric toeplitz hash has to be configured
> > > > separately for RSS and for hashing.
> > > 
> > > Yes, this requires a field like \field{mode}, with different values
> > > corresponding to different hashing algorithms, such as toeplitz or symmetric
> > > toeplitz.
> > > The outer port hash belongs to RSS/hash calculation.
> > 
> > So there will be need for more fields.
> 
> Yes, the mode field is outside of RSS/hash, as it should be at a higher
> level than RSS/hash.
> 
> > To me this implies extending with struct virtio_net_rss_tunnel_config
> > is a better idea since we then have some reserved space to
> > put "mode" down the road (in the reserved[6] space below).
> > 
> > No?
> > 
> > > > 
> > > > > Let's keep the inner header hash simple.
> > > > > 
> > > > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > > > Certainly. See:
> > > > > 
> > > > >      +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> > > > > with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > 
> > > > It does not really say that.
> > > Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
> > > applied to hash and RSS. Yes.
> > > When one wants to configure inner header hash separately, use
> > > VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
> > > separately.
> > > When one wants to configure both inner header hash and RSS/hash, use
> > > VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
> > > VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> > > The inner header hash is decoupled from RSS/hash, and no extra fields will
> > > be sent every configuration.
> > 
> > But why is tunnel so different? Rest of things can be configured
> > for hash and for rss separately.
> > 
> 
> It dawned on me where we were not aligned. Please see more.
> 
> > 
> > For example I can configure hash for IPv4 and IPv6 but RSS only for
> > IPv6.
> 
> Can we have this behavior?
> I don't think so, RSS and Hash have the same device configuration, which
> is determined by the *last* *device-received* command of the VIRTIO_NET_CTRL_MQ class.
> See the spec:
> "If more than one multiqueue mode is negotiated, the resulting device
> configuration is defined by the last command sent by the driver."

Oh I was confused. You are right, also this:

If the feature VIRTIO_NET_F_RSS was negotiated:
\begin{itemize}
\item The device uses \field{hash_types} of the virtio_net_rss_config structure as 'Enabled hash types' bitmask.
\item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_rss_config structure (see
\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / Setting RSS parameters}).
\end{itemize}
        
If the feature VIRTIO_NET_F_RSS was not negotiated:
\begin{itemize}
\item The device uses \field{hash_types} of the virtio_net_hash_config structure as 'Enabled hash types' bitmask.
\item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_hash_config structure (see
\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}).
\end{itemize}

Sorry about making this noise. I thought I saw a bug.

> > But for some reason, if I configure tunnel for GRE it will
> > affect both hash and RSS. Why? It seems quite possible to me that I
> > was offload for hash for GRE but don't want to use it
> > for RSS to avoid RX underrun issues we discussed.
> 
> This makes sense. So the scenario you are referring to should be:
> The configuration of RSS/HASH is IPV6, but we have configured
> GRE_rfc2784 (both inner and outer headers are IPv4). When the
> device receives a GRE_rfc2784 packet, which matches the GRE_rfc2784
> encapsulation type, and then the device need to calculate the hash
> based on RSS/Hash, but it turns out that the configured IPv6 of RSS/hash
> does not match the inner IPv4. Then nothing happened.  ?

That would be my understanding too.


> If yes, I agree to virtio_net_rss_tunnel_config and virtio_net_hash_tunnel_config.

Or even name them virtio_net_rss_ext_config and virtio_net_hash_ext_config
as we expect to use them down the road for other stuff.


But since you have corrected my mistake above, a separate SET is ok too.
And this is what was reviewed in the last 5 revisions or so. You decide.

> > 
> > 
> > > > 
> > > > 
> > > > 
> > > > > > We should have reserved more space. We can still do it if you like:
> > > > > > 
> > > > > > struct virtio_net_rss_tunnel_config {
> > > > > >         le32 enabled_tunnel_types;
> > > > > >         le16 reserved[6];
> > > > > >         struct virtio_net_rss_config hash;
> > > > > > };
> > > > > > 
> > > > > > struct virtio_net_hash_tunnel_config {
> > > > > >         le32 enabled_tunnel_types;
> > > > > >         le16 reserved[6];
> > > > > >         struct virtio_net_hash_config hash;
> > > > > > };
> > > > > > 
> > > > > > ?
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > > > If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
> > > > > > > > > in virtio_net_rss_config will follow the variable length field and cause
> > > > > > > > > misalignment.
> > > > > > > > > 
> > > > > > > > > If we let the inner header hash reuse the virtio_net_hash_config structure, it
> > > > > > > > > can work, but the only disadvantage is that the configuration of the inner
> > > > > > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > > > > > Just imagine:
> > > > > > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > > > > > then if we only want to configure the inner header hash (such as
> > > > > > > > > enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
> > > > > > > > > 2. but then if we want to configure the inner header hash and RSS (such as
> > > > > > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > > > > > virtio_net_hash_config once, because virtio_net_rss_config now does not carry
> > > > > > > > > enabled_tunnel_types due to misalignment.
> > > > > > > > > 
> > > > > > > > > So, I think the following structure will make it clearer to configure inner header
> > > > > > > > > hash and RSS/hash calculation.
> > > > > > > > > But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
> > > > > > > > > am ok, no objection:
> > > > > > > > > 
> > > > > > > > > 1. The supported_tunnel_types are placed in the device config space;
> > > > > > > > > 
> > > > > > > > Yes. I forgot the variable length part.
> > > > > > > > The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
> > > > > > Or it could be an advantage since one normally wants to configure a
> > > > > > symmetric key with this. Further device can just use the new config
> > > > > When we want to configure the hash key, he continues to use the previous
> > > > > rss/hash calculation interface. This is ok.
> > > > > 
> > > > > Thanks.
> > > > I don't understand this sentence. My point is simply that
> > > > to use the tunnel key has to be symmetric. So two commands
> > > > will be required: one to set tunnel types, one to
> > > > set the key.
> > > 
> > > Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET and
> > > the other
> > > should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> > > 
> > > Thanks.
> > 
> > Well that is still 1 command for tunnels that affects both rss and
> > hashing. Why not 2 commands for rss and for hashing?
> > 
> 
> I think I get you now.
> 
> If the above is correct, then I think we should have the following:
> VIRTIO_NET_CTRL_TUNNEL_RSS -> virtio_net_rss_tunnel_config
> VIRTIO_NET_CTRL_TUNNEL_HASH -> virtio_net_hash_tunnel_config
> 
> ?
> 
> Thanks!
> 
> > 
> > > > 
> > > > > > with no need to check what the old one was. I'd call it a wash.
> > > > > > 
> > > > > > > > Given these two disadvantages, I also prefer independent SET command the way you have it.
> > > > > > > OK, let's wait for Michael's input again.
> > > > > > > 
> > > > > > > Thanks.
> > > > > > This part is not critical to me, but now I understand we need two sets of SET commands.
> > > > > > 
> > > > > > 
> > > > > > > > > 2.
> > > > > > > > > Reserve the following structure:
> > > > > > > > > 
> > > > > > > > >          struct virtnet_hash_tunnel {
> > > > > > > > > le32 enabled_tunnel_types;
> > > > > > > > >          };
> > > > > > > > > 
> > > > > > > > > 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> > > > > > > > > command for enabled_tunnel_types.
> > > > > > > > > 
> > > > > > > > > [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
> > > > > > > > > 
> > > > > > > > > Thanks a lot!
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > 
> > > > This publicly archived list offers a means to provide input to the
> > > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > > 
> > > > In order to verify user consent to the Feedback License terms and
> > > > to minimize spam in the list archive, subscription is required
> > > > before posting.
> > > > 
> > > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > > List help: virtio-comment-help@lists.oasis-open.org
> > > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > > Committee: https://www.oasis-open.org/committees/virtio/
> > > > Join OASIS: https://www.oasis-open.org/join/


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

在 2023/7/1 上午12:56, Michael S. Tsirkin 写道:
> On Sat, Jul 01, 2023 at 12:09:09AM +0800, Heng Qi wrote:
>> On Fri, Jun 30, 2023 at 10:52:25AM -0400, Michael S. Tsirkin wrote:
>>> On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
>>>>
>>>> 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
>>>>> On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
>>>>>> 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
>>>>>>> On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
>>>>>>>> 在 2023/6/30 上午9:36, Parav Pandit 写道:
>>>>>>>>>> From: Heng Qi <hengqi@linux.alibaba.com>
>>>>>>>>>> Sent: Thursday, June 29, 2023 8:54 PM
>>>>>>>>>>
>>>>>>>>>> On Thu, Jun 29, 2023 at 04:59:28PM +0000, Parav Pandit wrote:
>>>>>>>>>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>>>>>> Sent: Thursday, June 29, 2023 7:48 AM
>>>>>>>>>>>>>> struct virtio_net_hash_config reserved is fine.
>>>>>>>>>>>>> +1.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Inner header hash is orthogonal to RSS, and it's fine to have its
>>>>>>>>>>>>> own structure and commands.
>>>>>>>>>>>>> There is no need to send additional RSS fields when we configure
>>>>>>>>>>>>> inner header hash.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>> Not RSS, hash calculations. It's not critical, but I note that
>>>>>>>>>>>> practically you said you will enable this with symmetric hash so it
>>>>>>>>>>>> makes sense to me to send this in the same command with the key.
>>>>>>>>>>>>
>>>>>>>>>>> In the v19, we have,
>>>>>>>>>>>
>>>>>>>>>>> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
>>>>>>>>>> along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>>>>>>>>>>> So it is done along with rss, so in same struct as rss config is fine.
>>>>>>>>>> Do you mean having both virtio_net_rss_config and virtio_net_hash_config
>>>>>>>>>> have enabled_hash_types?
>>>>>>>>>> Like this:
>>>>>>>>>>
>>>>>>>>>> struct virtio_net_rss_config {
>>>>>>>>>>          le32 hash_types;
>>>>>>>>>>          le16 indirection_table_mask;
>>>>>>>>>>          struct rss_rq_id unclassified_queue;
>>>>>>>>>>          struct rss_rq_id indirection_table[indirection_table_length];
>>>>>>>>>>          le16 max_tx_vq;
>>>>>>>>>>          u8 hash_key_length;
>>>>>>>>>>          u8 hash_key_data[hash_key_length];
>>>>>>>>>> +    le32 enabled_tunnel_types;
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> struct virtio_net_hash_config {
>>>>>>>>>>          le32 hash_types;
>>>>>>>>>> -    le16 reserved[4];
>>>>>>>>>> +    le32 enabled_tunnel_types;
>>>>>>>>>> +    le16 reserved[2];
>>>>>>>>>>          u8 hash_key_length;
>>>>>>>>>>          u8 hash_key_data[hash_key_length];
>>>>>>>>>> };
>>>>>>> Oh, I forgot that rss and hash had identical structures.
>>>>>>> And we want to keep that I think.
>>>>>>>
>>>>>>> But now it's not clear to me: does the same enabled_tunnel_types
>>>>>>> apply to both hash calculation and rss?
>>>>>> Yes. What I'm trying to say is that making the inner header hash and
>>>>>> RSS/hash calculation clear delimitation will make everything easier.
>>>>>> The inner header hash just configures enabled_tunnel_types.
>>>>>> The RSS/hash calculation is configured as before without modification.
>>>>>>
>>>>>>> I note we normally have separate configs for hash and rss.
>>>>>>> So to keep it consistent what should we do? two set commands?
>>>>>> As I explained above, like outer udp port hash/symmetric toeplitz hash,
>>>>>> these fall under the umbrella of RSS/hash calculation.
>>>>> Yes but note that symmetric toeplitz hash has to be configured
>>>>> separately for RSS and for hashing.
>>>> Yes, this requires a field like \field{mode}, with different values
>>>> corresponding to different hashing algorithms, such as toeplitz or symmetric
>>>> toeplitz.
>>>> The outer port hash belongs to RSS/hash calculation.
>>> So there will be need for more fields.
>> Yes, the mode field is outside of RSS/hash, as it should be at a higher
>> level than RSS/hash.
>>
>>> To me this implies extending with struct virtio_net_rss_tunnel_config
>>> is a better idea since we then have some reserved space to
>>> put "mode" down the road (in the reserved[6] space below).
>>>
>>> No?
>>>
>>>>>> Let's keep the inner header hash simple.
>>>>>>
>>>>>>> Or does enabled_tunnel_types apply to both rss and hash?
>>>>>> Certainly. See:
>>>>>>
>>>>>>       +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
>>>>>> with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>>>>> It does not really say that.
>>>> Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
>>>> applied to hash and RSS. Yes.
>>>> When one wants to configure inner header hash separately, use
>>>> VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
>>>> separately.
>>>> When one wants to configure both inner header hash and RSS/hash, use
>>>> VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
>>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
>>>> The inner header hash is decoupled from RSS/hash, and no extra fields will
>>>> be sent every configuration.
>>> But why is tunnel so different? Rest of things can be configured
>>> for hash and for rss separately.
>>>
>> It dawned on me where we were not aligned. Please see more.
>>
>>> For example I can configure hash for IPv4 and IPv6 but RSS only for
>>> IPv6.
>> Can we have this behavior?
>> I don't think so, RSS and Hash have the same device configuration, which
>> is determined by the *last* *device-received* command of the VIRTIO_NET_CTRL_MQ class.
>> See the spec:
>> "If more than one multiqueue mode is negotiated, the resulting device
>> configuration is defined by the last command sent by the driver."
> Oh I was confused. You are right, also this:
>
> If the feature VIRTIO_NET_F_RSS was negotiated:
> \begin{itemize}
> \item The device uses \field{hash_types} of the virtio_net_rss_config structure as 'Enabled hash types' bitmask.
> \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_rss_config structure (see
> \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / Setting RSS parameters}).
> \end{itemize}
>          
> If the feature VIRTIO_NET_F_RSS was not negotiated:
> \begin{itemize}
> \item The device uses \field{hash_types} of the virtio_net_hash_config structure as 'Enabled hash types' bitmask.
> \item The device uses a key as defined in \field{hash_key_data} and \field{hash_key_length} of the virtio_net_hash_config structure (see
> \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Hash calculation}).
> \end{itemize}
>
> Sorry about making this noise. I thought I saw a bug.
>
>>> But for some reason, if I configure tunnel for GRE it will
>>> affect both hash and RSS. Why? It seems quite possible to me that I
>>> was offload for hash for GRE but don't want to use it
>>> for RSS to avoid RX underrun issues we discussed.
>> This makes sense. So the scenario you are referring to should be:
>> The configuration of RSS/HASH is IPV6, but we have configured
>> GRE_rfc2784 (both inner and outer headers are IPv4). When the
>> device receives a GRE_rfc2784 packet, which matches the GRE_rfc2784
>> encapsulation type, and then the device need to calculate the hash
>> based on RSS/Hash, but it turns out that the configured IPv6 of RSS/hash
>> does not match the inner IPv4. Then nothing happened.  ?
> That would be my understanding too.
>
>
>> If yes, I agree to virtio_net_rss_tunnel_config and virtio_net_hash_tunnel_config.
> Or even name them virtio_net_rss_ext_config and virtio_net_hash_ext_config
> as we expect to use them down the road for other stuff.
>
>
> But since you have corrected my mistake above, a separate SET is ok too.
> And this is what was reviewed in the last 5 revisions or so. You decide.

OK, I'm going to do this(a separate SET version). A new version will be 
released over the weekend.

Thanks.

>
>>>
>>>>>
>>>>>
>>>>>>> We should have reserved more space. We can still do it if you like:
>>>>>>>
>>>>>>> struct virtio_net_rss_tunnel_config {
>>>>>>>          le32 enabled_tunnel_types;
>>>>>>>          le16 reserved[6];
>>>>>>>          struct virtio_net_rss_config hash;
>>>>>>> };
>>>>>>>
>>>>>>> struct virtio_net_hash_tunnel_config {
>>>>>>>          le32 enabled_tunnel_types;
>>>>>>>          le16 reserved[6];
>>>>>>>          struct virtio_net_hash_config hash;
>>>>>>> };
>>>>>>>
>>>>>>> ?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>>> If yes, this should have been discussed in v10 [1] before, enabled_tunnel_types
>>>>>>>>>> in virtio_net_rss_config will follow the variable length field and cause
>>>>>>>>>> misalignment.
>>>>>>>>>>
>>>>>>>>>> If we let the inner header hash reuse the virtio_net_hash_config structure, it
>>>>>>>>>> can work, but the only disadvantage is that the configuration of the inner
>>>>>>>>>> header hash and *RSS*(not hash calculations) becomes somewhat coupled.
>>>>>>>>>> Just imagine:
>>>>>>>>>> If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
>>>>>>>>>> VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
>>>>>>>>>> then if we only want to configure the inner header hash (such as
>>>>>>>>>> enabled_tunnel_types), it is good for us to send virtio_net_hash_config alone;
>>>>>>>>>> 2. but then if we want to configure the inner header hash and RSS (such as
>>>>>>>>>> indirection table), we need to send all virtio_net_rss_config and
>>>>>>>>>> virtio_net_hash_config once, because virtio_net_rss_config now does not carry
>>>>>>>>>> enabled_tunnel_types due to misalignment.
>>>>>>>>>>
>>>>>>>>>> So, I think the following structure will make it clearer to configure inner header
>>>>>>>>>> hash and RSS/hash calculation.
>>>>>>>>>> But in any case, if we still propose to reuse virtio_net_hash_config proposal, I
>>>>>>>>>> am ok, no objection:
>>>>>>>>>>
>>>>>>>>>> 1. The supported_tunnel_types are placed in the device config space;
>>>>>>>>>>
>>>>>>>>> Yes. I forgot the variable length part.
>>>>>>>>> The second disadvantage I remember now is one need to resupply all the rss hash config parameter and device needs to compare and modify for this one field.
>>>>>>> Or it could be an advantage since one normally wants to configure a
>>>>>>> symmetric key with this. Further device can just use the new config
>>>>>> When we want to configure the hash key, he continues to use the previous
>>>>>> rss/hash calculation interface. This is ok.
>>>>>>
>>>>>> Thanks.
>>>>> I don't understand this sentence. My point is simply that
>>>>> to use the tunnel key has to be symmetric. So two commands
>>>>> will be required: one to set tunnel types, one to
>>>>> set the key.
>>>> Yes, you are right. Then one should be VIRTIO_NET_CTRL_HASH_TUNNEL_SET and
>>>> the other
>>>> should be VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
>>>>
>>>> Thanks.
>>> Well that is still 1 command for tunnels that affects both rss and
>>> hashing. Why not 2 commands for rss and for hashing?
>>>
>> I think I get you now.
>>
>> If the above is correct, then I think we should have the following:
>> VIRTIO_NET_CTRL_TUNNEL_RSS -> virtio_net_rss_tunnel_config
>> VIRTIO_NET_CTRL_TUNNEL_HASH -> virtio_net_hash_tunnel_config
>>
>> ?
>>
>> Thanks!
>>
>>>>>>> with no need to check what the old one was. I'd call it a wash.
>>>>>>>
>>>>>>>>> Given these two disadvantages, I also prefer independent SET command the way you have it.
>>>>>>>> OK, let's wait for Michael's input again.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>> This part is not critical to me, but now I understand we need two sets of SET commands.
>>>>>>>
>>>>>>>
>>>>>>>>>> 2.
>>>>>>>>>> Reserve the following structure:
>>>>>>>>>>
>>>>>>>>>>           struct virtnet_hash_tunnel {
>>>>>>>>>> le32 enabled_tunnel_types;
>>>>>>>>>>           };
>>>>>>>>>>
>>>>>>>>>> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
>>>>>>>>>> command for enabled_tunnel_types.
>>>>>>>>>>
>>>>>>>>>> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00317.html
>>>>>>>>>>
>>>>>>>>>> Thanks a lot!
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>> This publicly archived list offers a means to provide input to the
>>>>> OASIS Virtual I/O Device (VIRTIO) TC.
>>>>>
>>>>> In order to verify user consent to the Feedback License terms and
>>>>> to minimize spam in the list archive, subscription is required
>>>>> before posting.
>>>>>
>>>>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>>>>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>>>>> List help: virtio-comment-help@lists.oasis-open.org
>>>>> List archive: https://lists.oasis-open.org/archives/virtio-comment/
>>>>> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
>>>>> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
>>>>> Committee: https://www.oasis-open.org/committees/virtio/
>>>>> Join OASIS: https://www.oasis-open.org/join/


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 01:56:34AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > Maybe I get it. You want to use the new features as a carrot to
> > > > force drivers to implement DMA? You suspect they will ignore the
> > > > spec requirement just because things seem to work?
> > > >
> > > Right because it is not a must normative.
> > 
> > Well SHOULD also does not mean "ok to just ignore".
> > 
> > 	This word, or the adjective "RECOMMENDED", mean that there
> > 	   may exist valid reasons in particular circumstances to ignore a
> > 	   particular item, but the full implications must be understood and
> > 	   carefully weighed before choosing a different course.
> >
> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> So rather a good design is device tells the starting offset for the extended config space.
> And extended config space MUST be accessed using a DMA.
> With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
> This also gives the ability to maintain current config as MMIO for backward compatibility.

Really we do not need offsets. We have feature bits, and offsets are
implied. But let's start a separate thread for discussions of this?


> > 
> > 
> > > > There's some logic here, for sure. you just might be right.
> > > >
> > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > >
> > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > temporary one field to config space.
> > >
> > > I propose to add a line to the spec " Device Configuration Space"
> > > section, something like,
> > >
> > > Note: Any new device configuration space fields additional MUST consider
> > accessing such fields via a DMA interface.
> > >
> > > And this will guide the new patches of what to do instead of last moment
> > rush.
> > 
> > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
> > MMIO might be an option for qemu helping us debug DMA issues.
> >
> There are too many queues whose debugging is needed and MMIO likely not the way to debug.
>  
> > The time to discuss this detail would be around when proposal for the DMA
> > access to config space is on list though: I feel this SHOULD vs MUST is a small
> > enough detail.
> >
> >From implementation POV it is certainly critical and good step forward to optimize virtio interface.
>  
> > Going back to inner hash. If we move supported_tunnels back to config space,
> > do you feel we still need GET or just drop it? I note we do not have GET for
> > either hash or rss config.
> >
> For hash and rss config, debugging is missing. :)
> Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
>  
> > And if we no longer have GET is there still a reason for a separate command as
> > opposed to a field in virtio_net_hash_config?
> > I know this was done in v11 but there it was misaligned.
> > We went with a command because we needed it for supported_tunnels but
> > now that is no longer the case and there are reserved words in
> > virtio_net_hash_config ...
> > 
> > Let me know how you feel it about that, not critical for me.
> 
> struct virtio_net_hash_config reserved is fine.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Heng Qi]

On Thu, Jun 29, 2023 at 01:56:34AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > Maybe I get it. You want to use the new features as a carrot to
> > > > force drivers to implement DMA? You suspect they will ignore the
> > > > spec requirement just because things seem to work?
> > > >
> > > Right because it is not a must normative.
> > 
> > Well SHOULD also does not mean "ok to just ignore".
> > 
> > 	This word, or the adjective "RECOMMENDED", mean that there
> > 	   may exist valid reasons in particular circumstances to ignore a
> > 	   particular item, but the full implications must be understood and
> > 	   carefully weighed before choosing a different course.
> >
> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> So rather a good design is device tells the starting offset for the extended config space.
> And extended config space MUST be accessed using a DMA.
> With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
> This also gives the ability to maintain current config as MMIO for backward compatibility.
>  
> > 
> > 
> > > > There's some logic here, for sure. you just might be right.
> > > >
> > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > >
> > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > temporary one field to config space.
> > >
> > > I propose to add a line to the spec " Device Configuration Space"
> > > section, something like,
> > >
> > > Note: Any new device configuration space fields additional MUST consider
> > accessing such fields via a DMA interface.
> > >
> > > And this will guide the new patches of what to do instead of last moment
> > rush.
> > 
> > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
> > MMIO might be an option for qemu helping us debug DMA issues.
> >
> There are too many queues whose debugging is needed and MMIO likely not the way to debug.
>  
> > The time to discuss this detail would be around when proposal for the DMA
> > access to config space is on list though: I feel this SHOULD vs MUST is a small
> > enough detail.
> >
> From implementation POV it is certainly critical and good step forward to optimize virtio interface.
>  
> > Going back to inner hash. If we move supported_tunnels back to config space,
> > do you feel we still need GET or just drop it? I note we do not have GET for
> > either hash or rss config.
> >
> For hash and rss config, debugging is missing. :)
> Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
>  

I would like to make sure if we're aligned. The new version should contain the following:
1. The supported_tunnel_types are placed in the device config space;
2. Reserve the following structure:

     struct virtnet_hash_tunnel {
		le32 enabled_tunnel_types;
     };

3. Reserve the SET command for enabled_tunnel_types and remove the GET command for enabled_tunnel_types.

If there is no problem, I will modify it accordingly.

Thanks!

> > And if we no longer have GET is there still a reason for a separate command as
> > opposed to a field in virtio_net_hash_config?
> > I know this was done in v11 but there it was misaligned.
> > We went with a command because we needed it for supported_tunnels but
> > now that is no longer the case and there are reserved words in
> > virtio_net_hash_config ...
> > 
> > Let me know how you feel it about that, not critical for me.
> 
> struct virtio_net_hash_config reserved is fine.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Thursday, June 29, 2023 2:41 AM

> I would like to make sure if we're aligned. The new version should contain the
> following:
> 1. The supported_tunnel_types are placed in the device config space; 2.
> Reserve the following structure:
> 
>      struct virtnet_hash_tunnel {
> 		le32 enabled_tunnel_types;
>      };
> 
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET
> command for enabled_tunnel_types.
> 
> If there is no problem, I will modify it accordingly.

Ack.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 02:40:42PM +0800, Heng Qi wrote:
> On Thu, Jun 29, 2023 at 01:56:34AM +0000, Parav Pandit wrote:
> > 
> > 
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > > Maybe I get it. You want to use the new features as a carrot to
> > > > > force drivers to implement DMA? You suspect they will ignore the
> > > > > spec requirement just because things seem to work?
> > > > >
> > > > Right because it is not a must normative.
> > > 
> > > Well SHOULD also does not mean "ok to just ignore".
> > > 
> > > 	This word, or the adjective "RECOMMENDED", mean that there
> > > 	   may exist valid reasons in particular circumstances to ignore a
> > > 	   particular item, but the full implications must be understood and
> > > 	   carefully weighed before choosing a different course.
> > >
> > RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.
> > So rather a good design is device tells the starting offset for the extended config space.
> > And extended config space MUST be accessed using a DMA.
> > With this sw can have infinite size MMIO and hw device forces DMA based on its implementation of where to start DMA from.
> > This also gives the ability to maintain current config as MMIO for backward compatibility.
> >  
> > > 
> > > 
> > > > > There's some logic here, for sure. you just might be right.
> > > > >
> > > > > However, surely we can discuss this small tweak in 1.4 timeframe?
> > > >
> > > > Sure, if we prefer the DMA approach I don't have a problem in adding
> > > temporary one field to config space.
> > > >
> > > > I propose to add a line to the spec " Device Configuration Space"
> > > > section, something like,
> > > >
> > > > Note: Any new device configuration space fields additional MUST consider
> > > accessing such fields via a DMA interface.
> > > >
> > > > And this will guide the new patches of what to do instead of last moment
> > > rush.
> > > 
> > > Yea, except again I'd probably make it a SHOULD: e.g. I can see how switching to
> > > MMIO might be an option for qemu helping us debug DMA issues.
> > >
> > There are too many queues whose debugging is needed and MMIO likely not the way to debug.
> >  
> > > The time to discuss this detail would be around when proposal for the DMA
> > > access to config space is on list though: I feel this SHOULD vs MUST is a small
> > > enough detail.
> > >
> > From implementation POV it is certainly critical and good step forward to optimize virtio interface.
> >  
> > > Going back to inner hash. If we move supported_tunnels back to config space,
> > > do you feel we still need GET or just drop it? I note we do not have GET for
> > > either hash or rss config.
> > >
> > For hash and rss config, debugging is missing. :)
> > Yes, we can drop the GET after switching supported_tunnels to struct virtio_net_hash_config.
> >  
> 
> I would like to make sure if we're aligned. The new version should contain the following:
> 1. The supported_tunnel_types are placed in the device config space;
> 2. Reserve the following structure:
> 
>      struct virtnet_hash_tunnel {
> 		le32 enabled_tunnel_types;
>      };
> 
> 3. Reserve the SET command for enabled_tunnel_types and remove the GET command for enabled_tunnel_types.
> 
> If there is no problem, I will modify it accordingly.
> 
> Thanks!
> > > And if we no longer have GET is there still a reason for a separate command as
> > > opposed to a field in virtio_net_hash_config?
> > > I know this was done in v11 but there it was misaligned.
> > > We went with a command because we needed it for supported_tunnels but
> > > now that is no longer the case and there are reserved words in
> > > virtio_net_hash_config ...
> > > 
> > > Let me know how you feel it about that, not critical for me.
> > 
> > struct virtio_net_hash_config reserved is fine.


Just a second, when I talked about virtio_net_hash_config I meant this:


struct virtio_net_hash_config {
    le32 hash_types;
-   le16 reserved[4];
+   le32 enabled_tunnel_types;
+   le16 reserved[2];
    u8 hash_key_length;
    u8 hash_key_data[hash_key_length];
};

and then we do not add a new command we modify the hash config command.

Parav still ok with you?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, June 29, 2023 7:46 AM


> Just a second, when I talked about virtio_net_hash_config I meant this:
> 
> 
> struct virtio_net_hash_config {
>     le32 hash_types;
> -   le16 reserved[4];
> +   le32 enabled_tunnel_types;
> +   le16 reserved[2];
>     u8 hash_key_length;
>     u8 hash_key_data[hash_key_length];
> };
> 
> and then we do not add a new command we modify the hash config command.
> 
> Parav still ok with you?

yes, looks good.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 01:56:34AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, June 28, 2023 3:45 PM
> > > > Maybe I get it. You want to use the new features as a carrot to
> > > > force drivers to implement DMA? You suspect they will ignore the
> > > > spec requirement just because things seem to work?
> > > >
> > > Right because it is not a must normative.
> > 
> > Well SHOULD also does not mean "ok to just ignore".
> > 
> > 	This word, or the adjective "RECOMMENDED", mean that there
> > 	   may exist valid reasons in particular circumstances to ignore a
> > 	   particular item, but the full implications must be understood and
> > 	   carefully weighed before choosing a different course.
> >
> RECOMMENDED and SHOULD forces the device to support MMIO, which is not good.

You said this already. Let's not repeat ourselves please.

It's a single word. We need to work on a patch, we can argue about
small detail once it's posted. It's not in your list of plans so
I am guessing we need someone on Red Hat side to work on this?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Michael S. Tsirkin
> Sent: Thursday, June 29, 2023 3:07 AM

> > > Well SHOULD also does not mean "ok to just ignore".
> > >
> > > 	This word, or the adjective "RECOMMENDED", mean that there
> > > 	   may exist valid reasons in particular circumstances to ignore a
> > > 	   particular item, but the full implications must be understood and
> > > 	   carefully weighed before choosing a different course.
> > >
> > RECOMMENDED and SHOULD forces the device to support MMIO, which is
> not good.
> 
> You said this already. Let's not repeat ourselves please.
> 
> It's a single word. We need to work on a patch, we can argue about small detail
> once it's posted. It's not in your list of plans so I am guessing we need someone
> on Red Hat side to work on this?

Yes, I prefer to work together and before writing the patch.
As just DMA for config space is only point fix problem.
So lets discuss in new thread and improve this for config space and also other limitations (like support multi-address VQ).



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 11:38:49AM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> > Behalf Of Michael S. Tsirkin
> > Sent: Thursday, June 29, 2023 3:07 AM
> 
> > > > Well SHOULD also does not mean "ok to just ignore".
> > > >
> > > > 	This word, or the adjective "RECOMMENDED", mean that there
> > > > 	   may exist valid reasons in particular circumstances to ignore a
> > > > 	   particular item, but the full implications must be understood and
> > > > 	   carefully weighed before choosing a different course.
> > > >
> > > RECOMMENDED and SHOULD forces the device to support MMIO, which is
> > not good.
> > 
> > You said this already. Let's not repeat ourselves please.
> > 
> > It's a single word. We need to work on a patch, we can argue about small detail
> > once it's posted. It's not in your list of plans so I am guessing we need someone
> > on Red Hat side to work on this?
> 
> Yes, I prefer to work together and before writing the patch.
> As just DMA for config space is only point fix problem.
> So lets discuss in new thread and improve this for config space and also other limitations (like support multi-address VQ).
> 

Interesting.  What is multi-address VQ?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> On Wed, Jun 28, 2023 at 12:35 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > 1. Currently, a received encapsulated packet has an outer and an inner header, but
> > the virtio device is unable to calculate the hash for the inner header. The same
> > flow can traverse through different tunnels, resulting in the encapsulated
> > packets being spread across multiple receive queues (refer to the figure below).
> > However, in certain scenarios, we may need to direct these encapsulated packets of
> > the same flow to a single receive queue. This facilitates the processing
> > of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
> >
> >                client1                    client2
> >                   |        +-------+         |
> >                   +------->|tunnels|<--------+
> >                            +-------+
> >                               |  |
> >                               v  v
> >                       +-----------------+
> >                       | monitoring host |
> >                       +-----------------+
> >
> > To achieve this, the device can calculate a symmetric hash based on the inner headers
> > of the same flow.
> >
> > 2. For legacy systems, they may lack entropy fields which modern protocols have in
> > the outer header, resulting in multiple flows with the same outer header but
> > different inner headers being directed to the same receive queue. This results in
> > poor receive performance.
> >
> > To address this limitation, inner header hash can be used to enable the device to advertise
> > the capability to calculate the hash for the inner packet, regaining better receive performance.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > ---
> > v18->v19:
> >         1. Have a single structure instead of two. @Michael S . Tsirkin
> >         2. Some small rewrites. @Michael S . Tsirkin
> >         3. Rebase to master.
> >
> > v17->v18:
> >         1. Some rewording suggestions from Michael (Thanks!).
> >         2. Use 0 to disable inner header hash and remove
> >            VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > v16->v17:
> >         1. Some small rewrites. @Parav Pandit
> >         2. Add Parav's Reviewed-by tag (Thanks!).
> >
> > v15->v16:
> >         1. Remove the hash_option. In order to delimit the inner header hash and RSS
> >            configuration, the ability to configure the outer src udp port hash is given
> >            to RSS. This is orthogonal to inner header hash, which will be done in the
> >            RSS capability extension topic (considered as an RSS extension together
> >            with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
> >         2. Fix a 'field' typo. @Parav Pandit
> >
> > v14->v15:
> >         1. Add tunnel hash option suggested by @Michael S . Tsirkin
> >         2. Adjust some descriptions.
> >
> > v13->v14:
> >         1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
> 
> I may miss some discussions, but this complicates the provisioning a lot.
> 
> Having it in the config space, then a type agnostic provisioning
> through config space + feature bits just works fine.
> 
> If we move it only via cvq, we need device specific provisioning interface.
> 
> Thanks

Yea that's what I said too. Debugging too.  I think we should build a
consistent solution that allows accessing config space through DMA,
separately from this effort.  Parav do you think you can live with this
approach so this specific proposal can move forward?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jason Wang]

在 2023/6/28 18:10, Michael S. Tsirkin 写道:
> On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
>> On Wed, Jun 28, 2023 at 12:35 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>> 1. Currently, a received encapsulated packet has an outer and an inner header, but
>>> the virtio device is unable to calculate the hash for the inner header. The same
>>> flow can traverse through different tunnels, resulting in the encapsulated
>>> packets being spread across multiple receive queues (refer to the figure below).
>>> However, in certain scenarios, we may need to direct these encapsulated packets of
>>> the same flow to a single receive queue. This facilitates the processing
>>> of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
>>>
>>>                 client1                    client2
>>>                    |        +-------+         |
>>>                    +------->|tunnels|<--------+
>>>                             +-------+
>>>                                |  |
>>>                                v  v
>>>                        +-----------------+
>>>                        | monitoring host |
>>>                        +-----------------+
>>>
>>> To achieve this, the device can calculate a symmetric hash based on the inner headers
>>> of the same flow.
>>>
>>> 2. For legacy systems, they may lack entropy fields which modern protocols have in
>>> the outer header, resulting in multiple flows with the same outer header but
>>> different inner headers being directed to the same receive queue. This results in
>>> poor receive performance.
>>>
>>> To address this limitation, inner header hash can be used to enable the device to advertise
>>> the capability to calculate the hash for the inner packet, regaining better receive performance.
>>>
>>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>> ---
>>> v18->v19:
>>>          1. Have a single structure instead of two. @Michael S . Tsirkin
>>>          2. Some small rewrites. @Michael S . Tsirkin
>>>          3. Rebase to master.
>>>
>>> v17->v18:
>>>          1. Some rewording suggestions from Michael (Thanks!).
>>>          2. Use 0 to disable inner header hash and remove
>>>             VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
>>> v16->v17:
>>>          1. Some small rewrites. @Parav Pandit
>>>          2. Add Parav's Reviewed-by tag (Thanks!).
>>>
>>> v15->v16:
>>>          1. Remove the hash_option. In order to delimit the inner header hash and RSS
>>>             configuration, the ability to configure the outer src udp port hash is given
>>>             to RSS. This is orthogonal to inner header hash, which will be done in the
>>>             RSS capability extension topic (considered as an RSS extension together
>>>             with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
>>>          2. Fix a 'field' typo. @Parav Pandit
>>>
>>> v14->v15:
>>>          1. Add tunnel hash option suggested by @Michael S . Tsirkin
>>>          2. Adjust some descriptions.
>>>
>>> v13->v14:
>>>          1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
>> I may miss some discussions, but this complicates the provisioning a lot.
>>
>> Having it in the config space, then a type agnostic provisioning
>> through config space + feature bits just works fine.
>>
>> If we move it only via cvq, we need device specific provisioning interface.
>>
>> Thanks
> Yea that's what I said too. Debugging too.  I think we should build a
> consistent solution that allows accessing config space through DMA,
> separately from this effort.  Parav do you think you can live with this
> approach so this specific proposal can move forward?


We can probably go another way, invent a new device configuration space 
capability which fixed size like PCI configuration access capability?

struct virtio_pci_cfg_cap {
         struct virtio_pci_cap cap;
         u8 dev_cfg_data[4]; /* Data for device configuration space 
access. */
};

So it won't grow as the size of device configuration space grows.

Thanks


>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 11:31:58AM +0800, Jason Wang wrote:
> 
> 在 2023/6/28 18:10, Michael S. Tsirkin 写道:
> > On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> > > On Wed, Jun 28, 2023 at 12:35 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > 1. Currently, a received encapsulated packet has an outer and an inner header, but
> > > > the virtio device is unable to calculate the hash for the inner header. The same
> > > > flow can traverse through different tunnels, resulting in the encapsulated
> > > > packets being spread across multiple receive queues (refer to the figure below).
> > > > However, in certain scenarios, we may need to direct these encapsulated packets of
> > > > the same flow to a single receive queue. This facilitates the processing
> > > > of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
> > > > 
> > > >                 client1                    client2
> > > >                    |        +-------+         |
> > > >                    +------->|tunnels|<--------+
> > > >                             +-------+
> > > >                                |  |
> > > >                                v  v
> > > >                        +-----------------+
> > > >                        | monitoring host |
> > > >                        +-----------------+
> > > > 
> > > > To achieve this, the device can calculate a symmetric hash based on the inner headers
> > > > of the same flow.
> > > > 
> > > > 2. For legacy systems, they may lack entropy fields which modern protocols have in
> > > > the outer header, resulting in multiple flows with the same outer header but
> > > > different inner headers being directed to the same receive queue. This results in
> > > > poor receive performance.
> > > > 
> > > > To address this limitation, inner header hash can be used to enable the device to advertise
> > > > the capability to calculate the hash for the inner packet, regaining better receive performance.
> > > > 
> > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > ---
> > > > v18->v19:
> > > >          1. Have a single structure instead of two. @Michael S . Tsirkin
> > > >          2. Some small rewrites. @Michael S . Tsirkin
> > > >          3. Rebase to master.
> > > > 
> > > > v17->v18:
> > > >          1. Some rewording suggestions from Michael (Thanks!).
> > > >          2. Use 0 to disable inner header hash and remove
> > > >             VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > > > v16->v17:
> > > >          1. Some small rewrites. @Parav Pandit
> > > >          2. Add Parav's Reviewed-by tag (Thanks!).
> > > > 
> > > > v15->v16:
> > > >          1. Remove the hash_option. In order to delimit the inner header hash and RSS
> > > >             configuration, the ability to configure the outer src udp port hash is given
> > > >             to RSS. This is orthogonal to inner header hash, which will be done in the
> > > >             RSS capability extension topic (considered as an RSS extension together
> > > >             with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
> > > >          2. Fix a 'field' typo. @Parav Pandit
> > > > 
> > > > v14->v15:
> > > >          1. Add tunnel hash option suggested by @Michael S . Tsirkin
> > > >          2. Adjust some descriptions.
> > > > 
> > > > v13->v14:
> > > >          1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
> > > I may miss some discussions, but this complicates the provisioning a lot.
> > > 
> > > Having it in the config space, then a type agnostic provisioning
> > > through config space + feature bits just works fine.
> > > 
> > > If we move it only via cvq, we need device specific provisioning interface.
> > > 
> > > Thanks
> > Yea that's what I said too. Debugging too.  I think we should build a
> > consistent solution that allows accessing config space through DMA,
> > separately from this effort.  Parav do you think you can live with this
> > approach so this specific proposal can move forward?
> 
> 
> We can probably go another way, invent a new device configuration space
> capability which fixed size like PCI configuration access capability?
> 
> struct virtio_pci_cfg_cap {
>         struct virtio_pci_cap cap;
>         u8 dev_cfg_data[4]; /* Data for device configuration space access.
> */
> };
> 
> So it won't grow as the size of device configuration space grows.
> 
> Thanks

It is true, it does not have to be DMA strictly speaking.

The basic issue is with synchronous access.

We can change the capability in some way to allow asynch then
that works. E.g. make device change length to 0 and driver
must poll before considering the operation done.

Having said that, this will increase # of VM exits even more.
Not a fan, DMA seems cleaner.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] Re: [PATCH v19] virtio-net: support inner header hash

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 07:54:29AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 29, 2023 at 11:31:58AM +0800, Jason Wang wrote:
> > 
> > 在 2023/6/28 18:10, Michael S. Tsirkin 写道:
> > > On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> > > > On Wed, Jun 28, 2023 at 12:35 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > 1. Currently, a received encapsulated packet has an outer and an inner header, but
> > > > > the virtio device is unable to calculate the hash for the inner header. The same
> > > > > flow can traverse through different tunnels, resulting in the encapsulated
> > > > > packets being spread across multiple receive queues (refer to the figure below).
> > > > > However, in certain scenarios, we may need to direct these encapsulated packets of
> > > > > the same flow to a single receive queue. This facilitates the processing
> > > > > of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
> > > > > 
> > > > >                 client1                    client2
> > > > >                    |        +-------+         |
> > > > >                    +------->|tunnels|<--------+
> > > > >                             +-------+
> > > > >                                |  |
> > > > >                                v  v
> > > > >                        +-----------------+
> > > > >                        | monitoring host |
> > > > >                        +-----------------+
> > > > > 
> > > > > To achieve this, the device can calculate a symmetric hash based on the inner headers
> > > > > of the same flow.
> > > > > 
> > > > > 2. For legacy systems, they may lack entropy fields which modern protocols have in
> > > > > the outer header, resulting in multiple flows with the same outer header but
> > > > > different inner headers being directed to the same receive queue. This results in
> > > > > poor receive performance.
> > > > > 
> > > > > To address this limitation, inner header hash can be used to enable the device to advertise
> > > > > the capability to calculate the hash for the inner packet, regaining better receive performance.
> > > > > 
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > ---
> > > > > v18->v19:
> > > > >          1. Have a single structure instead of two. @Michael S . Tsirkin
> > > > >          2. Some small rewrites. @Michael S . Tsirkin
> > > > >          3. Rebase to master.
> > > > > 
> > > > > v17->v18:
> > > > >          1. Some rewording suggestions from Michael (Thanks!).
> > > > >          2. Use 0 to disable inner header hash and remove
> > > > >             VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > > > > v16->v17:
> > > > >          1. Some small rewrites. @Parav Pandit
> > > > >          2. Add Parav's Reviewed-by tag (Thanks!).
> > > > > 
> > > > > v15->v16:
> > > > >          1. Remove the hash_option. In order to delimit the inner header hash and RSS
> > > > >             configuration, the ability to configure the outer src udp port hash is given
> > > > >             to RSS. This is orthogonal to inner header hash, which will be done in the
> > > > >             RSS capability extension topic (considered as an RSS extension together
> > > > >             with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
> > > > >          2. Fix a 'field' typo. @Parav Pandit
> > > > > 
> > > > > v14->v15:
> > > > >          1. Add tunnel hash option suggested by @Michael S . Tsirkin
> > > > >          2. Adjust some descriptions.
> > > > > 
> > > > > v13->v14:
> > > > >          1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
> > > > I may miss some discussions, but this complicates the provisioning a lot.
> > > > 
> > > > Having it in the config space, then a type agnostic provisioning
> > > > through config space + feature bits just works fine.
> > > > 
> > > > If we move it only via cvq, we need device specific provisioning interface.
> > > > 
> > > > Thanks
> > > Yea that's what I said too. Debugging too.  I think we should build a
> > > consistent solution that allows accessing config space through DMA,
> > > separately from this effort.  Parav do you think you can live with this
> > > approach so this specific proposal can move forward?
> > 
> > 
> > We can probably go another way, invent a new device configuration space
> > capability which fixed size like PCI configuration access capability?
> > 
> > struct virtio_pci_cfg_cap {
> >         struct virtio_pci_cap cap;
> >         u8 dev_cfg_data[4]; /* Data for device configuration space access.
> > */
> > };
> > 
> > So it won't grow as the size of device configuration space grows.
> > 
> > Thanks
> 
> It is true, it does not have to be DMA strictly speaking.
> 
> The basic issue is with synchronous access.
> 
> We can change the capability in some way to allow asynch then
> that works. E.g. make device change length to 0 and driver
> must poll before considering the operation done.
> 
> Having said that, this will increase # of VM exits even more.
> Not a fan, DMA seems cleaner.

And again, I am now guilty of this too - hijacking inner hash thread
for config space change discussions :(
Let's all stop please, start a new thread wen we are ready.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org