Re: [virtio-comment] Re: [PATCH v4 1/2] virtio-net: update description for VIRTIO_NET_F_GUEST_CSUM.

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Dec 06, 2023 at 12:51:32PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/12/6 下午12:36, Jason Wang 写道:
> > 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.
> 
> Seems not, I understand what Michael means is that we don't need
> supported_validate_types.
> Because DATA_VALID is set per packet, similar to streaming. From the
> perspective of the driver
> and the device, there is no need to know which packet types the device
> supports setting DATA_VALID for.
> We just need to know what packet types DATA_VALID might cover. Is this
> aligning, Michael?

I don't know if it's even that. IIRC DATA_VALID mostly just means "this
packet is local, don't worry about checksums".
That being said I don't see how improving the description here
is related to the discussion around XDP.


> Hi Xuan! WDYT?
> 
> 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/
> > 
> > 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/