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

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

On Mon, Nov 27, 2023 at 11:54:52AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, November 27, 2023 5:17 PM
> > 
> > On Mon, Nov 27, 2023 at 11:40:25AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, November 27, 2023 4:58 PM
> > > >
> > > > On Mon, Nov 27, 2023 at 10:19:49AM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Friday, November 24, 2023 5:04 PM
> > > > > >
> > > > > > On Fri, Nov 24, 2023 at 06:31:03AM +0000, Parav Pandit wrote:
> > > > > > >
> > > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > Sent: Friday, November 24, 2023 11:48 AM
> > > > > > > >
> > > > > > > > On Fri, Nov 24, 2023 at 05:40:26AM +0000, Parav Pandit wrote:
> > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > There is a very simple method though.  We allow devices to
> > > > > > > > expose a subset of features when DMA is not used. So drivers
> > > > > > > > that want maximum features will always opt for DMA. We can
> > > > > > > > also strongly recommend that all drivers support DMA if available.
> > > > > > > Yeah, don't see how this is elegant at all with all mixed bits.
> > > > > >
> > > > > > It's elegant because simple low end devices can cheaply
> > > > > > implement MMIO and not worry about DMA.
> > > > > >
> > > > > It is not of much help in this case because any low end cheap
> > > > > device which
> > > > want to support flow filter commands need to have CVQ anyway.
> > > > > And hence reusing the same CVQ is more elegant that already does the
> > DMA.
> > > > >
> > > > > So CVQ is fulfilling all the below needs.
> > > > > 1. Single interface for the get/set config flow filters 2. DMA the
> > > > > data 3. Not have any partial issues
> > > >
> > > > I don't know what these are.
> > > I mean partial writes for fields.
> > 
> > We don't allow these and in any case, writeble fields are best avoided.
> > 
> Right. This is why flow filter objects are created using cvq, and its associated caps also comes via same cvq channel.
> 
> > 
> > > >
> > > > > 4. provides consistent structures that provisioning side will be
> > > > > able to use
> > > >
> > > > Problem for provisioning is extra definitions will be needed, in a
> > > > device specific way.
> > > In vdpa tool and other OS tools of iproute2 developed, setting and getting
> > those device specific values are useful.
> > > It is ok.
> > 
> > It does not become ok just by saying so. You are taking a single RO value and
> > instead of it having an address there are now 2 other ways to address it. And
> > you fail to see the problem and the pain you are inflicting on software
> > developers. Just stick with an address if you can.
> There is zero problem with sw.
> Sw just need to issue send_command() and done with it, like rest of the commands.
> A pain would be create yet another DMA interface.

No because it will be a generic thing for all types.


> > 
> > > >
> > > > > > > Nor do I see any enforcement, single method via cvq still holds strong.
> > > > > >
> > > > > > You don't need to enforce things, if people want to put a lot of
> > > > > > RAM on device and put it in a register let them.
> > > > > >
> > > > > Not enforced. It uses the CVQ for flow group and flow filter life
> > > > > cycles and for
> > > > the sharing this config as well.
> > > > > Also aligns with stats that rest also agreed on.
> > > >
> > > > I am talking about your attempt to generally say "no more config
> > > > fields everything must be in CVQ".
> > > Config fields for initialization time is fine as the spec allows it today.
> > > Things which can differ, it is ok to use cvq interface.
> > 
> > I don't know what does "Things that can differ" means. Generally device caps
> > are perfect for config space. Accessed at init time only, RO.
> > 
> You ignore the comment I answered before that proposal here is not based on RO/RW.
> It is based on initialization time vs run time.

initialization time is really probe. That thing which only happens
once when driver is setup. Clearly there is no reason to
check capabilities many times so it will be done during setup,
once.


> > > > I think it's wrong definitiely for non network devices must
> > > > sometimes for network too and generally we need a solution for
> > > > config over DMA. This specific thing - whether it fits in CVQ is a
> > > > separate discussion.
> > > >
> > > I explained it before, that 6 out of 19 devices has cvq which are complex
> > enough doing things over cvq.
> > > These are non-network devices already.
> > >
> > > If one of those remaining device becomes complex, it is likely it will need a
> > cvq to suffice for the dma interface and it can just do with depth = 1.
> > 
> > Using generic caps and not net specific ones is a good idea.
> > 
> context here is cvq and net.

yes, you can use cvq command for this.  this will only serve net.  I am
saying a generic interface for config over DMA is better and will
serve everyone.

We are doing this commonly, and when we did not push for a generic
interface like with e.g. s/g limits for blk we later regret it.


> > 
> > 
> > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > The method proposed here is elegant and clearly promote
> > > > > > > > > one way to do
> > > > > > > > things for driver and device with predictability.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I don't see it as elegant at all. What is elegant is *a
> > > > > > > > single
> > > > > > > > tag* that describes each property of the device. And this
> > > > > > > > single tag should be
> > > > > > good for everything:
> > > > > > > > driver, provisioning, migration. And config space offset serves as
> > such.
> > > > > > > The single tag is the set of structures.
> > > > > >
> > > > > > I have no idea how this will work. If migration format i started
> > > > > > reviewing is anything to go by then there will be a huge
> > > > > > elaborate structure nothing single or simple. By comparison
> > > > > > there's already a proposal how provisioning can work by supplying
> > config space.
> > > > > > it is just a clean model to grasp.
> > > > > >
> > > > > The provisioning model is simple is to supply all the configuration.
> > > > > To draw parallels to some sw side,
> > > > >
> > > > > There is per functionality socket option to set things, instead of
> > > > > one giant
> > > > structure.
> > > > > There is per functionality ethtool option/cmd instead of Set
> > > > > ALL/get ALL
> > > > enforcement.
> > > >
> > > > I'm not sure how much of a parallel one can draw.
> > > > Do not see a lot of similarity.
> > > For lot of configuration they are similar that happens at slow pace.
> > >
> > > > Devices commonly use register map. Everyone understands this paradigm.
> > > >
> > > For initialization early device setup time, yes.
> > >
> > > >
> > > > I am not altogether happy with the way you are making migration
> > > > generate duplicate definitions for lots of things we already have definitions
> > for.
> > > > Having a 3rd one for provisioning? Gimme a break.
> > >
> > > For migration, we are not duplicating. Some structures are not well defined,
> > it has some duplication.
> > 
> > And fyi it's already making people unhappy.
> > 
> Those exceptions are not the interesting one to take as example here.
> 
> > > But large part seems be able to utilized pre-defined structs.
> > > And here for flow filter also same structs will be used.
> > 
> > So if there's a 64 bit bitmap in config space, then provisioning command which
> > already gets config space can just use its offset.
> > Simpler, better.
> > 
> It is not simple to implement per device unique config space as we discussed already.

we did it for many years and many device types.
99% of devices do config over a register map of some kind.
It is not as *flexible* true but flexibility here is an enemy,
too easy to make a mess.


And your array of types where a single field would suffice
is a classical case in point. config space keeps us disciplined.

> And no need another DMA interface either as cvq service that need already.

This is too focused on net and short term, for my taste.
we need it so we
1. don't need to have this discussion every time
2. can begin to save on registers for config space in X years and
   for new device types


And, while apparently focusing on short term people also somehow manage to
make a point out of every bikeshed instead of the "let's just ship it"
attitude that would be consistent. I find this baffling.


-- 
MST