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

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

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.

> > 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.


> 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".


> > 
> > 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.
> > >
> > >
>