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