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

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

On Mon, Nov 27, 2023 at 12:49:35PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, November 27, 2023 6:03 PM
> > 
> > On Mon, Nov 27, 2023 at 11:50:24AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, November 27, 2023 5:10 PM
> > > >
> > > > On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Monday, November 27, 2023 4:53 PM
> > > > > >
> > > > > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > > > > It appears that's not what you meant.
> > > > > > > > > > I can try and guess what you meant but I'd rather have
> > > > > > > > > > you rewrite this in a way that makes the meaning explicit.
> > > > > > > > > >
> > > > > > > > > If you insist, I can duplicate in driver requirements too.
> > > > > > > > > >
> > > > > > > > > > > There are many capabilities here each with a different use.
> > > > > > > > > >
> > > > > > > > > > And you are under the impression that dumping all kind
> > > > > > > > > > of stuff in a single place is good somehow?
> > > > > > > > > It is not dumped.
> > > > > > > > > It is structured based on the functionality.
> > > > > > > > > The config space proposal tends to dump everything in
> > > > > > > > > unstructured way at
> > > > > > > > single place that cannot extended elegantly.
> > > > > > > > >
> > > > > > > > > For example, Xuan's dynamic array cannot be placed next to
> > > > > > > > > other dynamic
> > > > > > > > array of flow filter.
> > > > > > > >
> > > > > > > > I don't know what "dynamic array" is exactly but generally,
> > > > > > > > Any kind of a big array is not appropriate in config space -
> > > > > > > > I don't see any of these
> > > > > > here, though.
> > > > > > > Ah, now I see the disconnect.
> > > > > > >
> > > > > > > There is.
> > > > > > > struct virtio_net_ctrl_ff_match_types
> > > > > >
> > > > > > Now I think I don't understand what this array does at all.
> > > > > > Why are there multiple entries with each entry also including a
> > > > > > mask of multiple fields?
> > > > > >
> > > > > Each entry indicates a bitmask of supported fields that can be filtered.
> > > > > The type indicates how to interpret each fields_map.
> > > > > Any new type for MPLS can be added easily by defining its type and
> > mask.
> > > > > And same type enums and field mask also used by the data path too.
> > > >
> > > > Oh wait I think I begin to remember now. There are 12 bits defined
> > > > so far and you made it this huge array. Right? With code to parse
> > > > and format it all. Just add a 64 bit field and it will last us long
> > > > enough. No parsing and formatting just #define.
> > > The infrastructure defines 12 bits only because this is first minimal viable
> > work.
> > > There will be more fields to be added for more types.
> > 
> > Maybe.  Let's see a list and see if we even get close to 32 bits.
> > 
> There are many ether types to filter based on the device.
> Many types of IPv4 and IPV6 header fields.
> 
> https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml

If I grep for RFC there I get 31 types. Looks like 64 will last us
for a while.


> > > It is not good to mix up all the fields and bit definitions.
> > 
> > Not good how? 
> I provided example why it will be so bad to mix up the socket options under one socket option command.
> 
> >  It's definitely much simpler than what you wrote.
> Simpler for spec != simpler to implement in device.

Not just spec. Simpler for drivers too. And I suspect that yes a bitmap
is simpler in device too. 100% sure in a software device.

> > Does it matter that bits for a given transport are not all consequitive? I do not
> > see why it matters.
> > 
> It is surely not elegant to spread things around that way. 
> Actually it is not good for TLV definition in the data path flow filter commands.

That TLV thing still needs some thought. Here again,
we can have a single config space array and it's nice and clean
or we need to invent 

> > 
> > > And there are per device max limits which is way more than 12 bits.
> > 
> > These are just a couple of 32 bit fields.
> > 
> And they can be different that cannot be de-duplicated across different member devices.

why would they be different? only if provisioned.  But again, just
access it over DMA then it is not a problem that they are different. The
objection is to using a device specific mechanism as opposed to building
a generic config space over DMA.

> The proposal is not based on the count or RO/RW etc.
> It is based on when it needs to be accessed, as they are not init time early use to initialize the device, they are over cvq.

all these limits are very clearly init time. For example max id can be
used to allocate an array.

-- 
MST