[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 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. > > > > 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/ > > > > > >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]