Re: [PATCH net-next v3 01/11] virtio-pci: Expose generic device capability operations

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

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.

-- 
MST