Linux Virtualization - Oct 2023 - [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

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.

no cycles and minimal transport specific code, right?
> 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.
> 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.

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.
>