public inbox for virtio-comment@lists.linux.dev
 help / color / mirror / Atom feed
* [PATCH v2] virtio-net: Introduce a new field to indicate outer network header offset
@ 2025-01-28 14:21 Kommula Shiva Shankar
  2025-02-12  5:54 ` Parav Pandit
  0 siblings, 1 reply; 2+ messages in thread
From: Kommula Shiva Shankar @ 2025-01-28 14:21 UTC (permalink / raw)
  To: pabeni, willemb, jasowang, parav, virtio-comment
  Cc: ndabilpuram, jerinj, rbhansali

This patch introduces a new field in the virtio_net_header called out_nh_offset, along with a new net device feature, VIRTIO_NET_F_OUT_NET_HEADER.

Currently, there is no field available to directly read the outer network header offset in case of segmentation offload.
This requires reading packet data, which significantly affects performance in datapath.
Additionally, some hardware implementations requrie knowledege of the outer L3 offset (aka L2 length) for inline IPSec hardware acceleration.

To address this limitation, we propose splitting the csum_offset field into two 8-bit fields named csum_offset and out_nh_offset.
The csum_offset indicates the offset value from the csum_start and  may not exceed 256B bits(2^8) for protocols that use a 16-bit one's complement checksum

Following table lists such protocols and their checksum offset fields within their headers

+-----+--------+
|Proto|csum_off|
+-----+--------+
| IPV4| 10B    |
| ICMP| 2B     |
| IGMP| 2B     |
|  TCP| 16B    |
|  UDP| 6B     |
+-----+--------+

The out_nh_offset represents the start offset of the outer network header from the beginning of the packet data

This issue was briefly discussed on the mailing list in a different thread, which can be found here https://lore.kernel.org/all/DM4PR18MB4269FAAC3CFC7E57E25DFBD2DF8B2@DM4PR18MB4269.namprd18.prod.outlook.com/

 v1 -> v2:
 - explicitly state that the out_nh_offset can be set only when a valid network header is present.
 - updated out_nh_offset usage in the RX direction.
 - minor word cleanup.

https://lore.kernel.org/virtio-comment/20250114171636.3175670-1-kshankar@marvell.com/

Signed-off-by: Kommula Shiva Shankar <kshankar@marvell.com>
---
 device-types/net/description.tex | 38 +++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 76585b0..529b7b9 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -88,6 +88,10 @@ \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_OUT_NET_HEADER(50)] The Driver can provide the start of \field{out_nh_offset}
+    value. The Device gains advantage by not reading packet to calculate outer network
+    header offset.
+
 \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.
@@ -418,7 +422,13 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
         le16 hdr_len;
         le16 gso_size;
         le16 csum_start;
-        le16 csum_offset;
+        union {
+                le16 csum_offset;
+                struct { (Only if VIRTIO_NET_F_OUT_NET_HEADER negotiated)
+                        le8 csum_offset;
+                        le8 out_nh_offset;
+                };
+        };
         le16 num_buffers;
         le32 hash_value;        (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
         le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
@@ -457,6 +467,11 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 \item The driver can send a completely checksummed packet.  In this case,
   \field{flags} will be zero, and \field{gso_type} will be VIRTIO_NET_HDR_GSO_NONE.
 
+\item The driver MAY optionally provide the \field{out_nh_offset} value, which is negotiated
+   using the VIRTIO_NET_F_OUT_NET_HEADER. If \field{out_nh_offset} is nonzero, it indicates
+   a valid outer network header with in the packet, and specifies the offset in bytes from
+   the beginning of the packet. Otherwise \field{out_nh_offset} MUST be set to zero.
+
 \item If the driver negotiated VIRTIO_NET_F_CSUM, it can skip
   checksumming the packet:
   \begin{itemize}
@@ -531,6 +546,11 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 \field{flags} to zero and SHOULD supply a fully checksummed
 packet to the device.
 
+If the VIRTIO_NET_F_OUT_NET_HEADER feature has been negotiated,
+the driver MAY set \field{out_nh_offset} to indicate the start of the
+outer network header offset, if the packet contains a valid network header.
+Otherwise, \field{out_nh_offset} MUST be set to zero.
+
 If VIRTIO_NET_F_HOST_TSO4 is negotiated, the driver MAY set
 \field{gso_type} to VIRTIO_NET_HDR_GSO_TCPV4 to request TCPv4
 segmentation, otherwise the driver MUST NOT set
@@ -610,6 +630,11 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 If VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} is not set, the
 device MUST NOT use the \field{csum_start} and \field{csum_offset}.
 
+If the VIRTIO_NET_F_OUT_NET_HEADER feature has been negotiated,
+and \field{out_nh_offset} is not zero, the device MAY use \field{out_nh_offset}
+as the outer network header offset. Otherwise, device MUST NOT use
+the \field{out_nh_offset}.
+
 If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have
 been negotiated:
 \begin{itemize}
@@ -725,6 +750,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
   set: if so, device has validated the packet checksum.
   In case of multiple encapsulated protocols, one level of checksums
   has been validated.
+\item If the VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, and if the packet
+  contains a valid network header, \field{out_nh_offset} MAY be set to indicate the
+  outer network header offset in packet.
 \end{enumerate}
 
 Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
@@ -802,6 +830,10 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in
 \field{gso_type}.
 
+If VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, the device MAY
+set the \field{out_nh_offset} to indicate outer network header offset, if packet contains
+a valid network header. Otherwise, the device MUST set \field{out_nh_offset} to zero.
+
 If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
 device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
 \field{flags}, if so:
@@ -851,6 +883,10 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 
 The driver MUST ignore \field{flag} bits that it does not recognize.
 
+If VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, and if \field{out_nh_offset}
+is nonzero, the driver MAY use \field{out_nh_offset} as outer network header
+offset. Otherwise, the driver MUST not use the \field{out_nh_offset}.
+
 If VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} is not set or
 if VIRTIO_NET_HDR_F_RSC_INFO bit \field{flags} is set, the
 driver MUST NOT use the \field{csum_start} and \field{csum_offset}.
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* RE: [PATCH v2] virtio-net: Introduce a new field to indicate outer network header offset
  2025-01-28 14:21 [PATCH v2] virtio-net: Introduce a new field to indicate outer network header offset Kommula Shiva Shankar
@ 2025-02-12  5:54 ` Parav Pandit
  0 siblings, 0 replies; 2+ messages in thread
