[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 Tue, Dec 5, 2023 at 10:18âPM Heng Qi <hengqi@linux.alibaba.com> 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? Probably, but I think most devices only do this for TCP/UDP packets now, so we are fine. > > 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? Maybe. Thanks > 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! > > > > > Thanks > > > >> Thanks! > >> > >>>>> After this is written up we will come up with a good short > >>>>> description for the feature bit. > >>>>> > >>>> Thanks > >>>> > >>>>>>> Thanks! > >>>>>>> > >>>>>>>> Thanks > >>>>>>>> > >>>>>>>>> Thanks! > >>>>>>>>> > >>>>>>>>>> Thanks > >>>>>>>>>> > >>>>>>>>>>>> Thanks! > >>>>>>>>>>>> > >>>>>>>>>>>>> Thanks > >>>>>>>>>>>>> > >>>>>>>>>>>>>> Thanks! > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Thanks > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads > >>>>>>>>>>>>>>>> reconfiguration support. > >>>>>>>>>>>>>>>> -- > >>>>>>>>>>>>>>>> 2.19.1.6.gb485710b > >>>>>>>>>>>>>>>> > >>>>>>>>>>>> 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/ > >>>>>>>>> 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/ > >>>>>>>>> > >>>>>>> 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/ > >>>>>>> > > > 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]