[virtio-comment] 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

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

在 2023/12/21 上午9:41, Jason Wang 写道:
> On Wed, Dec 20, 2023 at 5:31 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>
>>
>> 在 2023/12/20 下午3:35, Michael S. Tsirkin 写道:
>>> On Wed, Dec 20, 2023 at 02:30:01PM +0800, Heng Qi wrote:
>>>> But why are we discussing this?
>>> I think basically at this point everyone is confused about what
>>> the feature does. right now we have packets
>>> with
>>> #define VIRTIO_NET_HDR_F_NEEDS_CSUM     1       -> partial
>>> #define VIRTIO_NET_HDR_F_DATA_VALID     2       -> unnecessary
>>> and packets without either                    -> none
>>>
>>> if both 1 and 2 are set then linux uses VIRTIO_NET_HDR_F_NEEDS_CSUM but
>>> I am not sure it's not a mistake. Maybe it does not matter.
>>>
>>> What does this new thing do? So far all we have is "XDP will turn it on"
>>> which is not really sufficient. I assumed it somehow replaces
>>> partial with complete. That would make sense for many reasons,
>>> for example the checksum fields in the header can be reused
>>> for other purposes. But maybe not?
>>
>> Hello Jaosn and Michael. I've summarized our discussion so far, so check
>> it out below. Thank you very much!
>>
>>   From the nic perspective, I think Jason's statement is correct, the
>> nic's checksum capability and setting DATA_VALID in flags
>> should not be determined by GUEST_CSUM feature. As long as the rx
>> checksum offload is turned on, DATA_VALID
>> should be set. (Though we now bind GUEST_CSUM negotiation with rx
>> checksum offload.)
> I think we can fix this in the driver. Probably by just advertising
> RXCSUM regardless of GUEST_CSUM?

Right.

>
>> Therefore, we need to pay attention to the information of rx checksum
>> offload. Please check it out:
>>
>> Devices that comply with the below description are said to be existing
>> devices:
>>       "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST*
>> set flags to zero and SHOULD supply a fully checksummed packet to the
>> driver."
>>
>> As suggested by Jason, devices that comply with the below description
>> are said to be new devices:
>>       "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MAY* set
>> flags to zero and SHOULD supply a fully checksummed packet to the driver."
>>
>>
>> 1. Rx checksum offload is turned on
>> GUEST_CSUM feature is not negotiated. (now it is only used to indicate
>> whether the driver can handle partially checksummed packets)
>>      a. Existing devices continue to set flags to 0;
> Note that existing devices can set DATA_VALID regardless of rx csum.

Right.

>
>>      b. New devices may validate the packets and have flags set to
>> DATA_VALID;
>>      c. Migration.
>>          Migration of existing devices continues to check GUEST_CSUM
>> feature and rx checksum offload;
>>          Migration of new devices only check rx checksum offload;
>>          Without updating the existing migration management and control
>> system, existing devices cannot be migrated to new devices, and new
>> devices cannot be migrated to existing devices.
> Yes.
>
>>      d. How offload should be controlled now needs attention. Should
>> CTRL_GUEST_OFFLOADS still issue GUEST_CSUM feature bit to control the rx
>> checksum offload?
> So the only thing we need to do for the driver is, when rx csum is disabled:
>
> 1) drop packets with NEEDS_CSUM
> 2) use CHECKSUM_NONE for the rest
>
> ?

YES.

>
>> 2. The new FULLY_CSUM feature must disable NEEDS_CSUM.
>> The device may set DATA_VALID regardless of whether FULLY_CSUM or
>> GUEST_CSUM is negotiated.
>>      a. Rx fully checksum offload is still controlled by
>> CTRL_GUEST_OFFLOADS carrying GUEST_FULLY_CSUM.
>>      b. When the rx device receives a partially checksummed packet, it
>> should calculate the checksum and delivering a fully checksummed packet
>> to the driver.
>>
>>
>> So now, if we modify the existing spec as Jason suggested, I think it's OK.
>> But we need to find out how to control rx checksum offload. WDYT?
> See above, the driver can just not set CHECKSUM_UNNECESSARY in this case.

I think what you are saying here is that CHECKSUM_UNNECESSARY cannot be 
set by the driver when rx checksum offload is turned off.

Thanks！

>
> Thanks
>
>> Thanks!
>>
>>>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/