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

On Thu, Jan 11 2024, Heng Qi <[email protected]> wrote:

> 在 2024/1/4 下午12:47, Heng Qi 写道:
>>
>>
>> 在 2024/1/3 下午2:35, Heng Qi 写道:
>>> There is a historical error in virtio spec:
>>>    "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST* 
>>> set flags
>>>     to zero and SHOULD supply a fully checksummed packet to the driver."
>>>
>>> Currently in Linux and virtio-related implementations, the device 
>>> validates
>>> the packet checksum and sets DATA_VALID regardless of whether
>>> VIRTIO_NET_F_GUEST_CSUM is negotiated.
>>>
>>> Please refer to the following summary and thread[1] for details and 
>>> reasons:
>>>
>>> Summary
>>> =============
>>> 1. GUEST_CSUM at virtio spec 0.95 is intended to be compatible with 
>>> partially
>>> checksummed packets (NEEDS_CSUM <-> CHECKSUM_PARTIAL). So GUEST_CSUM 
>>> is mapped
>>> to NETIF_F_RXCSUM.
>>> GUEST_CSUM only indicates whether the driver handles partially 
>>> checksummed packets.
>>> When XDP is loaded, the GUEST_CSUM's offload will be disabled, which 
>>> means that
>>> packets have NEEDS_CSUM set will be dropped, and packets have 
>>> DATA_VALID set will
>>> still be received.
>>>
>>> 2. When DATA_VALID was added to Linux in 2011[2] and virtio1.0, it 
>>> was actually
>>> expected that rx checksum offload (the driver can set 
>>> CHECKSUM_UNNECESSARY) had
>>> nothing to do with whether GUEST_CSUM was negotiated. But due to an 
>>> error, below
>>> desctiption was added incorrectly:
>>>    "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST* 
>>> set flags
>>>     to zero and SHOULD supply a fully checksummed packet to the driver."
>>>
>>> 3. We now hope to correct this error. Let the setting of DATA_VALID 
>>> not be
>>> controlled by whether GUEST_CSUM is negotiated, but only controlled 
>>> by whether
>>> rx checksum offload is enabled on the OS side. The state of this rx 
>>> checksum
>>> offload is not aware of the device.
>>>
>>> 4. [Optional] NETIF_F_RXCSUM corresponding to rx checksum offload 
>>> should be
>>> added to dev->hw_features. When the user turns off rx checksum 
>>> offload through
>>> ethtool -K, neither NEEDS_CSUM nor DATA_VALID should be taken care 
>>> of, that is,
>>> all packets will be marked as CHECKSUM_NONE.
>>>
>>> Related Link
>>> =============
>>> [1] 
>>> https://lists.oasis-open.org/archives/virtio-comment/202312/msg00135.html
>>> [2] 10a8d94a9574 ("virtio_net: introduce VIRTIO_NET_HDR_F_DATA_VALID")
>>>
>>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/185
>>
>> Hi Michael, Cornelia.
>>
>> Could you please open a voting for this fix?
>
> Hi Michael and Cornelia.
>
> Sincerely asking if you missed this message?

I've been back from end-of-year PTO only this week...

I'm currently waiting for a confirmed updated schedule for the
infrastructure migration, as I don't want to risk having the platform
out of order in the middle of the voting period.

>
> In addition, Jason recently responded with his reviewed-by tag.
> Do I need to publish a new version carrying his tag (thanks Jason)?

No, we can add his R-b when pushing to git.


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]