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

[Parav Pandit <parav@nvidia.com>]

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

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

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