RE: [virtio-comment] Re: [PATCH v6 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 2:43 PM
> 
> On Fri, Nov 24, 2023 at 06:41:11AM +0000, Parav Pandit wrote:
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, November 24, 2023 11:32 AM
> >
> >
> > > Just to clarify my point, what's I'm saying is
> > >
> > > 1) For CVQ, it is used to send the commands to the device. It's not
> > > good to use transport to send those commands
> > This is not what is proposed.
> >
> > CVQ is proposed to exchange capabilities and configuration between driver
> and device.
> > (not to transport to send commands)
> >
> > > 2) The configuration space is the place to advertise the
> > > capabilities, e.g what kind of commands could the device accept.
> > >
> > and it is not efficient and not used anymore with 1.3 and higher.
> > They are exchanged using single get/set interface where device does not
> need to do special cross synchronization between what is exposed via
> interface_1 (config space) and control via interface_2 (cvq).
> 

> Why because you wrote that google doc? There never was any concensus on
> this.
No. The cross synchronization needs within the device to be done even without writing the doc.
The doc captures multiple discussions that captured the pros and cons of each in FAQ form and our view in this area.

> 
> > > 2) doesn't conflict with 1)
> > >
> > CVQ does not conflict with anything as it stands today in 1.3.
> >
> > > And we are discussing the provisioning which is more about 2) but
> > > not 1) here.
> > Provisioning will set the device and device will run based on what is being
> set.
> > If MSIX is provisioned, it will show up in MSIX attribute.
> > If the BAR region size is provisioned it will show in PCI MMIO size.
> > If 1.3 legacy mac address configured, shows up in virtio_net_config.
> > if 1.3 rss configuration, shows up in virtio net config
> >
> > if 1.3 stats configuration, shows up in stats.
> > If 1.4 flow filters provisioned shows in flow filter caps.
> >
> > The primary tenet is: config space contains only the necessary driver
> bootstap fields.
> 
> I don't want to argue what bootstrap is. Please use existing spec terminology.
> initialization time is a better definition we already use in spec - it is clearly
> whatever info driver needs during probe and whatever subsystem specific
> initialization callback is.  For example, for network - ndo_open.
> 
Initialization time as defined in spec as listed in " Device Initialization".
This means only upto DRIVER_OK phase.

Does the driver need to know about flow filter capabilities to initialize the device? No.

Can it be detected post initialization? i.e. after DRIVER_OK without burning some MMIO registers? Sure yes.

> 
> > The bright line in based on this usage: bootstap or not.
> > If its runtime, sure have it over CVQ.
> > Following the nice tenet of B.2 of the spec snippet: "Device configuration
> space should only be used for initialization-time parameters"
> > Thatâs it.
> > And everyone is already aligned to it.
> 
> Absolutely. Specifically, when do you expect driver to probe these caps? As
> you yourself explained, it has to do it before ethtool calls - that clearly means it
> will do it in probe.
> To me this simply screams "initialization time".
> 
It can be done in driver probe routine after DRIVER_OK, this is fine.

> 
> > >
> > > That's it.
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > > Worst case devices can lie, it is not clear worrying about
> > > > > statistics when provisioning is important enough.
> > > > Device statistics cannot be broken when migrating.
> > > >
> > > > > And
> > > > > you are now under the impression everyone got convinced when in
> > > > > reality everyone just got tired of arguing. This is not a good
> > > > > pattern to try
> > > and repeat.
> > > > > Let me repeat the relevant part here for
> > > > > you:
> > > > >
> > > > >       So why shouldnât we just add some transport VQ on the
> > > > > owner device to transport the SIOV deviceâs configuration?
> > > > >
> > > > >               Ans:
> > > > >               Such addition means that hardware vendors need to
> > > > > build runtime configurations in 4 different ways.
> > > > >               One way using CVQ for PCI PF and VFs
> > > > >               2nd way as backward compatible SIOV using ownerâs
> > > > > admin VQ
> > > > >               3rd way using SIOVâs own CVQ channel for TDISP
> > > > >               4th way using mix of backward compatible and
> > > > > secure and efficient way using #2 and #3.
> > > > >
> > > > >
> > > > > This is just a straw-man argument. No - we make a capability
> > > > > optional,
> > > > Capabilities are not optional.
> > > > The design has undergone carefully review to ensure that driver
> > > > honors the
> > > capability, and it is migratable both.
> > > >
> > > > You dint explain why capability should be optional.
> > > > The first patch has defined how the capabilities are used by the driver.
> > > >
> > > > > we
> > > > > strongly suggest that *drivers* support both old and new
> > > > > mechanism, and then *devices* will only implement what's required.
> > > > There are other examples in the same document that makes things
> > > > worst
> > > with old and new.
> > > >
> > > > Also there is literally no way to enforce that driver supports
> > > > both and new. It
> > > is just sounds like an excuse to force infinite config space.
> > > >
> > > > The method proposed here is elegant and clearly promote one way to
> > > > do
> > > things for driver and device with predictability.
> > > >
> > > >
> >