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
On Thu, Dec 21, 2023 at 11:51 AM Heng Qi wrote: > > > > 在 2023/12/21 上午9:41, Jason Wang 写道: > > On Wed, Dec 20, 2023 at 5:31 PM Heng Qi 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-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
On Thu, Dec 21, 2023 at 11:43 AM Heng Qi wrote:
>
>
>
> 在 2023/12/21 上午9:34, Jason Wang 写道:
> > On Wed, Dec 20, 2023 at 3:42 PM Heng Qi wrote:
> >>
> >>
> >> 在 2023/12/20 下午2:59, Jason Wang 写道:
> >>> On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
>
> 在 2023/12/20 下午1:48, Jason Wang 写道:
> > On Wed, Dec 20, 2023 at 12:07 AM Heng Qi
> > wrote:
> >> 在 2023/12/19 下午3:53, Jason Wang 写道:
> >>> On Mon, Dec 18, 2023 at 12:54 PM Heng Qi
> >>> wrote:
> 在 2023/12/18 上午11:10, Jason Wang 写道:
> > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi
> > wrote:
> >> Hi all!
> >>
> >> I would like to ask if anyone has any comments on this version, if
> >> so
> >> please let me know!
> >> If not, I will collect Michael's comments and publish a new
> >> version next
> >> Monday.
> > I have a dumb question. (And sorry if I asked it before)
> >
> > Looking at the spec and code. It looks to me DATA_VALID could be set
> > without GUEST_CSUM.
> I don't see that in the spec.
> Am I missing something? [1][2]
>
> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
> features
> *enable receive checksum*, large receive offload and ECN support
> which
> are the input equivalents of the transmit checksum, transmit
> segmentation *offloading* and ECN features, as described in 5.1.6.2.
>
> [2] 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.
> >>> So this is kind of ambiguous and seems not what I wanted when I wrote
> >>> the code for DATA_VALID in 2011.
> >> Hi Jason, please see below.
> >>
> >>> NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> >>> correct.
> >> Yes. This mapping is because the PARTIAL checksum usually does not go
> >> through the physical wire,
> >> so it is considered safe, and the checksum does not need to be
> >> verified.
> >>
> >>> So spec had
> >>>
> >>> """
> >>> If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> >>> is set, the driver MUST NOT rely on the packet checksum being correct.
> >>> """
> >> Yes. The checksum of a packet without NEEDS_CSUM or has not been
> >> verified (DATA_VALID set) is unreliable.
> >> This patch doesn't break that.
> >>
> >>> For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> >>> exclusive with CHECKSUM_PARTAIL.
> >> Yes. Both cannot be set or appear at the same time.
> > So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
> >
> > NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
> This is not containing checksum, the pseudo header checksum is saved in
> the checksum field of the transport header.
> >>> I have a hard time understanding this. But yes, basically I meant the
> >>> checksum is partial. So the device can't do validation.
> >> If the rx device does receive a partially checksummed packet, but the
> >> driver requires a fullly
> >> checksummed packet, then the rx device can help to calculate the full
> >> checksum for packets.
> > So this can only happen for virtual devices as hardware devices can't
> > receive partial csum packets.
>
> YES. It should be.
>
> >
> > DATA_VALID: the checksum has been validated, this implies the packet
> > contains a checksum
> I'm not sure if both are set at the same time, and even if set,
> CHECKSUM_PARTIAL will still work when forwarded.
> But why are we discussing this?
> >>> I don't get this question.
> >>>
> >>> As a reviewer, I have the right to raise any issue I spot. This is how
> >>> the community works.
> >> Sorry I wasn't questioning your question, and I think you captured the
> >> concerns very well from a nic perspective.
> > I see, thanks. I want to offer help indeed.
>
> Thanks very much!
>
> >
> >>> It is intended to reply to the past discussion
> >>>
> >>> 1) like your above statement "Both cannot be set or appear at the same
> >>> time."
> >>> 2) the example in Linux where CHECKSUM_UNNECESSARY and
> >>> CHECKSUM_PARTIAL are mutually exclusive.
> >>>
> >>> And this is what Linux did right now:
> >>>
> >>> For tun_put_user():
> >>>
> >>> if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >>> ...
> >>> } else if (has_data_valid &&
> >>>
[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
On Thu, Dec 21, 2023 at 11:45 AM Heng Qi wrote: > > > > 在 2023/12/21 上午9:34, Jason Wang 写道: > > On Wed, Dec 20, 2023 at 3:35 PM Michael S. Tsirkin wrote: > >> 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. > > It looks not? CHECKSUM_COMPLETE is less optimal than > > CHECKSUM_UNNCESSARY as validation is still needed. > > > > If I understand correctly, this new thing wants DATA_VALID only. > > Disable NEEDS_CSUM or calculate fully checksummed packets to fully > checksummed packets (how this is done does not matter). > The driver will only receive two types of packets: CHECKSUM_NONE and > DATA_VALID (CHECKSUM_UNNECESSARY). Right, this is my understanding as well. Thanks > > Thanks！ > > > > > Thanks > > > > > > > >> That would make sense for many reasons, > >> for example the checksum fields in the header can be reused > >> for other purposes. But maybe not? > >> > >> > >> -- > >> MST > >> > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
在 2023/12/21 上午9:34, Jason Wang 写道: On Wed, Dec 20, 2023 at 3:35 PM Michael S. Tsirkin wrote: 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. It looks not? CHECKSUM_COMPLETE is less optimal than CHECKSUM_UNNCESSARY as validation is still needed. If I understand correctly, this new thing wants DATA_VALID only. Disable NEEDS_CSUM or calculate fully checksummed packets to fully checksummed packets (how this is done does not matter). The driver will only receive two types of packets: CHECKSUM_NONE and DATA_VALID (CHECKSUM_UNNECESSARY). Thanks！ Thanks That would make sense for many reasons, for example the checksum fields in the header can be reused for other purposes. But maybe not? -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
在 2023/12/21 上午9:34, Jason Wang 写道:
On Wed, Dec 20, 2023 at 3:42 PM Heng Qi wrote:
在 2023/12/20 下午2:59, Jason Wang 写道:
On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
在 2023/12/20 下午1:48, Jason Wang 写道:
On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
在 2023/12/19 下午3:53, Jason Wang 写道:
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
在 2023/12/18 上午11:10, Jason Wang 写道:
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
I don't see that in the spec.
Am I missing something? [1][2]
[1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
*enable receive checksum*, large receive offload and ECN support which
are the input equivalents of the transmit checksum, transmit
segmentation *offloading* and ECN features, as described in 5.1.6.2.
[2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
Hi Jason, please see below.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct.
Yes. This mapping is because the PARTIAL checksum usually does not go
through the physical wire,
so it is considered safe, and the checksum does not need to be verified.
So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
Yes. The checksum of a packet without NEEDS_CSUM or has not been
verified (DATA_VALID set) is unreliable.
This patch doesn't break that.
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL.
Yes. Both cannot be set or appear at the same time.
So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
This is not containing checksum, the pseudo header checksum is saved in
the checksum field of the transport header.
I have a hard time understanding this. But yes, basically I meant the
checksum is partial. So the device can't do validation.
If the rx device does receive a partially checksummed packet, but the
driver requires a fullly
checksummed packet, then the rx device can help to calculate the full
checksum for packets.
So this can only happen for virtual devices as hardware devices can't
receive partial csum packets.
YES. It should be.
DATA_VALID: the checksum has been validated, this implies the packet
contains a checksum
I'm not sure if both are set at the same time, and even if set,
CHECKSUM_PARTIAL will still work when forwarded.
But why are we discussing this?
I don't get this question.
As a reviewer, I have the right to raise any issue I spot. This is how
the community works.
Sorry I wasn't questioning your question, and I think you captured the
concerns very well from a nic perspective.
I see, thanks. I want to offer help indeed.
Thanks very much!
It is intended to reply to the past discussion
1) like your above statement "Both cannot be set or appear at the same time."
2) the example in Linux where CHECKSUM_UNNECESSARY and
CHECKSUM_PARTIAL are mutually exclusive.
And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
I think you are talking about this commit:
10a8d94a95742bb15b4e617ee9884bb4381362be
But in fact, as your commit log says, I think this is a hack.
It's not, see below.
Host nics
does not fall into the scope of virtio spec?
Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
virtio-net differs in this case.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Sorry. I cannot follow this view.
1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
now, because we have no dispute about it) does represent the device's
[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
On Thu, Dec 21, 2023 at 9:41 AM Jason Wang wrote: > > On Wed, Dec 20, 2023 at 5:31 PM Heng Qi 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? > > > > > 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. > > > 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 > > ? > > > > > 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. For sure, when GUEST_CSUM is enabled, we need to disable it as well. Thanks > > Thanks > > > > > Thanks! > > > > > > > > > > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
On Wed, Dec 20, 2023 at 5:31 PM Heng Qi 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? > > 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. > 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 ? > > 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. Thanks > > Thanks! > > > > > > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Wed, Dec 20, 2023 at 3:35 PM Michael S. Tsirkin wrote: > > 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. It looks not? CHECKSUM_COMPLETE is less optimal than CHECKSUM_UNNCESSARY as validation is still needed. If I understand correctly, this new thing wants DATA_VALID only. Thanks > That would make sense for many reasons, > for example the checksum fields in the header can be reused > for other purposes. But maybe not? > > > -- > MST > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
On Wed, Dec 20, 2023 at 3:42 PM Heng Qi wrote:
>
>
>
> 在 2023/12/20 下午2:59, Jason Wang 写道:
> > On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
> >>
> >>
> >> 在 2023/12/20 下午1:48, Jason Wang 写道:
> >>> On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
>
> 在 2023/12/19 下午3:53, Jason Wang 写道:
> > On Mon, Dec 18, 2023 at 12:54 PM Heng Qi
> > wrote:
> >> 在 2023/12/18 上午11:10, Jason Wang 写道:
> >>> On Fri, Dec 15, 2023 at 5:51 PM Heng Qi
> >>> wrote:
> Hi all!
>
> I would like to ask if anyone has any comments on this version, if so
> please let me know!
> If not, I will collect Michael's comments and publish a new version
> next
> Monday.
> >>> I have a dumb question. (And sorry if I asked it before)
> >>>
> >>> Looking at the spec and code. It looks to me DATA_VALID could be set
> >>> without GUEST_CSUM.
> >> I don't see that in the spec.
> >> Am I missing something? [1][2]
> >>
> >> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> >> VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> >> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> >> *enable receive checksum*, large receive offload and ECN support which
> >> are the input equivalents of the transmit checksum, transmit
> >> segmentation *offloading* and ECN features, as described in 5.1.6.2.
> >>
> >> [2] 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.
> > So this is kind of ambiguous and seems not what I wanted when I wrote
> > the code for DATA_VALID in 2011.
> Hi Jason, please see below.
>
> > NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> > correct.
> Yes. This mapping is because the PARTIAL checksum usually does not go
> through the physical wire,
> so it is considered safe, and the checksum does not need to be verified.
>
> > So spec had
> >
> > """
> > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> > is set, the driver MUST NOT rely on the packet checksum being correct.
> > """
> Yes. The checksum of a packet without NEEDS_CSUM or has not been
> verified (DATA_VALID set) is unreliable.
> This patch doesn't break that.
>
> > For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> > exclusive with CHECKSUM_PARTAIL.
> Yes. Both cannot be set or appear at the same time.
> >>> So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
> >>>
> >>> NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
> >> This is not containing checksum, the pseudo header checksum is saved in
> >> the checksum field of the transport header.
> > I have a hard time understanding this. But yes, basically I meant the
> > checksum is partial. So the device can't do validation.
>
> If the rx device does receive a partially checksummed packet, but the
> driver requires a fullly
> checksummed packet, then the rx device can help to calculate the full
> checksum for packets.
So this can only happen for virtual devices as hardware devices can't
receive partial csum packets.
>
> >
> >>> DATA_VALID: the checksum has been validated, this implies the packet
> >>> contains a checksum
> >> I'm not sure if both are set at the same time, and even if set,
> >> CHECKSUM_PARTIAL will still work when forwarded.
> >> But why are we discussing this?
> > I don't get this question.
> >
> > As a reviewer, I have the right to raise any issue I spot. This is how
> > the community works.
>
> Sorry I wasn't questioning your question, and I think you captured the
> concerns very well from a nic perspective.
I see, thanks. I want to offer help indeed.
>
> >
> > It is intended to reply to the past discussion
> >
> > 1) like your above statement "Both cannot be set or appear at the same
> > time."
> > 2) the example in Linux where CHECKSUM_UNNECESSARY and
> > CHECKSUM_PARTIAL are mutually exclusive.
> >
> > And this is what Linux did right now:
> >
> > For tun_put_user():
> >
> >if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >...
> >} else if (has_data_valid &&
> > skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> >} /* else everything is zero */
> >
> > This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> > I was not wrong.
> I think you are talking about this commit:
> 10a8d94a95742bb15b4e617ee9884bb4381362be
>
> But in fact, as your
[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
在 2023/12/20 下午2:59, Jason Wang 写道:
On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
在 2023/12/20 下午1:48, Jason Wang 写道:
On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
在 2023/12/19 下午3:53, Jason Wang 写道:
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
在 2023/12/18 上午11:10, Jason Wang 写道:
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
I don't see that in the spec.
Am I missing something? [1][2]
[1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
*enable receive checksum*, large receive offload and ECN support which
are the input equivalents of the transmit checksum, transmit
segmentation *offloading* and ECN features, as described in 5.1.6.2.
[2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
Hi Jason, please see below.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct.
Yes. This mapping is because the PARTIAL checksum usually does not go
through the physical wire,
so it is considered safe, and the checksum does not need to be verified.
So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
Yes. The checksum of a packet without NEEDS_CSUM or has not been
verified (DATA_VALID set) is unreliable.
This patch doesn't break that.
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL.
Yes. Both cannot be set or appear at the same time.
So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
This is not containing checksum, the pseudo header checksum is saved in
the checksum field of the transport header.
I have a hard time understanding this. But yes, basically I meant the
checksum is partial. So the device can't do validation.
DATA_VALID: the checksum has been validated, this implies the packet
contains a checksum
I'm not sure if both are set at the same time, and even if set,
CHECKSUM_PARTIAL will still work when forwarded.
But why are we discussing this?
I don't get this question.
As a reviewer, I have the right to raise any issue I spot. This is how
the community works.
It is intended to reply to the past discussion
1) like your above statement "Both cannot be set or appear at the same time."
2) the example in Linux where CHECKSUM_UNNECESSARY and
CHECKSUM_PARTIAL are mutually exclusive.
And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
I think you are talking about this commit:
10a8d94a95742bb15b4e617ee9884bb4381362be
But in fact, as your commit log says, I think this is a hack.
It's not, see below.
Host nics
does not fall into the scope of virtio spec?
Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
virtio-net differs in this case.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Sorry. I cannot follow this view.
1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
now, because we have no dispute about it) does represent the device's
ability to calculate and verify checksums.
Its ability to handle partial checksums (NEEDS_CSUM) is just a special
processing of virtio, the Linux kernel never had a netdev feature for
partial checksum handling.
1.1 VIRTIO_NET_F_GUEST_{TSO4, TSO6, USO4} etc. depend on
VIRTIO_NET_F_GUEST_CSUM.
The reason for being relied upon is not that they are related
to NEEDS_CSUM, but that the device needs to recalculate and verify the
checksum of the packets when merging the packets.
See netdev_fix_features:
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
[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
在 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.) 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; 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. 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? 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? Thanks! - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[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
在 2023/12/20 下午2:59, Jason Wang 写道:
On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
在 2023/12/20 下午1:48, Jason Wang 写道:
On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
在 2023/12/19 下午3:53, Jason Wang 写道:
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
在 2023/12/18 上午11:10, Jason Wang 写道:
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
I don't see that in the spec.
Am I missing something? [1][2]
[1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
*enable receive checksum*, large receive offload and ECN support which
are the input equivalents of the transmit checksum, transmit
segmentation *offloading* and ECN features, as described in 5.1.6.2.
[2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
Hi Jason, please see below.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct.
Yes. This mapping is because the PARTIAL checksum usually does not go
through the physical wire,
so it is considered safe, and the checksum does not need to be verified.
So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
Yes. The checksum of a packet without NEEDS_CSUM or has not been
verified (DATA_VALID set) is unreliable.
This patch doesn't break that.
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL.
Yes. Both cannot be set or appear at the same time.
So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
This is not containing checksum, the pseudo header checksum is saved in
the checksum field of the transport header.
I have a hard time understanding this. But yes, basically I meant the
checksum is partial. So the device can't do validation.
If the rx device does receive a partially checksummed packet, but the
driver requires a fullly
checksummed packet, then the rx device can help to calculate the full
checksum for packets.
DATA_VALID: the checksum has been validated, this implies the packet
contains a checksum
I'm not sure if both are set at the same time, and even if set,
CHECKSUM_PARTIAL will still work when forwarded.
But why are we discussing this?
I don't get this question.
As a reviewer, I have the right to raise any issue I spot. This is how
the community works.
Sorry I wasn't questioning your question, and I think you captured the
concerns very well from a nic perspective.
It is intended to reply to the past discussion
1) like your above statement "Both cannot be set or appear at the same time."
2) the example in Linux where CHECKSUM_UNNECESSARY and
CHECKSUM_PARTIAL are mutually exclusive.
And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
I think you are talking about this commit:
10a8d94a95742bb15b4e617ee9884bb4381362be
But in fact, as your commit log says, I think this is a hack.
It's not, see below.
Host nics
does not fall into the scope of virtio spec?
Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
virtio-net differs in this case.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Sorry. I cannot follow this view.
1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
now, because we have no dispute about it) does represent the device's
ability to calculate and verify checksums.
Its ability to handle partial checksums (NEEDS_CSUM) is just a special
processing of virtio, the Linux kernel never had a netdev feature for
partial checksum handling.
1.1 VIRTIO_NET_F_GUEST_{TSO4, TSO6, USO4} etc. depend on
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
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? -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Wed, Dec 20, 2023 at 2:30 PM Heng Qi wrote:
>
>
>
> 在 2023/12/20 下午1:48, Jason Wang 写道:
> > On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
> >>
> >>
> >> 在 2023/12/19 下午3:53, Jason Wang 写道:
> >>> On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
>
> 在 2023/12/18 上午11:10, Jason Wang 写道:
> > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi
> > wrote:
> >> Hi all!
> >>
> >> I would like to ask if anyone has any comments on this version, if so
> >> please let me know!
> >> If not, I will collect Michael's comments and publish a new version
> >> next
> >> Monday.
> > I have a dumb question. (And sorry if I asked it before)
> >
> > Looking at the spec and code. It looks to me DATA_VALID could be set
> > without GUEST_CSUM.
> I don't see that in the spec.
> Am I missing something? [1][2]
>
> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> *enable receive checksum*, large receive offload and ECN support which
> are the input equivalents of the transmit checksum, transmit
> segmentation *offloading* and ECN features, as described in 5.1.6.2.
>
> [2] 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.
> >>> So this is kind of ambiguous and seems not what I wanted when I wrote
> >>> the code for DATA_VALID in 2011.
> >> Hi Jason, please see below.
> >>
> >>> NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> >>> correct.
> >> Yes. This mapping is because the PARTIAL checksum usually does not go
> >> through the physical wire,
> >> so it is considered safe, and the checksum does not need to be verified.
> >>
> >>> So spec had
> >>>
> >>> """
> >>> If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> >>> is set, the driver MUST NOT rely on the packet checksum being correct.
> >>> """
> >> Yes. The checksum of a packet without NEEDS_CSUM or has not been
> >> verified (DATA_VALID set) is unreliable.
> >> This patch doesn't break that.
> >>
> >>> For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> >>> exclusive with CHECKSUM_PARTAIL.
> >> Yes. Both cannot be set or appear at the same time.
> > So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
> >
> > NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
>
> This is not containing checksum, the pseudo header checksum is saved in
> the checksum field of the transport header.
I have a hard time understanding this. But yes, basically I meant the
checksum is partial. So the device can't do validation.
>
> > DATA_VALID: the checksum has been validated, this implies the packet
> > contains a checksum
>
> I'm not sure if both are set at the same time, and even if set,
> CHECKSUM_PARTIAL will still work when forwarded.
> But why are we discussing this?
I don't get this question.
As a reviewer, I have the right to raise any issue I spot. This is how
the community works.
It is intended to reply to the past discussion
1) like your above statement "Both cannot be set or appear at the same time."
2) the example in Linux where CHECKSUM_UNNECESSARY and
CHECKSUM_PARTIAL are mutually exclusive.
>
> >
> >>> And this is what Linux did right now:
> >>>
> >>> For tun_put_user():
> >>>
> >>> if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >>> ...
> >>> } else if (has_data_valid &&
> >>> skb->ip_summed == CHECKSUM_UNNECESSARY) {
> >>> hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> >>> } /* else everything is zero */
> >>>
> >>> This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> >>> I was not wrong.
> >> I think you are talking about this commit:
> >> 10a8d94a95742bb15b4e617ee9884bb4381362be
> >>
> >> But in fact, as your commit log says, I think this is a hack.
> > It's not, see below.
> >
> >> Host nics
> >> does not fall into the scope of virtio spec?
> > Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
> > virtio-net differs in this case.
> >
> >>
> >>> And in receive_buf():
> >>>
> >>> if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> >>> skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>>
> >>> I think we can fix this by safely removing "*MUST set flags to zero*"
> >>> in [2] from the spec.
> >> Sorry. I cannot follow this view.
> >>
> >> 1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
> >> now, because we have no dispute about it) does represent the device's
> >> ability to calculate and verify checksums.
> >> Its
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
在 2023/12/20 下午1:48, Jason Wang 写道:
On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
在 2023/12/19 下午3:53, Jason Wang 写道:
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
在 2023/12/18 上午11:10, Jason Wang 写道:
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
I don't see that in the spec.
Am I missing something? [1][2]
[1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
*enable receive checksum*, large receive offload and ECN support which
are the input equivalents of the transmit checksum, transmit
segmentation *offloading* and ECN features, as described in 5.1.6.2.
[2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
Hi Jason, please see below.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct.
Yes. This mapping is because the PARTIAL checksum usually does not go
through the physical wire,
so it is considered safe, and the checksum does not need to be verified.
So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
Yes. The checksum of a packet without NEEDS_CSUM or has not been
verified (DATA_VALID set) is unreliable.
This patch doesn't break that.
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL.
Yes. Both cannot be set or appear at the same time.
So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
This is not containing checksum, the pseudo header checksum is saved in
the checksum field of the transport header.
DATA_VALID: the checksum has been validated, this implies the packet
contains a checksum
I'm not sure if both are set at the same time, and even if set,
CHECKSUM_PARTIAL will still work when forwarded.
But why are we discussing this?
And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
I think you are talking about this commit:
10a8d94a95742bb15b4e617ee9884bb4381362be
But in fact, as your commit log says, I think this is a hack.
It's not, see below.
Host nics
does not fall into the scope of virtio spec?
Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
virtio-net differs in this case.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Sorry. I cannot follow this view.
1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
now, because we have no dispute about it) does represent the device's
ability to calculate and verify checksums.
Its ability to handle partial checksums (NEEDS_CSUM) is just a special
processing of virtio, the Linux kernel never had a netdev feature for
partial checksum handling.
1.1 VIRTIO_NET_F_GUEST_{TSO4, TSO6, USO4} etc. depend on
VIRTIO_NET_F_GUEST_CSUM.
The reason for being relied upon is not that they are related
to NEEDS_CSUM, but that the device needs to recalculate and verify the
checksum of the packets when merging the packets.
See netdev_fix_features:
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
dev->features |= NETIF_F_RXCSUM;
- netdev_fix_features ->
if (!(features & NETIF_F_RXCSUM)) {
/* NETIF_F_GRO_HW implies doing RXCSUM since every packet
* successfully merged by hardware must also have the
* checksum verified by hardware. If the user does not
* want to enable RXCSUM, logically, we should disable
GRO_HW.
*/
if (features & NETIF_F_GRO_HW) {
netdev_dbg(dev, "Dropping
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Wed, Dec 20, 2023 at 2:41 AM Michael S. Tsirkin wrote:
>
> On Tue, Dec 19, 2023 at 03:53:14PM +0800, Jason Wang wrote:
> > On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
> > >
> > >
> > >
> > > 在 2023/12/18 上午11:10, Jason Wang 写道:
> > > > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi
> > > > wrote:
> > > >> Hi all!
> > > >>
> > > >> I would like to ask if anyone has any comments on this version, if so
> > > >> please let me know!
> > > >> If not, I will collect Michael's comments and publish a new version
> > > >> next
> > > >> Monday.
> > > > I have a dumb question. (And sorry if I asked it before)
> > > >
> > > > Looking at the spec and code. It looks to me DATA_VALID could be set
> > > > without GUEST_CSUM.
> > >
> > > I don't see that in the spec.
> > > Am I missing something? [1][2]
> > >
> > > [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > > VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> > > Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> > > *enable receive checksum*, large receive offload and ECN support which
> > > are the input equivalents of the transmit checksum, transmit
> > > segmentation *offloading* and ECN features, as described in 5.1.6.2.
> > >
> > > [2] 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.
> >
> > So this is kind of ambiguous and seems not what I wanted when I wrote
> > the code for DATA_VALID in 2011.
> >
> > NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> > correct. So spec had
> >
> > """
> > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> > is set, the driver MUST NOT rely on the packet checksum being correct.
> > """
> >
> > For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> > exclusive with CHECKSUM_PARTAIL. And this is what Linux did right now:
> >
> > For tun_put_user():
> >
> > if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > ...
> > } else if (has_data_valid &&
> >skb->ip_summed == CHECKSUM_UNNECESSARY) {
> >hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> > } /* else everything is zero */
> >
> > This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> > I was not wrong.
> >
> > And in receive_buf():
> >
> > if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > skb->ip_summed = CHECKSUM_UNNECESSARY;
> >
> > I think we can fix this by safely removing "*MUST set flags to zero*"
> > in [2] from the spec.
> >
> > Thanks
>
> I don't get why you want to remove this. I think this text refers to
> when no offloads are negotiated. Why would device then set any flags?
Current Linux can produce DATA_VALID without GUEST_CSUM. So it just
wants to align with that.
Driver can just not set CHECKSUM_UNNECESSARY if the rx csum is disabled.
Thanks
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Wed, Dec 20, 2023 at 12:07 AM Heng Qi wrote:
>
>
>
> 在 2023/12/19 下午3:53, Jason Wang 写道:
> > On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
> >>
> >>
> >> 在 2023/12/18 上午11:10, Jason Wang 写道:
> >>> On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
> Hi all!
>
> I would like to ask if anyone has any comments on this version, if so
> please let me know!
> If not, I will collect Michael's comments and publish a new version next
> Monday.
> >>> I have a dumb question. (And sorry if I asked it before)
> >>>
> >>> Looking at the spec and code. It looks to me DATA_VALID could be set
> >>> without GUEST_CSUM.
> >> I don't see that in the spec.
> >> Am I missing something? [1][2]
> >>
> >> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> >> VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> >> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> >> *enable receive checksum*, large receive offload and ECN support which
> >> are the input equivalents of the transmit checksum, transmit
> >> segmentation *offloading* and ECN features, as described in 5.1.6.2.
> >>
> >> [2] 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.
> > So this is kind of ambiguous and seems not what I wanted when I wrote
> > the code for DATA_VALID in 2011.
>
> Hi Jason, please see below.
>
> >
> > NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> > correct.
>
> Yes. This mapping is because the PARTIAL checksum usually does not go
> through the physical wire,
> so it is considered safe, and the checksum does not need to be verified.
>
> > So spec had
> >
> > """
> > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> > is set, the driver MUST NOT rely on the packet checksum being correct.
> > """
>
> Yes. The checksum of a packet without NEEDS_CSUM or has not been
> verified (DATA_VALID set) is unreliable.
> This patch doesn't break that.
>
> >
> > For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> > exclusive with CHECKSUM_PARTAIL.
>
> Yes. Both cannot be set or appear at the same time.
So setting both DATA_VALID and NEEDS_CSUM seems ambiguous.
NEEDS_CSUM: the data is correct but the packet doesn't contain checksum
DATA_VALID: the checksum has been validated, this implies the packet
contains a checksum
>
> > And this is what Linux did right now:
> >
> > For tun_put_user():
> >
> > if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > ...
> > } else if (has_data_valid &&
> > skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> > } /* else everything is zero */
> >
> > This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> > I was not wrong.
>
> I think you are talking about this commit:
> 10a8d94a95742bb15b4e617ee9884bb4381362be
>
> But in fact, as your commit log says, I think this is a hack.
It's not, see below.
> Host nics
> does not fall into the scope of virtio spec?
Seems not, a lot of NIC produces CHECKSUM_UNNECESSARY, I don't see how
virtio-net differs in this case.
>
>
> >
> > And in receive_buf():
> >
> > if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > skb->ip_summed = CHECKSUM_UNNECESSARY;
> >
> > I think we can fix this by safely removing "*MUST set flags to zero*"
> > in [2] from the spec.
>
> Sorry. I cannot follow this view.
>
> 1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
> now, because we have no dispute about it) does represent the device's
> ability to calculate and verify checksums.
> Its ability to handle partial checksums (NEEDS_CSUM) is just a special
> processing of virtio, the Linux kernel never had a netdev feature for
> partial checksum handling.
>
>1.1 VIRTIO_NET_F_GUEST_{TSO4, TSO6, USO4} etc. depend on
> VIRTIO_NET_F_GUEST_CSUM.
> The reason for being relied upon is not that they are related
> to NEEDS_CSUM, but that the device needs to recalculate and verify the
> checksum of the packets when merging the packets.
> See netdev_fix_features:
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> dev->features |= NETIF_F_RXCSUM;
>- netdev_fix_features ->
> if (!(features & NETIF_F_RXCSUM)) {
> /* NETIF_F_GRO_HW implies doing RXCSUM since every packet
>* successfully merged by hardware must also have the
>* checksum verified by hardware. If the user does not
>* want to enable RXCSUM, logically, we should disable
> GRO_HW.
>*/
> if (features & NETIF_F_GRO_HW) {
>
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Tue, Dec 19, 2023 at 03:53:14PM +0800, Jason Wang wrote:
> On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
> >
> >
> >
> > 在 2023/12/18 上午11:10, Jason Wang 写道:
> > > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
> > >> Hi all!
> > >>
> > >> I would like to ask if anyone has any comments on this version, if so
> > >> please let me know!
> > >> If not, I will collect Michael's comments and publish a new version next
> > >> Monday.
> > > I have a dumb question. (And sorry if I asked it before)
> > >
> > > Looking at the spec and code. It looks to me DATA_VALID could be set
> > > without GUEST_CSUM.
> >
> > I don't see that in the spec.
> > Am I missing something? [1][2]
> >
> > [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> > Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> > *enable receive checksum*, large receive offload and ECN support which
> > are the input equivalents of the transmit checksum, transmit
> > segmentation *offloading* and ECN features, as described in 5.1.6.2.
> >
> > [2] 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.
>
> So this is kind of ambiguous and seems not what I wanted when I wrote
> the code for DATA_VALID in 2011.
>
> NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> correct. So spec had
>
> """
> If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> is set, the driver MUST NOT rely on the packet checksum being correct.
> """
>
> For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> exclusive with CHECKSUM_PARTAIL. And this is what Linux did right now:
>
> For tun_put_user():
>
> if (skb->ip_summed == CHECKSUM_PARTIAL) {
> ...
> } else if (has_data_valid &&
>skb->ip_summed == CHECKSUM_UNNECESSARY) {
>hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> } /* else everything is zero */
>
> This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> I was not wrong.
>
> And in receive_buf():
>
> if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> I think we can fix this by safely removing "*MUST set flags to zero*"
> in [2] from the spec.
>
> Thanks
I don't get why you want to remove this. I think this text refers to
when no offloads are negotiated. Why would device then set any flags?
>
> >
> > I think the reason why the feature bit is not checked in the code is
> > because the check is omitted because it is on a per-packet basis,
> > just like the reason why supported_valid_types is not needed as
> > discussed in the v4 version threads. It is not unnecessary.
> >
> > Thanks!
> >
> > >
> > > If yes, why do we need to bother here? If we disable GUEST_CSUM, the
> > > packet will contain checksum. And if the device sets DATA_VALID, it
> > > means the checksum is validated.
> > >
> > > Thanks
> > >
> > >
> > >
> > >> Since Christmas is coming, I think this feature may be in danger of
> > >> following the pace of
> > >> our hw version releases, so I sincerely request that you please review
> > >> it as soon as possible.
> > >>
> > >> Thanks!
> > >>
> > >> 在 2023/12/12 下午5:30, Heng Qi 写道:
> > >>>
> > >>> 在 2023/12/12 下午5:23, Heng Qi 写道:
> >
> > 在 2023/12/12 下午4:44, Michael S. Tsirkin 写道:
> > > On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote:
> > >> 在 2023/12/12 上午12:35, Michael S. Tsirkin 写道:
> > >>> On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote:
> > virtio-net works in a virtualized system and is somewhat
> > different from
> > physical nics. One of the differences is that to save virtio device
> > resources, rx may receive partially checksummed packets. However,
> > XDP may
> > cause partially checksummed packets to be dropped.
> > So XDP loading currently conflicts with the feature
> > VIRTIO_NET_F_GUEST_CSUM.
> >
> > This patch lets the device to supply fully checksummed packets to
> > the driver.
> > Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the
> > benefits of
> > device validation checksum.
> >
> > In addition, implementation of some performant devices always do
> > not generate
> > partially checksummed packets, but the standard driver still need
> > to clear
> > VIRTIO_NET_F_GUEST_CSUM when XDP is there.
> > A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the
> > above
> > situation, which provides the driver with configurable offload.
> >
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
在 2023/12/19 下午3:53, Jason Wang 写道:
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
在 2023/12/18 上午11:10, Jason Wang 写道:
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
I don't see that in the spec.
Am I missing something? [1][2]
[1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
*enable receive checksum*, large receive offload and ECN support which
are the input equivalents of the transmit checksum, transmit
segmentation *offloading* and ECN features, as described in 5.1.6.2.
[2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
Hi Jason, please see below.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct.
Yes. This mapping is because the PARTIAL checksum usually does not go
through the physical wire,
so it is considered safe, and the checksum does not need to be verified.
So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
Yes. The checksum of a packet without NEEDS_CSUM or has not been
verified (DATA_VALID set) is unreliable.
This patch doesn't break that.
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL.
Yes. Both cannot be set or appear at the same time.
And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
I think you are talking about this commit:
10a8d94a95742bb15b4e617ee9884bb4381362be
But in fact, as your commit log says, I think this is a hack. Host nics
does not fall into the scope of virtio spec?
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Sorry. I cannot follow this view.
1. First of all, VIRTIO_NET_F_GUEST_CSUM (partial csum is not considered
now, because we have no dispute about it) does represent the device's
ability to calculate and verify checksums.
Its ability to handle partial checksums (NEEDS_CSUM) is just a special
processing of virtio, the Linux kernel never had a netdev feature for
partial checksum handling.
1.1 VIRTIO_NET_F_GUEST_{TSO4, TSO6, USO4} etc. depend on
VIRTIO_NET_F_GUEST_CSUM.
The reason for being relied upon is not that they are related
to NEEDS_CSUM, but that the device needs to recalculate and verify the
checksum of the packets when merging the packets.
See netdev_fix_features:
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
dev->features |= NETIF_F_RXCSUM;
- netdev_fix_features ->
if (!(features & NETIF_F_RXCSUM)) {
/* NETIF_F_GRO_HW implies doing RXCSUM since every packet
* successfully merged by hardware must also have the
* checksum verified by hardware. If the user does not
* want to enable RXCSUM, logically, we should disable
GRO_HW.
*/
if (features & NETIF_F_GRO_HW) {
netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since
no RXCSUM feature.\n");
features &= ~NETIF_F_GRO_HW;
}
}
1.2 See NETIF_F_RXCSUM_BIT /* Receive checksumming offload */
Most device drivers use NETIF_RX_CSUM to indicate device checksum
capabilities,
and the corresponding offload can be dynamically switched on and
off by user tools such as ethtool.
2. The implementation of vhost-user, large-scale commercial virtio
device that I know of, and other devices are
completely designed and implemented in accordance with virtio 1.0 and
later. They are comply with the current
specifications and the Linux kernel's definition of NETIF_F_RXCSUM
(VIRTIO_NET_F_GUEST_CSUM).
Thanks!
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
>
>
>
> 在 2023/12/18 上午11:10, Jason Wang 写道:
> > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
> >> Hi all!
> >>
> >> I would like to ask if anyone has any comments on this version, if so
> >> please let me know!
> >> If not, I will collect Michael's comments and publish a new version next
> >> Monday.
> > I have a dumb question. (And sorry if I asked it before)
> >
> > Looking at the spec and code. It looks to me DATA_VALID could be set
> > without GUEST_CSUM.
>
> I don't see that in the spec.
> Am I missing something? [1][2]
>
> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> VIRTIO_NET_HDR_F_DATA_VALID bit in 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.
> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> *enable receive checksum*, large receive offload and ECN support which
> are the input equivalents of the transmit checksum, transmit
> segmentation *offloading* and ECN features, as described in 5.1.6.2.
>
> [2] 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.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct. So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL. And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Thanks
>
> I think the reason why the feature bit is not checked in the code is
> because the check is omitted because it is on a per-packet basis,
> just like the reason why supported_valid_types is not needed as
> discussed in the v4 version threads. It is not unnecessary.
>
> Thanks!
>
> >
> > If yes, why do we need to bother here? If we disable GUEST_CSUM, the
> > packet will contain checksum. And if the device sets DATA_VALID, it
> > means the checksum is validated.
> >
> > Thanks
> >
> >
> >
> >> Since Christmas is coming, I think this feature may be in danger of
> >> following the pace of
> >> our hw version releases, so I sincerely request that you please review
> >> it as soon as possible.
> >>
> >> Thanks!
> >>
> >> 在 2023/12/12 下午5:30, Heng Qi 写道:
> >>>
> >>> 在 2023/12/12 下午5:23, Heng Qi 写道:
>
> 在 2023/12/12 下午4:44, Michael S. Tsirkin 写道:
> > On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote:
> >> 在 2023/12/12 上午12:35, Michael S. Tsirkin 写道:
> >>> On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote:
> virtio-net works in a virtualized system and is somewhat
> different from
> physical nics. One of the differences is that to save virtio device
> resources, rx may receive partially checksummed packets. However,
> XDP may
> cause partially checksummed packets to be dropped.
> So XDP loading currently conflicts with the feature
> VIRTIO_NET_F_GUEST_CSUM.
>
> This patch lets the device to supply fully checksummed packets to
> the driver.
> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the
> benefits of
> device validation checksum.
>
> In addition, implementation of some performant devices always do
> not generate
> partially checksummed packets, but the standard driver still need
> to clear
> VIRTIO_NET_F_GUEST_CSUM when XDP is there.
> A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the
> above
> situation, which provides the driver with configurable offload.
> If the offload is enabled, then the device must deliver fully
> checksummed packets to the driver and may validate the checksum.
>
> Use case example:
> If VIRTIO_NET_F_GUEST_FULLY_CSUM is negotiated and the offload is
> enabled,
> after XDP processes a fully checksummed packet, the
> VIRTIO_NET_HDR_F_DATA_VALID bit
> is retained if the device has validated its
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
在 2023/12/18 上午11:10, Jason Wang 写道: On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote: Hi all! I would like to ask if anyone has any comments on this version, if so please let me know! If not, I will collect Michael's comments and publish a new version next Monday. I have a dumb question. (And sorry if I asked it before) Looking at the spec and code. It looks to me DATA_VALID could be set without GUEST_CSUM. I don't see that in the spec. Am I missing something? [1][2] [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_DATA_VALID bit in 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. Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features *enable receive checksum*, large receive offload and ECN support which are the input equivalents of the transmit checksum, transmit segmentation *offloading* and ECN features, as described in 5.1.6.2. [2] 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. I think the reason why the feature bit is not checked in the code is because the check is omitted because it is on a per-packet basis, just like the reason why supported_valid_types is not needed as discussed in the v4 version threads. It is not unnecessary. Thanks! If yes, why do we need to bother here? If we disable GUEST_CSUM, the packet will contain checksum. And if the device sets DATA_VALID, it means the checksum is validated. Thanks Since Christmas is coming, I think this feature may be in danger of following the pace of our hw version releases, so I sincerely request that you please review it as soon as possible. Thanks! 在 2023/12/12 下午5:30, Heng Qi 写道: 在 2023/12/12 下午5:23, Heng Qi 写道: 在 2023/12/12 下午4:44, Michael S. Tsirkin 写道: On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote: 在 2023/12/12 上午12:35, Michael S. Tsirkin 写道: On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote: virtio-net works in a virtualized system and is somewhat different from physical nics. One of the differences is that to save virtio device resources, rx may receive partially checksummed packets. However, XDP may cause partially checksummed packets to be dropped. So XDP loading currently conflicts with the feature VIRTIO_NET_F_GUEST_CSUM. This patch lets the device to supply fully checksummed packets to the driver. Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of device validation checksum. In addition, implementation of some performant devices always do not generate partially checksummed packets, but the standard driver still need to clear VIRTIO_NET_F_GUEST_CSUM when XDP is there. A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the above situation, which provides the driver with configurable offload. If the offload is enabled, then the device must deliver fully checksummed packets to the driver and may validate the checksum. Use case example: If VIRTIO_NET_F_GUEST_FULLY_CSUM is negotiated and the offload is enabled, after XDP processes a fully checksummed packet, the VIRTIO_NET_HDR_F_DATA_VALID bit is retained if the device has validated its checksum, resulting in the guest not needing to validate the checksum again. This is useful for guests: 1. Bring the driver advantages such as cpu savings. 2. For devices that do not generate partially checksummed packets themselves, XDP can be loaded in the driver without modifying the hardware behavior. Several solutions have been discussed in the previous proposal[1]. After historical discussion, we have tried the method proposed by Jason[2], but some complex scenarios and challenges are difficult to deal with. We now return to the method suggested in [1]. [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html [2] https://lore.kernel.org/all/20230628030506.2213-1-hen...@linux.alibaba.com/ Signed-off-by: Heng Qi Reviewed-by: Xuan Zhuo --- v4->v5: - Remove the modification to the GUEST_CSUM. - The description of this feature has been reorganized for greater clarity. 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| 74 +++-- device-types/net/device-conformance.tex | 1 + device-types/net/driver-conformance.tex | 1 + introduction.tex| 3 + 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/device-types/net/description.tex b/device-types/net/description.tex index aff5e08..ab6c13d 100644 ---
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
>
> Hi all!
>
> I would like to ask if anyone has any comments on this version, if so
> please let me know!
> If not, I will collect Michael's comments and publish a new version next
> Monday.
I have a dumb question. (And sorry if I asked it before)
Looking at the spec and code. It looks to me DATA_VALID could be set
without GUEST_CSUM.
If yes, why do we need to bother here? If we disable GUEST_CSUM, the
packet will contain checksum. And if the device sets DATA_VALID, it
means the checksum is validated.
Thanks
>
> Since Christmas is coming, I think this feature may be in danger of
> following the pace of
> our hw version releases, so I sincerely request that you please review
> it as soon as possible.
>
> Thanks!
>
> 在 2023/12/12 下午5:30, Heng Qi 写道:
> >
> >
> > 在 2023/12/12 下午5:23, Heng Qi 写道:
> >>
> >>
> >> 在 2023/12/12 下午4:44, Michael S. Tsirkin 写道:
> >>> On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote:
>
> 在 2023/12/12 上午12:35, Michael S. Tsirkin 写道:
> > On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote:
> >> virtio-net works in a virtualized system and is somewhat
> >> different from
> >> physical nics. One of the differences is that to save virtio device
> >> resources, rx may receive partially checksummed packets. However,
> >> XDP may
> >> cause partially checksummed packets to be dropped.
> >> So XDP loading currently conflicts with the feature
> >> VIRTIO_NET_F_GUEST_CSUM.
> >>
> >> This patch lets the device to supply fully checksummed packets to
> >> the driver.
> >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the
> >> benefits of
> >> device validation checksum.
> >>
> >> In addition, implementation of some performant devices always do
> >> not generate
> >> partially checksummed packets, but the standard driver still need
> >> to clear
> >> VIRTIO_NET_F_GUEST_CSUM when XDP is there.
> >> A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the
> >> above
> >> situation, which provides the driver with configurable offload.
> >> If the offload is enabled, then the device must deliver fully
> >> checksummed packets to the driver and may validate the checksum.
> >>
> >> Use case example:
> >> If VIRTIO_NET_F_GUEST_FULLY_CSUM is negotiated and the offload is
> >> enabled,
> >> after XDP processes a fully checksummed packet, the
> >> VIRTIO_NET_HDR_F_DATA_VALID bit
> >> is retained if the device has validated its checksum, resulting
> >> in the guest
> >> not needing to validate the checksum again. This is useful for
> >> guests:
> >> 1. Bring the driver advantages such as cpu savings.
> >> 2. For devices that do not generate partially checksummed
> >> packets themselves,
> >>XDP can be loaded in the driver without modifying the
> >> hardware behavior.
> >>
> >> Several solutions have been discussed in the previous proposal[1].
> >> After historical discussion, we have tried the method proposed by
> >> Jason[2],
> >> but some complex scenarios and challenges are difficult to deal
> >> with.
> >> We now return to the method suggested in [1].
> >>
> >> [1]
> >> https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> >>
> >> [2]
> >> https://lore.kernel.org/all/20230628030506.2213-1-hen...@linux.alibaba.com/
> >>
> >> Signed-off-by: Heng Qi
> >> Reviewed-by: Xuan Zhuo
> >> ---
> >> v4->v5:
> >> - Remove the modification to the GUEST_CSUM.
> >> - The description of this feature has been reorganized for
> >> greater clarity.
> >>
> >> 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| 74
> >> +++--
> >>device-types/net/device-conformance.tex | 1 +
> >>device-types/net/driver-conformance.tex | 1 +
> >>introduction.tex| 3 +
> >>4 files changed, 73 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/device-types/net/description.tex
> >> b/device-types/net/description.tex
> >> index aff5e08..ab6c13d 100644
> >> --- a/device-types/net/description.tex
> >> +++ b/device-types/net/description.tex
> >> @@ -122,6 +122,9 @@ \subsection{Feature bits}\label{sec:Device
> >> Types / Network Device / Feature bits
> >>device with the same MAC
[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
Hi all!
I would like to ask if anyone has any comments on this version, if so
please let me know!
If not, I will collect Michael's comments and publish a new version next
Monday.
Since Christmas is coming, I think this feature may be in danger of
following the pace of
our hw version releases, so I sincerely request that you please review
it as soon as possible.
Thanks!
在 2023/12/12 下午5:30, Heng Qi 写道:
在 2023/12/12 下午5:23, Heng Qi 写道:
在 2023/12/12 下午4:44, Michael S. Tsirkin 写道:
On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote:
在 2023/12/12 上午12:35, Michael S. Tsirkin 写道:
On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote:
virtio-net works in a virtualized system and is somewhat
different from
physical nics. One of the differences is that to save virtio device
resources, rx may receive partially checksummed packets. However,
XDP may
cause partially checksummed packets to be dropped.
So XDP loading currently conflicts with the feature
VIRTIO_NET_F_GUEST_CSUM.
This patch lets the device to supply fully checksummed packets to
the driver.
Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the
benefits of
device validation checksum.
In addition, implementation of some performant devices always do
not generate
partially checksummed packets, but the standard driver still need
to clear
VIRTIO_NET_F_GUEST_CSUM when XDP is there.
A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the
above
situation, which provides the driver with configurable offload.
If the offload is enabled, then the device must deliver fully
checksummed packets to the driver and may validate the checksum.
Use case example:
If VIRTIO_NET_F_GUEST_FULLY_CSUM is negotiated and the offload is
enabled,
after XDP processes a fully checksummed packet, the
VIRTIO_NET_HDR_F_DATA_VALID bit
is retained if the device has validated its checksum, resulting
in the guest
not needing to validate the checksum again. This is useful for
guests:
1. Bring the driver advantages such as cpu savings.
2. For devices that do not generate partially checksummed
packets themselves,
XDP can be loaded in the driver without modifying the
hardware behavior.
Several solutions have been discussed in the previous proposal[1].
After historical discussion, we have tried the method proposed by
Jason[2],
but some complex scenarios and challenges are difficult to deal
with.
We now return to the method suggested in [1].
[1]
https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
[2]
https://lore.kernel.org/all/20230628030506.2213-1-hen...@linux.alibaba.com/
Signed-off-by: Heng Qi
Reviewed-by: Xuan Zhuo
---
v4->v5:
- Remove the modification to the GUEST_CSUM.
- The description of this feature has been reorganized for
greater clarity.
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 | 74
+++--
device-types/net/device-conformance.tex | 1 +
device-types/net/driver-conformance.tex | 1 +
introduction.tex | 3 +
4 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/device-types/net/description.tex
b/device-types/net/description.tex
index aff5e08..ab6c13d 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -122,6 +122,9 @@ \subsection{Feature bits}\label{sec:Device
Types / Network Device / Feature bits
device with the same MAC address.
\item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and
duplex.
+
+\item[VIRTIO_NET_F_GUEST_FULLY_CSUM (64)] Device delivers fully
checksummed packets
+ to the driver and may validate the checksum.
\end{description}
I propose
VIRTIO_NET_F_GUEST_CSUM_COMPLETE
instead.
Can I ask here if *complete* in VIRTIO_NET_F_GUEST_CSUM_COMPLETE and
CHECKSUM_COMPLETE mean the same thing?
If so, it seems that it's no longer the same as the description of
this
patch.
Oh. I thought it is. Then I guess I misunderstand what this patch is
supposed to be doing, again.
Here's some context:
From the perspective of the Linux kernel, the GUEST_CSUM feature is
negotiated to support
(1) CHECKSUM_NONE, (2) CHECKSUM_UNNECESSARY, (3) CHECKSUM_PARTIAL,
which
respectively correspond to (1) the device does not validate the
packet checksum (may not have
the ability to validate some protocols or does not recognize the
packet); (2) the device has verified
the data packet, then sets DATA_VALID bit in flags; (3) In order to
save device resources, VMs
on the same host deliver partially checksummed packets, and
NEEDS_CSUM bit is set in flags.
GUEST_FULLY_CSUM did not change the
