On 11/10/2023 12:03, Michael S. Tsirkin wrote:
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.
OK, let's leave them in virtio-pci as you suggested here.
You can see below [1] some scheme of how a specific command will look like.
Few notes:
- virtio_pci_vf_get_pf_dev() will become a static function.
- The commands will be placed inside virtio_pci_common.c and will use
locally the above static function to get the owner PF.
- Post of preparing the command we may call directly to
vp_avq_cmd_exec() which is part of vfio-pci and not to virtio.
- vp_avq_cmd_exec() will be part of virtio_pci_modern.c as you asked in
the ML.
- The AQ creation/destruction will still be called upon probing virtio
as was in V0, it will use the underlay config->create/destroy_avq() ops
if exist.
- virtio_admin_cmd_exec() won't be exported any more outside virtio,
we'll have an exported symbol in virtio-pci per command.
Is the above fine for you ?
By the way, from API namespace POV, are you fine with
virtio_admin_legacy_io_write() or maybe let's have '_pci' as part of the
name ? (i.e. virtio_pci_admin_legacy_io_write)
[1]
int virtio_admin_legacy_io_write(struct pci_dev *pdev, u16 opcode, u8
offset,
u8 size, u8 *buf)
{
struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
struct virtio_admin_cmd_legacy_wr_data *in;
struct virtio_admin_cmd cmd = {};
struct scatterlist in_sg;
int ret;
int vf_id;
if (!virtio_dev)
return -ENODEV;
vf_id = pci_iov_vf_id(pdev);
if (vf_id < 0)
return -EINVAL;
in = kzalloc(sizeof(*in) + size, GFP_KERNEL);
if (!in)
return -ENOMEM;
in->offset = offset;
memcpy(in->registers, buf, size);
sg_init_one(&in_sg, in, sizeof(*in) + size);
cmd.opcode = opcode;
cmd.group_type = VIRTIO_ADMIN_GROUP_TYPE_SRIOV;
cmd.group_member_id = vf_id + 1;
cmd.data_sg = &in_sg;
ret = vp_avq_cmd_exec(virtio_dev, &cmd);
kfree(in);
return ret;
} EXPORT_SYMBOL_GPL(virtio_admin_legacy_io_write);
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.
Yes, it's a PF.
Based on current spec for the existing admin commands we issue commands
only on the PF.
In any case, moving to the above suggested scheme to handle per command
and to get the VF PCI as the first argument we now have a full control
for any future command.
Yishai
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@xxxxxxxxxx>
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.
