On Thu, Sep 25, 2025 at 11:53:50AM -0500, Dan Jurgens wrote: > On 9/25/25 8:08 AM, Michael S. Tsirkin wrote: > > On Thu, Sep 25, 2025 at 05:39:54PM +0530, Parav Pandit wrote: > >> > >> On 25-09-2025 05:19 pm, Michael S. Tsirkin wrote: > >>> On Thu, Sep 25, 2025 at 04:15:19PM +0530, Parav Pandit wrote: > >>>> On 25-09-2025 04:05 pm, Michael S. Tsirkin wrote: > >>>>> On Thu, Sep 25, 2025 at 03:21:38PM +0530, Parav Pandit wrote: > >>>>>> Function pointers are there for multiple transports to implement their > >>>>>> own > >>>>>> implementation. > >>>>> My understanding is that you want to use flow control admin commands > >>>>> in virtio net, without making it depend on virtio pci. > >>>> No flow control in vnet. > >>>>> This why the callbacks are here. Is that right? > >>>> No. callbacks are there so that transport agnostic layer can invoke it, > >>>> which is drivers/virtio/virtio.c. > >>>> > >>>> And transport specific code stays in transport layer, which is presently > >>>> following config_ops design. > >>>> > >>>>> That is fair enough, but it looks like every new command then > >>>>> needs a lot of boilerplate code with a callback a wrapper and > >>>>> a transport implementation. > >>>> Not really. I dont see any callbacks or wrapper in current proposed > >>>> patches. > >>>> > >>>> All it has is transport specific implementation of admin commands. > >>>> > >>>>> > >>>>> Why not just put all this code in virtio core? It looks like the > >>>>> transport just needs to expose an API to find the admin vq. > >>>> Can you please be specific of which line in the current code can be > >>>> moved to > >>>> virtio core? > >>>> > >>>> When the spec was drafted, _one_ was thinking of admin command transport > >>>> over non admin vq also. > >>>> > >>>> So current implementation of letting transport decide on how to > >>>> transport a > >>>> command seems right to me. > >>>> > >>>> But sure, if you can pin point the lines of code that can be shifted to > >>>> generic layer, that would be good. > >>> I imagine a get_admin_vq operation in config_ops. The rest of the > >>> code seems to be transport independent and could be part of > >>> the core. WDYT? > >>> > >> IMHV, the code before vp_modern_admin_cmd_exec() can be part of > >> drivers/virtio/virtio_admin_cmds.c and admin_cmd_exec() can be part of the > >> config ops. > >> > >> However such refactor can be differed when it actually becomes boiler plate > >> code where there is more than one transport and/or more than one way to > >> send > >> admin cmds. > > > > Well administration virtqueue section is currently not a part of a > > transport section in the spec. But if you think it will change and so > > find it cleaner for transports to expose, instead of a VQ, a generic > > interfaces to send an admin command, that's fine too. That is still a > > far cry from adding all the object management in the transport. > > > > > > Well we have all the new code you are writing, and hacking around > > the fact it's in the wrong module with a level of indirection > > seems wrong. > > If you need help moving this code let me know, it's not hard. > > > >> Even if its done, it probably will require vfio-virtio-pci to interact with > >> generic virtio layer. Not sure added value of that complication to be part > >> of this series. > >> > >> > >> Dan, > >> > >> WDYT? > > > > > > virtio pci pulls in the core already, and VFIO only uses the SRIOV > > group, so it can keep using the existing pci device based interfaces, > > if you prefer. > > > > I can make changes here. I'd appreciate if you review the rest of the > series while I do so. Patches 3+ are isolated from this, so it won't be > a waste of your time.
OK - will review 3+, thanks! -- MST
