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:27:46AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Friday, November 24, 2023 11:37 AM
> > 
> > On Fri, Nov 24, 2023 at 05:53:02AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Friday, November 24, 2023 11:03 AM
> > > >
> > > > On Fri, Nov 24, 2023 at 12:02:23PM +0800, Jason Wang wrote:
> > > > > > > I won't be able to absorb this comment of DMA interface.
> > > > > > > If I discuss further, I will repeat the whole document [1] and
> > > > > > > I will avoid
> > > > that now.
> > > > > > >
> > > > > > > [1]
> > > > > > > https://docs.google.com/document/d/1Iyn-
> > > > l3Nm0yls3pZaul4lZiVj8x1s73
> > > > > > > Ed6rOsmn6LfXc/edit#heading=h.qexbtyc2jpwr
> > > > > >
> > > > > >
> > > > > > I really worry about how provisioning will work. And I do not at
> > > > > > all cherish replicating all of these query capability commands for
> > provisioning.
> > > > >
> > > > > +1
> > > > >
> > > > > There's nothing that prevents the config space from being
> > > > > implemented in a way other than registers.
> > > >
> > > > Care doing it finally? Let's see if what Parav is worrying about is
> > > > then addressed.
> > >
> > > The whole concept that everything must be in one giant config space is just
> > simply bad.
> > > It does not exist either in virtio spec today.
> > 
> > But it does, this is what transports are doing.
> I don't understand what is "it".

"it" here is passing device init time configuration to drivers.


> Transport like pci transport bits.

in a simple, logical and functional manner.

> CVQ is functional object that helps to arrange the bits in logical, functional manner instead of trying to place them in bit array.

In a device specific way.

> > 
> > > once can see that what is presented in the commands cannot be placed in
> > config space at dynamic location.
> > > Same was the case with statistics too.
> > > Same was the case with VQ coaleasing knobs.
> > > Same with hash knobs.
> > > With flow filters,
> > > With rss contexts
> > > With rtc
> > > With new queues creation apis.
> > >
> > > The endless list continues...
> > >
> > > And reserving bits for future (for other than pad bytes) for future addition in
> > config space is equally not elegant design.
> > > Bits will get spread out at random location making things even harder to
> > maintain.
> > >
> > > The device is no longer a simple mac_addr + N queues device with some
> > static rss config anymore.
> > >
> > > With all modern work, every capability query and run time configuration is
> > done over cvq interface today.
> > > Single get/set channel from driver to device all using existing resources.
> > >
> > > The real hw device also does not need to refer to two places of config and
> > cvq when serving cvq commands.
> > > Oh, the list of advantages just continues with what 1.3 spec has done.
> > 
> > I don't see the problem sorry. We've been doing this for many years with many
> > ways to access config space.  It scaled well.
> > 
> I don't see it. sorry.
> The configuration of the device is done using cvq.

runtime configuration, absolutely. We found out writeable config space
field are painful for a variety of ways, the main one being device can't
report errors.  So we generally avoid writeable config space.  But read
only - no good reason to avoid them for init time things.




> > 
> > Then hardware offload guys come and say that in PCI spec current transport is
> > forcing use of on-device memory, and they want to build cheap offload PCI
> > based devices. Fine, let's build a transport variant that does not force this.
> 
> All new capabilities and control is over the cvq. What is baked until 1.2 is sort of legacy.

Just repeating this will not make everyone agree.

> >  And
> > we want optional compatibility, so let's also find a way to do that.  This makes
> > much more sense than forcing transport specific issues on everyone.
> > 
> Trying to attribute as some transport specific issue is just not aligned to the spec written today.
> 
> > To add to that, what did not historicall scale well is transport-specific registers.
> Then you should have put the VQ notification coalescing functionality in a horrible virtio_net_config register like how a queue reset in the common config space.

No because of the 4 commands: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET none are initialization time.
They are not capabilities: they control and inspec device
runtime state.

If we had VIRTIO_NET_CTRL_NOTF_COAL_CAP_GET that would belong in
config space.

> Thank God that mistake was not done and many similar mistakes were avoided.

Let's not get emotional here please.

> > That was a bad design, with all transports doing exactly the same thing is
> > slightly different ways.  And what you are advocating for with CVQ is exactly
> > replicating the bad design not the good one.
> 
> CVQ design is what extends the current spec in good way. Followed by many other non nvidia nics listed in the doc for reference.

I don't know what you are referring to here.  Register maps are all over
the place. It's a simple, standard, well understood practice.

We have some niche uses due to need for extreme VF# counts, this
forces DMA for them, not a good reason to force it for everyone.
The sooner you just stop forcing this down everyone's throat
the faster we can make progress on things that matter.


-- 
MST