[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Re: [PATCH v4 1/2] virtio-net: update description for VIRTIO_NET_F_GUEST_CSUM.
On Thu, Dec 7, 2023 at 12:52âPM Heng Qi <hengqi@linux.alibaba.com> wrote: > > > > å 2023/12/6 äå5:03, Heng Qi åé: > > > > > > å 2023/12/6 äå4:46, Jason Wang åé: > >> On Wed, Dec 6, 2023 at 4:08âPM Xuan Zhuo <xuanzhuo@linux.alibaba.com> > >> wrote: > >>> On Wed, 6 Dec 2023 12:36:53 +0800, Jason Wang <jasowang@redhat.com> > >>> wrote: > >>>> On Wed, Dec 6, 2023 at 10:21âAM Heng Qi <hengqi@linux.alibaba.com> > >>>> wrote: > >>>>> > >>>>> > >>>>> å 2023/12/5 äå10:45, Michael S. Tsirkin åé: > >>>>>> On Tue, Dec 05, 2023 at 10:18:32PM +0800, Heng Qi wrote: > >>>>>>> å 2023/12/5 äå11:52, Jason Wang åé: > >>>>>>>> On Mon, Dec 4, 2023 at 5:34âPM Heng Qi > >>>>>>>> <hengqi@linux.alibaba.com> wrote: > >>>>>>>>> å 2023/12/4 äå5:05, Michael S. Tsirkin åé: > >>>>>>>>>> On Mon, Dec 04, 2023 at 04:59:49PM +0800, Jason Wang wrote: > >>>>>>>>>>> On Mon, Dec 4, 2023 at 4:53âPM Michael S. Tsirkin > >>>>>>>>>>> <mst@redhat.com> wrote: > >>>>>>>>>>>> On Mon, Dec 04, 2023 at 04:49:46PM +0800, Jason Wang wrote: > >>>>>>>>>>>>> On Mon, Dec 4, 2023 at 3:37âPM Heng Qi > >>>>>>>>>>>>> <hengqi@linux.alibaba.com> wrote: > >>>>>>>>>>>>>> å 2023/12/4 äå3:18, Jason Wang åé: > >>>>>>>>>>>>>>> On Fri, Dec 1, 2023 at 3:16âPM Heng Qi > >>>>>>>>>>>>>>> <hengqi@linux.alibaba.com> wrote: > >>>>>>>>>>>>>>>> å 2023/12/1 äå3:05, Jason Wang åé: > >>>>>>>>>>>>>>>>> On Fri, Dec 1, 2023 at 2:30âPM Heng Qi > >>>>>>>>>>>>>>>>> <hengqi@linux.alibaba.com> wrote: > >>>>>>>>>>>>>>>>>> å 2023/12/1 äå2:24, Heng Qi åé: > >>>>>>>>>>>>>>>>>>> å 2023/12/1 äå1:18, Jason Wang åé: > >>>>>>>>>>>>>>>>>>>> On Wed, Nov 29, 2023 at 4:23âPM Heng Qi > >>>>>>>>>>>>>>>>>>>> <hengqi@linux.alibaba.com> > >>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>> å 2023/11/29 äå4:00, Jason Wang åé: > >>>>>>>>>>>>>>>>>>>>>> On Tue, Nov 28, 2023 at 4:08âPM Heng Qi > >>>>>>>>>>>>>>>>>>>>>> <hengqi@linux.alibaba.com> > >>>>>>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>>>> To prevent readers from misunderstanding that > >>>>>>>>>>>>>>>>>>>>>>> the driver can > >>>>>>>>>>>>>>>>>>>>>>> only handles packets with partial checksum when > >>>>>>>>>>>>>>>>>>>>>>> VIRTIO_NET_F_GUEST_CSUM is negotiated, we update > >>>>>>>>>>>>>>>>>>>>>>> the description. > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > >>>>>>>>>>>>>>>>>>>>>>> --- > >>>>>>>>>>>>>>>>>>>>>>> device-types/net/description.tex | 2 +- > >>>>>>>>>>>>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 > >>>>>>>>>>>>>>>>>>>>>>> deletion(-) > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> diff --git a/device-types/net/description.tex > >>>>>>>>>>>>>>>>>>>>>>> b/device-types/net/description.tex > >>>>>>>>>>>>>>>>>>>>>>> index aff5e08..529f470 100644 > >>>>>>>>>>>>>>>>>>>>>>> --- a/device-types/net/description.tex > >>>>>>>>>>>>>>>>>>>>>>> +++ b/device-types/net/description.tex > >>>>>>>>>>>>>>>>>>>>>>> @@ -38,7 +38,7 @@ \subsection{Feature > >>>>>>>>>>>>>>>>>>>>>>> bits}\label{sec:Device Types > >>>>>>>>>>>>>>>>>>>>>>> / Network Device / Feature bits > >>>>>>>>>>>>>>>>>>>>>>> \begin{description} > >>>>>>>>>>>>>>>>>>>>>>> \item[VIRTIO_NET_F_CSUM (0)] Device handles > >>>>>>>>>>>>>>>>>>>>>>> packets with > >>>>>>>>>>>>>>>>>>>>>>> partial checksum offload. > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> -\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver > >>>>>>>>>>>>>>>>>>>>>>> handles packets with > >>>>>>>>>>>>>>>>>>>>>>> partial checksum. > >>>>>>>>>>>>>>>>>>>>>>> +\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver > >>>>>>>>>>>>>>>>>>>>>>> handles packets with > >>>>>>>>>>>>>>>>>>>>>>> partial checksum or full checksum. > >>>>>>>>>>>>>>>>>>>>>> So patch 2 said > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> " > >>>>>>>>>>>>>>>>>>>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver > >>>>>>>>>>>>>>>>>>>>>> handles packets with > >>>>>>>>>>>>>>>>>>>>>> full checksum. > >>>>>>>>>>>>>>>>>>>>>> \end{description} > >>>>>>>>>>>>>>>>>>>>>> " > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> Is there any difference between the two "full > >>>>>>>>>>>>>>>>>>>>>> checksum" here? > >>>>>>>>>>>>>>>>>>>>> There's no difference. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> The core is that VIRTIO_NET_F_GUEST_FULL_CSUM > >>>>>>>>>>>>>>>>>>>>> means that the driver > >>>>>>>>>>>>>>>>>>>>> "can > >>>>>>>>>>>>>>>>>>>>> only" handle packets with full checksum. > >>>>>>>>>>>>>>>>>>>> This seems to be odd. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Driver can always handle packet with full checksum, > >>>>>>>>>>>>>>>>>>>> no? > >>>>>>>>>>>>>>>>>>> Yes. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> I meant it > >>>>>>>>>>>>>>>>>>>> will be then to be functional equivalent to ! > >>>>>>>>>>>>>>>>>>>> VIRTIO_NET_F_GUEST_FULL_CSUM? > >>>>>>>>>>>>>>>>>>> Are you referring to > >>>>>>>>>>>>>>>>>>> "functional equivalent to !VIRTIO_NET_F_GUEST_CSUM" ? > >>>>>>>>>>>>>>>>>> Sorry, this is a typo. I meant > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Are you referring to > >>>>>>>>>>>>>>>>>> "functional equivalent to > >>>>>>>>>>>>>>>>>> !VIRTIO_NET_F_GUEST_FULL_CSUM" ? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> If so, I think it's no. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Maybe a description similar to the following would > >>>>>>>>>>>>>>>>>>> be more clearer: > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] Driver > >>>>>>>>>>>>>>>>>>> does not handle > >>>>>>>>>>>>>>>>>>> packets with partial checksum. > >>>>>>>>>>>>>>>>> I may miss something here, but what's the difference > >>>>>>>>>>>>>>>>> between > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> VIRTIO_NET_F_GUEST_FULL_CSUM > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> and > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> !VIRTIO_NET_F_GUEST_CSUM? > >>>>>>>>>>>>>>>> From the device perspective: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> If !VIRTIO_NET_F_GUEST_CSUM, the device delivers > >>>>>>>>>>>>>>>> packets with full > >>>>>>>>>>>>>>>> checksum to the driver, > >>>>>>>>>>>>>>>> but the device can not validate the checksum for these > >>>>>>>>>>>>>>>> packets. That is, > >>>>>>>>>>>>>>>> the flags in virtio-net-hdr > >>>>>>>>>>>>>>>> will not contain _DATA_VALID, and the driver or stack > >>>>>>>>>>>>>>>> needs to validate > >>>>>>>>>>>>>>>> these packets. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> If VIRTIO_NET_F_GUEST_FULL_CSUM, the device delivers > >>>>>>>>>>>>>>>> packets with full > >>>>>>>>>>>>>>>> checksum to the driver, > >>>>>>>>>>>>>>>> and the device can validate the checksum for these > >>>>>>>>>>>>>>>> packets. That is, the > >>>>>>>>>>>>>>>> flags in virtio-net-hdr > >>>>>>>>>>>>>>>> will contain _DATA_VALID, > >>>>>>>>>>>>>>> I think DATA_VALID is optional here as device can't > >>>>>>>>>>>>>>> recognize all type > >>>>>>>>>>>>>>> of protocols. > >>>>>>>>>>>>>> Yes, you are right, so I used "device *can*" here. Which > >>>>>>>>>>>>>> packet types > >>>>>>>>>>>>>> the device recognizes or validates > >>>>>>>>>>>>>> depends on the device's implementation. This is also the > >>>>>>>>>>>>>> current > >>>>>>>>>>>>>> practice of GUEST_CSUM. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> and the driver or stack does not need to > >>>>>>>>>>>>>>>> validate these packets. > >>>>>>>>>>>>>>> Ok, so I think there're something that is subtle here, > >>>>>>>>>>>>>> Ok, I see. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> and that's why > >>>>>>>>>>>>>>> I'm asking here: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1) "Driver does not handle packets with partial > >>>>>>>>>>>>>>> checksum" is not > >>>>>>>>>>>>>>> accurate, !GUEST_CUSM also fit for this definition. > >>>>>>>>>>>>>>> 2) "Driver handles packets with full checksum" is kind > >>>>>>>>>>>>>>> of ambiguous as > >>>>>>>>>>>>>>> it doesn't say whether or not the packet has been > >>>>>>>>>>>>>>> validated or not. > >>>>>>>>>>>>>> Maybe the description below would be less subtle? > >>>>>>>>>>>>>> +\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles > >>>>>>>>>>>>>> packets with partial > >>>>>>>>>>>>>> checksum or full checksum. > >>>>>>>>>>>>> I'd suggest to leave it as is. As I didn't find any issue > >>>>>>>>>>>>> since even > >>>>>>>>>>>>> with DATA_VALID. Did you? > >>>>>>>>>>>>> > >>>>>>>>>>>>>> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (64)] The driver > >>>>>>>>>>>>>> handles packets > >>>>>>>>>>>>>> with full checksum, > >>>>>>>>>>>>>> and the device optionally validates the packet's checksum. > >>>>>>>>>>>>> Or maybe something like (not a native speaker) > >>>>>>>>>>>>> > >>>>>>>>>>>>> The driver handles packets with full checksum which the > >>>>>>>>>>>>> device has > >>>>>>>>>>>>> already validated. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Thanks > >>>>>>>>>>>> I feel we just need a proper definition of what does "full > >>>>>>>>>>>> checksum" > >>>>>>>>>>>> mean in this context. It is used but not defined. > >>>>>>>>>>>> Assume this feature was negotiated. > >>>>>>>>>>>> My understanding is that this is just like > >>>>>>>>>>>> VIRTIO_NET_F_GUEST_CSUM > >>>>>>>>>>>> but certain values in the header are then disallowed? Which? > >>>>>>>>>>>> This should be in the spec. > >>>>>>>>>>> Yes, I think it is probably the headers that DATA_VALID can > >>>>>>>>>>> work. We > >>>>>>>>>>> never define it in the past. > >>>>>>>>>>> > >>>>>>>>>>> E.g in the Linux we map DATA_VALID to CHECKSUM_UNNECESSARY, > >>>>>>>>>>> but it can > >>>>>>>>>>> only work for some specific protocols: > >>>>>>>>>>> > >>>>>>>>>>> """ > >>>>>>>>>>> * %CHECKSUM_UNNECESSARY is applicable to following > >>>>>>>>>>> protocols: > >>>>>>>>>>> * > >>>>>>>>>>> * - TCP: IPv6 and IPv4. > >>>>>>>>>>> * - UDP: IPv4 and IPv6. A device may apply > >>>>>>>>>>> CHECKSUM_UNNECESSARY to a > >>>>>>>>>>> * zero UDP checksum for either IPv4 or IPv6, the > >>>>>>>>>>> networking stack > >>>>>>>>>>> * may perform further validation in this case. > >>>>>>>>>>> * - GRE: only if the checksum is present in the > >>>>>>>>>>> header. > >>>>>>>>>>> * - SCTP: indicates the CRC in SCTP header has been > >>>>>>>>>>> validated. > >>>>>>>>>>> * - FCOE: indicates the CRC in FC frame has been > >>>>>>>>>>> validated. > >>>>>>>>>>> """ > >>>>>>>>>>> > >>>>>>>>>>> I'm not sure whether it's just fine to duplicate the > >>>>>>>>>>> definition or > >>>>>>>>>>> it's too late to define any now. > >>>>>>>>>> I think it's mostly harmless for other protocols. > >>>>>>>>> I'm not sure if this should be defined by a new FULL_CSUM > >>>>>>>>> feature. > >>>>>>>>> This seems to be an issue with GUEST_CSUM. > >>>>>>>>> > >>>>>>>>> I think we should supplement these with a new patch for > >>>>>>>>> GUEST_CSUM? > >>>>>>>> Probably. My understanding is: > >>>>>>>> > >>>>>>>> You want to reuse DATA_VALID here, so we need to stick to a > >>>>>>>> consistent > >>>>>>>> semantic for GUEST_CUSM and FULL_CSUM. So we need a definition of > >>>>>>>> "full csum" or what kind of packet could DATA_VALID work here. > >>>>>>> I agree, we can be clear about what types of packets DATA_VALID > >>>>>>> might > >>>>>>> cover, e.g. TCP/UDP/GRE/SCTP/FoCE. > >>>>>>> > >>>>>>> But I think we also need something like > >>>>>>> \field{supported_validate_types} to > >>>>>>> indicate which packet types the device supports validating and > >>>>>>> setting > >>>>>>> DATA_VALID, > >>>>>>> otherwise the device driver that negotiates this feature may > >>>>>>> fail to live > >>>>>>> migration. > >>>>>>> Am I right? > >>>>>>> > >>>>>>> I'm not sure how GUEST_CSUM works now as it should also suffer > >>>>>>> from the > >>>>>>> above > >>>>>>> mentioned issues with live migration, but no devices are > >>>>>>> reporting this > >>>>>>> right now. > >>>>>>> > >>>>>>> Maybe, each device only supports checksum verification for > >>>>>>> TCP/UDP by > >>>>>>> default? I don't know. > >>>>>>> But I hope we can focus on this and get consensus, because our > >>>>>>> hw release > >>>>>>> date is coming soon. > >>>>>>> > >>>>>>> Thanks a lot! > >>>>>> First, DATA_VALID is not a thing that hardware should ever use. > >>>>>> It's a hack when packets are passed within host. > >>>>> Get here. Thanks! > >>>> So if I understand correctly, we need a new flag here and define the > >>>> supported protocols. > >>> > >>> For what, migration? > >> No. > >> > >> It's basically a question of whether or not we want to reuse > >> DATA_VALID. It has limitations that it only works for some specific > >> protocols. > > > > Yes, I understand what Xuan means is that we reuse DATA_VALID and > > advertise > > what specific protocols DATA_VALID covers. > > > > Back to supported_validate_types, now I think we really don't need to > > let the device > > provide this field. Why? Because asssuming the device provides it, it > > seems useless > > because the driver does not parse the protocol type for each packet > > and does not use this field. > > The driver and stack just want to know whether the packet is valid or > > checksum unneccessary. > > Hi Jason and Michael. > > Are we aligned on this? For supported_validate_types, I don't think we need it. But I'm not sure if we are aligned. > > If yes, I'll write a new version to refresh the thread and try to avoid > future rollbacks. Maybe you can post a new version. Thanks > > Thanks a lot! > > > > > Thanks! > > > >> > >> If we don't, we need a new flag. > >> > >> 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/ >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]