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

[Heng Qi <hengqi@linux.alibaba.com>]

å 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 <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}
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org