[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Re: [virtio-dev] [PATCH v8] virtio-net: correct conditions for devices to validate packet checksum
On Thu, Jan 4, 2024 at 12:47âPM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > å 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? > > Thanks a lot! Patch looks good to me. Reviewed-by: Jason Wang <jasowang@redhat.com> Thanks > > > > > Suggested-by: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Heng Qi <hengqi@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} > > > 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/ >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]