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

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

å 2024/1/12 äå12:27, Cornelia Huck åé:
> On Thu, Jan 11 2024, Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> > å 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?
> I've been back from end-of-year PTO only this week...
>
> I'm currently waiting for a confirmed updated schedule for the
> infrastructure migration, as I don't want to risk having the platform
> out of order in the middle of the voting period.

I got it. It depends on when the infrastructure is migrated, thanks for 
the sync.

> > In addition, Jason recently responded with his reviewed-by tag.
> > Do I need to publish a new version carrying his tag (thanks Jason)?
> No, we can add his R-b when pushing to git.

Ok.

Thanks.