Re: [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

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

On Wed, Oct 11, 2023 at 11:58:11AM +0300, Yishai Hadas wrote:
> On 11/10/2023 11:02, Michael S. Tsirkin wrote:
> > On Wed, Oct 11, 2023 at 10:44:49AM +0300, Yishai Hadas wrote:
> > > On 10/10/2023 23:42, Michael S. Tsirkin wrote:
> > > > On Tue, Oct 10, 2023 at 07:09:08PM +0300, Yishai Hadas wrote:
> > > > > > > Assuming that we'll put each command inside virtio as the generic layer, we
> > > > > > > won't be able to call/use this API internally to get the PF as of cyclic
> > > > > > > dependencies between the modules, link will fail.
> > > > I just mean:
> > > > virtio_admin_legacy_io_write(sruct pci_device *,  ....)
> > > > 
> > > > 
> > > > internally it starts from vf gets the pf (or vf itself or whatever
> > > > the transport is) sends command gets status returns.
> > > > 
> > > > what is cyclic here?
> > > > 
> > > virtio-pci depends on virtio [1].
> > > 
> > > If we put the commands in the generic layer as we expect it to be (i.e.
> > > virtio), then trying to call internally call for virtio_pci_vf_get_pf_dev()
> > > to get the PF from the VF will end-up by a linker cyclic error as of below
> > > [2].
> > > 
> > > As of that, someone can suggest to put the commands in virtio-pci, however
> > > this will fully bypass the generic layer of virtio and future clients won't
> > > be able to use it.
> > virtio_pci would get pci device.
> > virtio pci convers that to virtio device of owner + group member id and calls virtio.
> 
> Do you suggest another set of exported symbols (i.e per command ) in virtio
> which will get the owner device + group member + the extra specific command
> parameters ?
> 
> This will end-up duplicating the number of export symbols per command.

Or make them inline.
Or maybe actually even the specific commands should live inside virtio pci
they are pci specific after all.

> > no cycles and minimal transport specific code, right?
> 
> See my above note, if we may just call virtio without any further work on
> the command's input, than YES.
> 
> If so, virtio will prepare the command by setting the relevant SG lists and
> other data and finally will call:
> 
> vdev->config->exec_admin_cmd(vdev, cmd);
> 
> Was that your plan ?

is vdev the pf? then it won't support the transport where commands
are submitted through bar0 of vf itself.

> > 
> > > In addition, passing in the VF PCI pointer instead of the VF group member ID
> > > + the VIRTIO PF device, will require in the future to duplicate each command
> > > once we'll use SIOV devices.
> > I don't think anyone knows how will SIOV look. But shuffling
> > APIs around is not a big deal. We'll see.
> 
> As you are the maintainer it's up-to-you, just need to consider another
> further duplication here.
> 
> Yishai
> 
> > 
> > > Instead, we suggest the below API for the above example.
> > > 
> > > virtio_admin_legacy_io_write(virtio_device *virtio_dev,  u64
> > > group_member_id,  ....)
> > > 
> > > [1]
> > > [yishaih@reg-l-vrt-209 linux]$ modinfo virtio-pci
> > > filename: /lib/modules/6.6.0-rc2+/kernel/drivers/virtio/virtio_pci.ko
> > > version:        1
> > > license:        GPL
> > > description:    virtio-pci
> > > author:         Anthony Liguori <aliguori@us.ibm.com>
> > > srcversion:     7355EAC9408D38891938391
> > > alias:          pci:v00001AF4d*sv*sd*bc*sc*i*
> > > depends: virtio_pci_modern_dev,virtio,virtio_ring,virtio_pci_legacy_dev
> > > retpoline:      Y
> > > intree:         Y
> > > name:           virtio_pci
> > > vermagic:       6.6.0-rc2+ SMP preempt mod_unload modversions
> > > parm:           force_legacy:Force legacy mode for transitional virtio 1
> > > devices (bool)
> > > 
> > > [2]
> > > 
> > > depmod: ERROR: Cycle detected: virtio -> virtio_pci -> virtio
> > > depmod: ERROR: Found 2 modules in dependency cycles!
> > > make[2]: *** [scripts/Makefile.modinst:128: depmod] Error 1
> > > make[1]: *** [/images/yishaih/src/kernel/linux/Makefile:1821:
> > > modules_install] Error 2
> > > 
> > > Yishai
> > virtio absolutely must not depend on virtio pci, it is used on
> > systems without pci at all.
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization