RE: [virtio-dev] RE: [PATCH v5 0/7] Introduce device group and device management

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, July 31, 2022 11:39 AM
> 
> On Sat, Jul 30, 2022 at 01:21:20PM +0000, Parav Pandit wrote:
> > > > Not a good reason to introduce something inferior.
> > >
> > > Avoiding duplication and focusing on low hanging fruit are all sound
> > > engineering principles.
> > >
> > It is not a duplication.
> > I iterate again. The bits we asked to put in the Aq commands are the one
> that does not need to be negotiated early.
> > Feature bits were mis-used to put all things in there because in past a
> generic AQ didn't exists.
> > And there is no reason for that past decision to influence current proposal.
> >
> > Feature bits that are so basic for device bring up stage, stays in feature bits
> area defined today.
> > Hence, its not a duplication.
> 
> Let's take a look at a network device for example.
> 
> 
> 	\item[VIRTIO_NET_F_CSUM (0)] Device handles packets with partial
> checksum.   This
> 	  ``checksum offload'' is a common feature on modern network cards.
> 
> 	\item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with
> partial checksum.
> 
> 	\item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel
> offloads
> 		reconfiguration support.
> 
> 	\item[VIRTIO_NET_F_MTU(3)] Device maximum MTU reporting is
> supported. If
> 	    offered by the device, device advises driver about the value of
> 	    its maximum MTU. If negotiated, the driver uses \field{mtu} as
> 	    the maximum MTU value.
> 
> 	\item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
> 
> 	\item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> 
> 	\item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> 
> 	\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with
> ECN.
> 
> 	\item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> 
> 	\item[VIRTIO_NET_F_HOST_TSO4 (11)] Device can receive TSOv4.
> 
> 	\item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> 
> 	\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with
> ECN.
> 
> 	\item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> 
> 	\item[VIRTIO_NET_F_MRG_RXBUF (15)] Driver can merge receive
> buffers.
> 
> 	\item[VIRTIO_NET_F_STATUS (16)] Configuration status field is
> 	    available.
> 
> 	\item[VIRTIO_NET_F_CTRL_VQ (17)] Control channel is available.
> 
> 	\item[VIRTIO_NET_F_CTRL_RX (18)] Control channel RX mode
> support.
> 
> 	\item[VIRTIO_NET_F_CTRL_VLAN (19)] Control channel VLAN
> filtering.
> 
> 	\item[VIRTIO_NET_F_GUEST_ANNOUNCE(21)] Driver can send
> gratuitous
> 	    packets.
> 
> 	\item[VIRTIO_NET_F_MQ(22)] Device supports multiqueue with
> automatic
> 	    receive steering.
> 
> 	\item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address
> through control
> 	    channel.
> 
> 	\item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> coalescing.
> 
> 	\item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4
> packets.
> 
> 	\item[VIRTIO_NET_F_GUEST_USO6 (55)] Driver can receive USOv6
> packets.
> 
> 	\item[VIRTIO_NET_F_HOST_USO (56)] Device can receive USO
> packets. Unlike UFO
> 	 (fragmenting the packet) the USO splits large UDP packet
> 	 to several segments when each of these smaller packets has UDP
> header.
> 
> 	\item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-
> packet hash
> 	    value and a type of calculated hash.
> 
> 	\item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the
> exact \field{hdr_len}
> 	    value. Device benefits from knowing the exact header length.
> 
> 	\item[VIRTIO_NET_F_RSS(60)] Device supports RSS (receive-side
> scaling)
> 	    with Toeplitz hash calculation and configurable hash
> 	    parameters for receive steering.
> 
> 	\item[VIRTIO_NET_F_RSC_EXT(61)] Device can process duplicated
> ACKs
> 	    and report number of coalesced segments and duplicated ACKs.
> 
> 	\item[VIRTIO_NET_F_STANDBY(62)] Device may act as a standby for
> a primary
> 	    device with the same MAC address.
> 
> 	\item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and
> duplex.
> 
> 
> which of these are basic for bring up stage?
> 
> 
As you pointed all above fields has zero relation to the device initialization.
Those can be very well-done post device init stage over an AQ.
The fact that such queue never existed before, it was logic to extend the feature bits.
Otherwise, they are very well suitable to be negotiated over net CVQ or new AQ.
> > >
> > Even for non SIOV devices?
> 
> Yes.
In that case it is no different than the AQ. :)
Not sure what are we debating for.
You say that feature bits of the VF _itself_ are negotiated over a transport queue by VF itself.
Including the above one that you listed?
Right?
If so, that is useful.
Yet to review the rfc.

> > > Not 100% sure what you are saing here, so I think you should post a
> > > new version, yes. Generally iterating faster is a good idea. If you
> > > feel you didn't get enough feedback on a given version and some time
> > > passed try pinging people.
> > Without closing the discussion, I have seen same topic come up again and
> again in future version.
> > So even though new series is up for posting, we better close the
> discussion.
> 
> That's a mistake, it will be harder for you to make progress like this.
> Reviewers have limited attention span and memory, text bitrots, you decide
> to make unrelated changes yourself... Sorry I'm repeating myself but iterate
> quickly in the open even if there are known issues not addressed yet, just
> make very sure to be open and detailed about issues you addressed and
> known issues you did not address.
Ok. Thanks for the useful direction.