[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, 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?
I don't think so.
Every packet has the flag(valid or none). The guest will check this flag.
If we support this flag in config, the driver need to check this?
For migration, if we want to migrate one from src to dst.
If the src device supports tcp and udp checksum and
the dst device just supports tcp checksum.
I think we should not migrate to this dst device. But this process
does not need the involvement of the driver.
But for describing the supported protocols, I think that is ok.
We lost that for GUEST_CSUM, we should not lost again for the new one.
Thanks.
>
> 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]