[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: 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 <hengqi@linux.alibaba.com> wrote: > > > > å 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ï Right. Thanks > > > > > Thanks > > > >> Thanks! > >> > >>> >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]