From: Jason Wang <jasowang@redhat.com>
To: Heng Qi <hengqi@linux.alibaba.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
"virtio-comment@lists.oasis-open.org"
<virtio-comment@lists.oasis-open.org>,
Yuri Benditovich <yuri.benditovich@daynix.com>,
Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>
Subject: Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
Date: Thu, 21 Dec 2023 12:04:27 +0800 [thread overview]
Message-ID: <CACGkMEv72n+Yb-PM4ZNNqYh8YYDcsCjeO_rV8+wamhVqahmdRA@mail.gmail.com> (raw)
In-Reply-To: <07a4dbe5-5467-494a-a24b-0300a0876390@linux.alibaba.com>
On Thu, Dec 21, 2023 at 11:51 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
>
> 在 2023/12/21 上午9:41, Jason Wang 写道:
> > On Wed, Dec 20, 2023 at 5:31 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>
> >>
> >> 在 2023/12/20 下午3:35, Michael S. Tsirkin 写道:
> >>> On Wed, Dec 20, 2023 at 02:30:01PM +0800, Heng Qi wrote:
> >>>> But why are we discussing this?
> >>> I think basically at this point everyone is confused about what
> >>> the feature does. right now we have packets
> >>> with
> >>> #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 -> partial
> >>> #define VIRTIO_NET_HDR_F_DATA_VALID 2 -> unnecessary
> >>> and packets without either -> none
> >>>
> >>> if both 1 and 2 are set then linux uses VIRTIO_NET_HDR_F_NEEDS_CSUM but
> >>> I am not sure it's not a mistake. Maybe it does not matter.
> >>>
> >>> What does this new thing do? So far all we have is "XDP will turn it on"
> >>> which is not really sufficient. I assumed it somehow replaces
> >>> partial with complete. That would make sense for many reasons,
> >>> for example the checksum fields in the header can be reused
> >>> for other purposes. But maybe not?
> >>
> >> Hello Jaosn and Michael. I've summarized our discussion so far, so check
> >> it out below. Thank you very much!
> >>
> >> From the nic perspective, I think Jason's statement is correct, the
> >> nic's checksum capability and setting DATA_VALID in flags
> >> should not be determined by GUEST_CSUM feature. As long as the rx
> >> checksum offload is turned on, DATA_VALID
> >> should be set. (Though we now bind GUEST_CSUM negotiation with rx
> >> checksum offload.)
> > I think we can fix this in the driver. Probably by just advertising
> > RXCSUM regardless of GUEST_CSUM?
>
> Right.
>
> >
> >> Therefore, we need to pay attention to the information of rx checksum
> >> offload. Please check it out:
> >>
> >> Devices that comply with the below description are said to be existing
> >> devices:
> >> "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."
> >>
> >> As suggested by Jason, devices that comply with the below description
> >> are said to be new devices:
> >> "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MAY* set
> >> flags to zero and SHOULD supply a fully checksummed packet to the driver."
> >>
> >>
> >> 1. Rx checksum offload is turned on
> >> GUEST_CSUM feature is not negotiated. (now it is only used to indicate
> >> whether the driver can handle partially checksummed packets)
> >> a. Existing devices continue to set flags to 0;
> > Note that existing devices can set DATA_VALID regardless of rx csum.
>
> Right.
>
> >
> >> b. New devices may validate the packets and have flags set to
> >> DATA_VALID;
> >> c. Migration.
> >> Migration of existing devices continues to check GUEST_CSUM
> >> feature and rx checksum offload;
> >> Migration of new devices only check rx checksum offload;
> >> Without updating the existing migration management and control
> >> system, existing devices cannot be migrated to new devices, and new
> >> devices cannot be migrated to existing devices.
> > Yes.
> >
> >> d. How offload should be controlled now needs attention. Should
> >> CTRL_GUEST_OFFLOADS still issue GUEST_CSUM feature bit to control the rx
> >> checksum offload?
> > So the only thing we need to do for the driver is, when rx csum is disabled:
> >
> > 1) drop packets with NEEDS_CSUM
> > 2) use CHECKSUM_NONE for the rest
> >
> > ?
>
> YES.
>
> >
> >> 2. The new FULLY_CSUM feature must disable NEEDS_CSUM.
> >> The device may set DATA_VALID regardless of whether FULLY_CSUM or
> >> GUEST_CSUM is negotiated.
> >> a. Rx fully checksum offload is still controlled by
> >> CTRL_GUEST_OFFLOADS carrying GUEST_FULLY_CSUM.
> >> b. When the rx device receives a partially checksummed packet, it
> >> should calculate the checksum and delivering a fully checksummed packet
> >> to the driver.
> >>
> >>
> >> So now, if we modify the existing spec as Jason suggested, I think it's OK.
> >> But we need to find out how to control rx checksum offload. WDYT?
> > See above, the driver can just not set CHECKSUM_UNNECESSARY in this case.
>
> I think what you are saying here is that CHECKSUM_UNNECESSARY cannot be
> set by the driver when rx checksum offload is turned off.
>
> Thanks！
Right.
Thanks
>
> >
> > Thanks
> >
> >> Thanks!
> >>
> >>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2023-12-21 4:05 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <959e1a7ccdfffaaadd865a627924cf492ce22bfa.1702277523.git.hengqi@linux.alibaba.com>
[not found] ` <20231211110350-mutt-send-email-mst@kernel.org>
[not found] ` <a8ba676e-de43-45ae-a6a8-a67d0dd41c1f@linux.alibaba.com>
[not found] ` <20231212032713-mutt-send-email-mst@kernel.org>
[not found] ` <bfd25f86-4292-45d2-a7a2-fe61bde0edb4@linux.alibaba.com>
[not found] ` <b739d275-3664-4509-8a91-f19690719475@linux.alibaba.com>
2023-12-15 9:51 ` [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum Heng Qi
2023-12-18 3:10 ` Jason Wang
2023-12-18 4:54 ` Heng Qi
2023-12-19 7:53 ` Jason Wang
2023-12-19 16:06 ` Heng Qi
2023-12-20 5:48 ` Jason Wang
2023-12-20 6:30 ` Heng Qi
2023-12-20 6:59 ` Jason Wang
2023-12-20 7:42 ` [virtio-dev] Re: [virtio-comment] " Heng Qi
2023-12-21 1:34 ` Jason Wang
2023-12-21 3:43 ` Heng Qi
2023-12-21 4:04 ` Jason Wang
2023-12-20 9:54 ` Heng Qi
2023-12-20 7:35 ` Michael S. Tsirkin
2023-12-20 9:31 ` [virtio-dev] Re: [virtio-comment] " Heng Qi
2023-12-21 1:41 ` Jason Wang
2023-12-21 1:50 ` Jason Wang
2023-12-21 3:51 ` Heng Qi
2023-12-21 4:04 ` Jason Wang [this message]
2023-12-21 1:34 ` Jason Wang
2023-12-21 3:45 ` [virtio-dev] Re: [virtio-comment] " Heng Qi
2023-12-21 3:51 ` Jason Wang
2023-12-19 18:41 ` Michael S. Tsirkin
2023-12-20 5:52 ` Jason Wang
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=CACGkMEv72n+Yb-PM4ZNNqYh8YYDcsCjeO_rV8+wamhVqahmdRA@mail.gmail.com \
--to=jasowang@redhat.com \
--cc=hengqi@linux.alibaba.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).