From: Heng Qi <hengqi@linux.alibaba.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Yuri Benditovich <yuri.benditovich@daynix.com>,
virtio-comment@lists.oasis-open.org,
virtio-dev@lists.oasis-open.org,
Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Subject: Re: [virtio-dev] Re: [PATCH v8] virtio-net: correct conditions for devices to validate packet checksum
Date: Tue, 20 Feb 2024 10:38:47 +0800 [thread overview]
Message-ID: <c4fd63b3-5bdf-45e7-b846-3d858c3bd8ea@linux.alibaba.com> (raw)
In-Reply-To: <1705384341.865516-1-xuanzhuo@linux.alibaba.com>
在 2024/1/16 下午1:52, Xuan Zhuo 写道:
> On Wed, 3 Jan 2024 14:35:02 +0800, Heng Qi <hengqi@linux.alibaba.com> wrote:
>> 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 Cornelia,
Can this proposal be opened for voting?
Thank you very much!
>>
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
>> ---
>> 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}
>> --
>> 1.8.3.1
>>
> 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
prev parent reply other threads:[~2024-02-20 2:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 6:35 [virtio-dev] [PATCH v8] virtio-net: correct conditions for devices to validate packet checksum Heng Qi
2024-01-04 4:47 ` Heng Qi
2024-01-08 3:48 ` [virtio-dev] Re: [virtio-comment] " Jason Wang
2024-01-11 3:27 ` Heng Qi
2024-01-11 16:27 ` Cornelia Huck
2024-01-12 3:09 ` [virtio-dev] Re: [virtio-comment] " Heng Qi
2024-01-16 5:52 ` [virtio-dev] " Xuan Zhuo
2024-02-20 2:38 ` Heng Qi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c4fd63b3-5bdf-45e7-b846-3d858c3bd8ea@linux.alibaba.com \
--to=hengqi@linux.alibaba.com \
--cc=cohuck@redhat.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=virtio-comment@lists.oasis-open.org \
--cc=virtio-dev@lists.oasis-open.org \
--cc=xuanzhuo@linux.alibaba.com \
--cc=yuri.benditovich@daynix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).