On Thu, Sep 25, 2025 at 11:53:50AM -0500, Dan Jurgens wrote:

On 9/25/25 8:08 AM, Michael S. Tsirkin wrote:

quoted

On Thu, Sep 25, 2025 at 05:39:54PM +0530, Parav Pandit wrote:

quoted

On 25-09-2025 05:19 pm, Michael S. Tsirkin wrote:

quoted

On Thu, Sep 25, 2025 at 04:15:19PM +0530, Parav Pandit wrote:

quoted

On 25-09-2025 04:05 pm, Michael S. Tsirkin wrote:

quoted

On Thu, Sep 25, 2025 at 03:21:38PM +0530, Parav Pandit wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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