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.
