[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/
>
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]