From: Parav Pandit @ 2025-02-12  5:54 UTC (permalink / raw)
  To: Kommula Shiva Shankar, pabeni@redhat.com, willemb@google.com,
	jasowang@redhat.com, virtio-comment@lists.linux.dev
  Cc: ndabilpuram@marvell.com, jerinj@marvell.com,
	rbhansali@marvell.com



> From: Kommula Shiva Shankar <kshankar@marvell.com>
> Sent: Tuesday, January 28, 2025 7:52 PM
> 
> This patch introduces a new field in the virtio_net_header called
> out_nh_offset, along with a new net device feature,
> VIRTIO_NET_F_OUT_NET_HEADER.
> 
> Currently, there is no field available to directly read the outer network header
> offset in case of segmentation offload.
I believe checksum offload also need to know the start address of the network header?

Currently, drivers lack a dedicated field to signal the start of the network header to the device when performing checksum offload or segmentation offload.

> This requires reading packet data, which significantly affects performance in
> datapath.
> Additionally, some hardware implementations requrie knowledege of the
> outer L3 offset (aka L2 length) for inline IPSec hardware acceleration.
> 
> To address this limitation, we propose splitting the csum_offset field into two
> 8-bit fields named csum_offset and out_nh_offset.
> The csum_offset indicates the offset value from the csum_start and  may not
> exceed 256B bits(2^8) for protocols that use a 16-bit one's complement
> checksum
> 
> Following table lists such protocols and their checksum offset fields within
> their headers
> 
> +-----+--------+
> |Proto|csum_off|
> +-----+--------+
> | IPV4| 10B    |
> | ICMP| 2B     |
> | IGMP| 2B     |
> |  TCP| 16B    |
> |  UDP| 6B     |
> +-----+--------+
> 
> The out_nh_offset represents the start offset of the outer network header
> from the beginning of the packet data
s/start offset/start byte offset

wording packet data is confusing in context of packet header and data.
I think it should be just, say 'from beginning of the packet'.

> 

[..]

> 
>  v1 -> v2:
>  - explicitly state that the out_nh_offset can be set only when a valid network
> header is present.
>  - updated out_nh_offset usage in the RX direction.
>  - minor word cleanup.
> 
[..]
> 
> Signed-off-by: Kommula Shiva Shankar <kshankar@marvell.com>
> ---
>  device-types/net/description.tex | 38
> +++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/device-types/net/description.tex b/device-
> types/net/description.tex
> index 76585b0..529b7b9 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -88,6 +88,10 @@ \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_OUT_NET_HEADER(50)] The Driver can provide the
> start of \field{out_nh_offset}
> +    value. The Device gains advantage by not reading packet to calculate outer
> network
> +    header offset.
> +
Please rebase the tree, the next free available bit is 69.

