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