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

[Jason Wang <jasowang@redhat.com>]

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?

>
> 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!
>
> >
> >
>