[PATCH v8] virtio-net: correct conditions for devices to validate packet checksum

? 2024/1/4 ??12:47, Heng Qi ??:
>
>
> ? 2024/1/3 ??2:35, Heng Qi ??:
>> There is a historical error in virtio spec:
>>    "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST*
>> set flags
>>     to zero and SHOULD supply a fully checksummed packet to the driver."
>>
>> Currently in Linux and virtio-related implementations, the device
>> validates
>> the packet checksum and sets DATA_VALID regardless of whether
>> VIRTIO_NET_F_GUEST_CSUM is negotiated.
>>
>> Please refer to the following summary and thread[1] for details and
>> reasons:
>>
>> Summary
>>

=============

>> 1. GUEST_CSUM at virtio spec 0.95 is intended to be compatible with
>> partially
>> checksummed packets (NEEDS_CSUM <-> CHECKSUM_PARTIAL). So GUEST_CSUM
>> is mapped
>> to NETIF_F_RXCSUM.
>> GUEST_CSUM only indicates whether the driver handles partially
>> checksummed packets.
>> When XDP is loaded, the GUEST_CSUM's offload will be disabled, which
>> means that
>> packets have NEEDS_CSUM set will be dropped, and packets have
>> DATA_VALID set will
>> still be received.
>>
>> 2. When DATA_VALID was added to Linux in 2011[2] and virtio1.0, it
>> was actually
>> expected that rx checksum offload (the driver can set
>> CHECKSUM_UNNECESSARY) had
>> nothing to do with whether GUEST_CSUM was negotiated. But due to an
>> error, below
>> desctiption was added incorrectly:
>>    "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST*
>> set flags
>>     to zero and SHOULD supply a fully checksummed packet to the driver."
>>
>> 3. We now hope to correct this error. Let the setting of DATA_VALID
>> not be
>> controlled by whether GUEST_CSUM is negotiated, but only controlled
>> by whether
>> rx checksum offload is enabled on the OS side. The state of this rx
>> checksum
>> offload is not aware of the device.
>>
>> 4. [Optional] NETIF_F_RXCSUM corresponding to rx checksum offload
>> should be
>> added to dev->hw_features. When the user turns off rx checksum
>> offload through
>> ethtool -K, neither NEEDS_CSUM nor DATA_VALID should be taken care
>> of, that is,
>> all packets will be marked as CHECKSUM_NONE.
>>
>> Related Link
>>

=============

>> [1]
>> https://lists.oasis-open.org/archives/virtio-comment/202312/msg00135.html
>> [2] 10a8d94a9574 ("virtio_net: introduce VIRTIO_NET_HDR_F_DATA_VALID")
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/185
>
> Hi Michael, Cornelia.
>
> Could you please open a voting for this fix?

Hi Michael and Cornelia.

Sincerely asking if you missed this message?

In addition, Jason recently responded with his reviewed-by tag.
Do I need to publish a new version carrying his tag (thanks Jason)?

Thanks.

>
> Thanks a lot!
>
>>
>> Suggested-by: Jason Wang <>
>> Signed-off-by: Heng Qi <>
>> ---
>> v7->v8:
>> - Drop the new FULLY_CSUM feature and optimize the description. @Jason
>>
>> v6->v7:
>> - FULLY_CSUM no longer depends on GUEST_CSUM.
>> - The description related to DATA_VALID has nothing to do with
>>    whether GUEST_CSUM is negotiated or not. @Jason
>>
>> v5->v6:
>> - Rewrite and clarify patch description. @Michael
>>
>> v4->v5:
>> - Remove the modification to the GUEST_CSUM. @Jason
>> - The description of this feature has been reorganized for greater
>> clarity. @Michael
>>
>> v3->v4:
>> - Streamline some repetitive descriptions. @Jason
>> - Add how features should work, when to be enabled, and overhead.
>> @Jason @Michael
>>
>> v2->v3:
>> - Add a section named "Driver Handles Fully Checksummed Packets"
>>    and more descriptions. @Michael
>>
>> v1->v2:
>> - Modify full checksum functionality as a configurable offload
>>    that is initially turned off. @Jason
>>
>>   device-types/net/description.tex | 14 +++

-----------

>>   1 file changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/device-types/net/description.tex
>> b/device-types/net/description.tex
>> index aff5e08..14978c0 100644
>> --- a/device-types/net/description.tex
>> +++ b/device-types/net/description.tex
>> @@ -723,8 +723,7 @@ \subsubsection{Processing of Incoming
>> Packets}\label{sec:Device Types / Network
>>     \field{num_buffers} is one, then the entire packet will be
>>     contained within this buffer, immediately following the struct
>>     virtio_net_hdr.
>> -\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
>> -  VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
>> +\item The VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
>>     set: if so, device has validated the packet checksum.
>>     In case of multiple encapsulated protocols, one level of checksums
>>     has been validated.
>> @@ -783,9 +782,8 @@ \subsubsection{Processing of Incoming
>> Packets}\label{sec:Device Types / Network
>>   packet together, such that at least \field{num_buffers} are
>>   observed by driver as used.
>>   -If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set
>> -\field{flags} to zero and SHOULD supply a fully checksummed
>> -packet to the driver.
>> +If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device SHOULD supply
>> +a fully checksummed packet to the driver.
>>     If VIRTIO_NET_F_GUEST_TSO4 is not negotiated, the device MUST NOT
>> set
>>   \field{gso_type} to VIRTIO_NET_HDR_GSO_TCPV4.
>> @@ -842,12 +840,6 @@ \subsubsection{Processing of Incoming
>> Packets}\label{sec:Device Types / Network
>>   not less than the length of the headers, including the transport
>>   header.
>>   -If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
>> -device MAY set the VIRTIO_NET_HDR_F_DATA_VALID bit in
>> -\field{flags}, if so, the device MUST validate the packet
>> -checksum (in case of multiple encapsulated protocols, one level
>> -of checksums is validated).
>> -
>>   \drivernormative{\paragraph}{Processing of Incoming
>>   Packets}{Device Types / Network Device / Device Operation /
>>   Processing of Incoming Packets}
>
>
>

---------------------------------------------------------------------
> To unsubscribe, e-mail: 
> For additional commands, e-mail: