[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 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.
Thanks
>
> > Second I don't understand what it has to do with this
> > discussion.
> > Your patch should be more specific and define which
> > header fields are compatible with the new offload option.
> >
> >
> >
> >>> 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]