Yishai Hadas
2023-Oct-11 08:58 UTC
[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device
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.> 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 ?> >> 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 at 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 at 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. >
Michael S. Tsirkin
2023-Oct-11 09:03 UTC
[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device
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 at 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 at 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. > >