>  \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.
> @@ -418,7 +422,13 @@ \subsection{Device Operation}\label{sec:Device
> Types / Network Device / Device O
>          le16 hdr_len;
>          le16 gso_size;
>          le16 csum_start;
> -        le16 csum_offset;
> +        union {
> +                le16 csum_offset;
> +                struct { (Only if VIRTIO_NET_F_OUT_NET_HEADER negotiated)
Please put the sentence "Only if VIRTIO as C comment.
/* ... */

> +                        le8 csum_offset;
> +                        le8 out_nh_offset;
> +                };
> +        };
>          le16 num_buffers;
>          le32 hash_value;        (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>          le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> @@ -457,6 +467,11 @@ \subsubsection{Packet
> Transmission}\label{sec:Device Types / Network Device / De  \item The driver
> can send a completely checksummed packet.  In this case,
>    \field{flags} will be zero, and \field{gso_type} will be
> VIRTIO_NET_HDR_GSO_NONE.
> 
> +\item The driver MAY optionally provide the \field{out_nh_offset} value,
> which is negotiated
Out_nh_offset is not negotiated. "which is negotiated" implies that.

You likely want to say,

When VIRTIO_NET_F_OUT_NET_HEADER is negotiated, the driver MAY optionally provide ...
A nonzero value of out_nh_offset indicates....

> +   using the VIRTIO_NET_F_OUT_NET_HEADER. If \field{out_nh_offset} is
> nonzero, it indicates
> +   a valid outer network header with in the packet, and specifies the offset in
> bytes from
> +   the beginning of the packet. Otherwise \field{out_nh_offset} MUST be set
> to zero.
> +
>  \item If the driver negotiated VIRTIO_NET_F_CSUM, it can skip
>    checksumming the packet:
>    \begin{itemize}
> @@ -531,6 +546,11 @@ \subsubsection{Packet
> Transmission}\label{sec:Device Types / Network Device / De  \field{flags} to
> zero and SHOULD supply a fully checksummed  packet to the device.
> 
> +If the VIRTIO_NET_F_OUT_NET_HEADER feature has been negotiated, the
> +driver MAY set \field{out_nh_offset} to indicate the start of the outer
> +network header offset, if the packet contains a valid network header.
> +Otherwise, \field{out_nh_offset} MUST be set to zero.
> +
It should be,
Otherwise, \field{out_nh_offset} is not used or not defined.
If you say, as zero, it means that field is valid and full 16-bits of csum_offset is not available leading to backward compatibility break.
 

>  If VIRTIO_NET_F_HOST_TSO4 is negotiated, the driver MAY set
> \field{gso_type} to VIRTIO_NET_HDR_GSO_TCPV4 to request TCPv4
> segmentation, otherwise the driver MUST NOT set @@ -610,6 +630,11 @@
> \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device
> / De  If VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} is not set, the
> device MUST NOT use the \field{csum_start} and \field{csum_offset}.
> 
> +If the VIRTIO_NET_F_OUT_NET_HEADER feature has been negotiated, and
> +\field{out_nh_offset} is not zero, the device MAY use
> +\field{out_nh_offset} as the outer network header offset. Otherwise,
> +device MUST NOT use the \field{out_nh_offset}.
> +
>  If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have
> been negotiated:
>  \begin{itemize}
> @@ -725,6 +750,9 @@ \subsubsection{Processing of Incoming
> Packets}\label{sec:Device Types / Network
>    set: if so, device has validated the packet checksum.
>    In case of multiple encapsulated protocols, one level of checksums
>    has been validated.
> +\item If the VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, and if
> +the packet
> +  contains a valid network header, \field{out_nh_offset} MAY be set to
> +indicate the
> +  outer network header offset in packet.
>  \end{enumerate}
> 
>  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN @@ -
> 802,6 +830,10 @@ \subsubsection{Processing of Incoming
> Packets}\label{sec:Device Types / Network  device MUST set the
> VIRTIO_NET_HDR_GSO_ECN bit in  \field{gso_type}.
> 
> +If VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, the device MAY
> set
> +the \field{out_nh_offset} to indicate outer network header offset, if
> +packet contains a valid network header. Otherwise, the device MUST set
> \field{out_nh_offset} to zero.
> +
MUST not use..

>  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the  device
> MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in  \field{flags}, if so:
> @@ -851,6 +883,10 @@ \subsubsection{Processing of Incoming
> Packets}\label{sec:Device Types / Network
> 
>  The driver MUST ignore \field{flag} bits that it does not recognize.
> 
> +If VIRTIO_NET_F_OUT_NET_HEADER has been negotiated, and if
> +\field{out_nh_offset} is nonzero, the driver MAY use
> +\field{out_nh_offset} as outer network header offset. Otherwise, the driver
> MUST not use the \field{out_nh_offset}.
> +
>  If VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} is not set or  if
> VIRTIO_NET_HDR_F_RSC_INFO bit \field{flags} is set, the  driver MUST NOT
> use the \field{csum_start} and \field{csum_offset}.
> --
> 2.43.0

Please rebase on top of the udp tunnel offload patches.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2025-02-12  5:54 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-28 14:21 [PATCH v2] virtio-net: Introduce a new field to indicate outer network header offset Kommula Shiva Shankar
2025-02-12  5:54 ` Parav Pandit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox