On Tue, Sep 26, 2023 at 02:14:01PM +0300, Yishai Hadas wrote: > On 22/09/2023 12:54, Michael S. Tsirkin wrote: > > On Thu, Sep 21, 2023 at 03:40:39PM +0300, Yishai Hadas wrote: > > > Expose admin commands over the virtio device, to be used by the > > > vfio-virtio driver in the next patches. > > > > > > It includes: list query/use, legacy write/read, read notify_info. > > > > > > Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxx> > > > > This stuff is pure virtio spec. I think it should live under > > drivers/virtio, too. > > The motivation to put it in the vfio layer was from the below main reasons: > > 1) Having it inside virtio may require to export a symbol/function per > command. > > This will end up today by 5 and in the future (e.g. live migration) with > much more exported symbols. > > With current code we export only 2 generic symbols > virtio_pci_vf_get_pf_dev(), virtio_admin_cmd_exec() which may fit for any > further extension. Except, there's no reasonable way for virtio to know what is done with the device then. You are not using just 2 symbols at all, instead you are using the rich vq API which was explicitly designed for the driver running the device being responsible for serializing accesses. Which is actually loaded and running. And I *think* your use won't conflict ATM mostly by luck. Witness the hack in patch 01 as exhibit 1 - nothing at all even hints at the fact that the reason for the complicated dance is because another driver pokes at some of the vqs. > 2) For now there is no logic in this vfio layer, however, in the future we > may have some DMA/other logic that should better fit to the caller/client > layer (i.e. vfio). You are poking at the device without any locks etc. Maybe it looks like no logic to you but it does not look like that from where I stand. > By the way, this follows what was done already between vfio/mlx5 to > mlx5_core modules where mlx5_core exposes generic APIs to execute a command > and to get the a PF from a given mlx5 VF. This is up to mlx5 maintainers. In particular they only need to worry that their patches work with specific hardware which they likely have. virtio has to work with multiple vendors - hardware and software - and exposing a low level API that I can't test on my laptop is not at all my ideal. > This way, we can enable further commands to be added/extended > easily/cleanly. Something for vfio maintainer to consider in case it was assumed that it's just this one weird thing but otherwise it's all generic vfio. It's not going to stop there, will it? The duplication of functionality with vdpa will continue :( I am much more interested in adding reusable functionality that everyone benefits from than in vfio poking at the device in its own weird ways that only benefit specific hardware. > See for example here [1, 2]. > > [1] https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/vfio/pci/mlx5/cmd.c#L210 > > [2] https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/vfio/pci/mlx5/cmd.c#L683 > > Yishai > > > > > --- > > > drivers/vfio/pci/virtio/cmd.c | 146 ++++++++++++++++++++++++++++++++++ > > > drivers/vfio/pci/virtio/cmd.h | 27 +++++++ > > > 2 files changed, 173 insertions(+) > > > create mode 100644 drivers/vfio/pci/virtio/cmd.c > > > create mode 100644 drivers/vfio/pci/virtio/cmd.h > > > > > > diff --git a/drivers/vfio/pci/virtio/cmd.c b/drivers/vfio/pci/virtio/cmd.c > > > new file mode 100644 > > > index 000000000000..f068239cdbb0 > > > --- /dev/null > > > +++ b/drivers/vfio/pci/virtio/cmd.c > > > @@ -0,0 +1,146 @@ > > > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB > > > +/* > > > + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved > > > + */ > > > + > > > +#include "cmd.h" > > > + > > > +int virtiovf_cmd_list_query(struct pci_dev *pdev, u8 *buf, int buf_size) > > > +{ > > > + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev); > > > + struct scatterlist out_sg; > > > + struct virtio_admin_cmd cmd = {}; > > > + > > > + if (!virtio_dev) > > > + return -ENOTCONN; > > > + > > > + sg_init_one(&out_sg, buf, buf_size); > > > + cmd.opcode = VIRTIO_ADMIN_CMD_LIST_QUERY; > > > + cmd.group_type = VIRTIO_ADMIN_GROUP_TYPE_SRIOV; > > > + cmd.result_sg = &out_sg; > > > + > > > + return virtio_admin_cmd_exec(virtio_dev, &cmd); > > > +} > > > + > > > +int virtiovf_cmd_list_use(struct pci_dev *pdev, u8 *buf, int buf_size) > > > +{ > > > + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev); > > > + struct scatterlist in_sg; > > > + struct virtio_admin_cmd cmd = {}; > > > + > > > + if (!virtio_dev) > > > + return -ENOTCONN; > > > + > > > + sg_init_one(&in_sg, buf, buf_size); > > > + cmd.opcode = VIRTIO_ADMIN_CMD_LIST_USE; > > > + cmd.group_type = VIRTIO_ADMIN_GROUP_TYPE_SRIOV; > > > + cmd.data_sg = &in_sg; > > > + > > > + return virtio_admin_cmd_exec(virtio_dev, &cmd); > > > +} > > > + > > > +int virtiovf_cmd_lr_write(struct virtiovf_pci_core_device *virtvdev, u16 opcode, > > > + u8 offset, u8 size, u8 *buf) > > > +{ > > > + struct virtio_device *virtio_dev = > > > + virtio_pci_vf_get_pf_dev(virtvdev->core_device.pdev); > > > + struct virtio_admin_cmd_data_lr_write *in; > > > + struct scatterlist in_sg; > > > + struct virtio_admin_cmd cmd = {}; > > > + int ret; > > > + > > > + if (!virtio_dev) > > > + return -ENOTCONN; > > > + > > > + 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 = virtvdev->vf_id + 1; > > > + cmd.data_sg = &in_sg; > > > + ret = virtio_admin_cmd_exec(virtio_dev, &cmd); > > > + > > > + kfree(in); > > > + return ret; > > > +} > > > + > > > +int virtiovf_cmd_lr_read(struct virtiovf_pci_core_device *virtvdev, u16 opcode, > > > + u8 offset, u8 size, u8 *buf) > > > +{ > > > + struct virtio_device *virtio_dev = > > > + virtio_pci_vf_get_pf_dev(virtvdev->core_device.pdev); > > > + struct virtio_admin_cmd_data_lr_read *in; > > > + struct scatterlist in_sg, out_sg; > > > + struct virtio_admin_cmd cmd = {}; > > > + int ret; > > > + > > > + if (!virtio_dev) > > > + return -ENOTCONN; > > > + > > > + in = kzalloc(sizeof(*in), GFP_KERNEL); > > > + if (!in) > > > + return -ENOMEM; > > > + > > > + in->offset = offset; > > > + sg_init_one(&in_sg, in, sizeof(*in)); > > > + sg_init_one(&out_sg, buf, size); > > > + cmd.opcode = opcode; > > > + cmd.group_type = VIRTIO_ADMIN_GROUP_TYPE_SRIOV; > > > + cmd.data_sg = &in_sg; > > > + cmd.result_sg = &out_sg; > > > + cmd.group_member_id = virtvdev->vf_id + 1; > > > + ret = virtio_admin_cmd_exec(virtio_dev, &cmd); > > > + > > > + kfree(in); > > > + return ret; > > > +} > > > + > > > +int virtiovf_cmd_lq_read_notify(struct virtiovf_pci_core_device *virtvdev, > > > + u8 req_bar_flags, u8 *bar, u64 *bar_offset) > > > +{ > > > + struct virtio_device *virtio_dev = > > > + virtio_pci_vf_get_pf_dev(virtvdev->core_device.pdev); > > > + struct virtio_admin_cmd_notify_info_result *out; > > > + struct scatterlist out_sg; > > > + struct virtio_admin_cmd cmd = {}; > > > + int ret; > > > + > > > + if (!virtio_dev) > > > + return -ENOTCONN; > > > + > > > + out = kzalloc(sizeof(*out), GFP_KERNEL); > > > + if (!out) > > > + return -ENOMEM; > > > + > > > + sg_init_one(&out_sg, out, sizeof(*out)); > > > + cmd.opcode = VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO; > > > + cmd.group_type = VIRTIO_ADMIN_GROUP_TYPE_SRIOV; > > > + cmd.result_sg = &out_sg; > > > + cmd.group_member_id = virtvdev->vf_id + 1; > > > + ret = virtio_admin_cmd_exec(virtio_dev, &cmd); > > > + if (!ret) { > > > + struct virtio_admin_cmd_notify_info_data *entry; > > > + int i; > > > + > > > + ret = -ENOENT; > > > + for (i = 0; i < VIRTIO_ADMIN_CMD_MAX_NOTIFY_INFO; i++) { > > > + entry = &out->entries[i]; > > > + if (entry->flags == VIRTIO_ADMIN_CMD_NOTIFY_INFO_FLAGS_END) > > > + break; > > > + if (entry->flags != req_bar_flags) > > > + continue; > > > + *bar = entry->bar; > > > + *bar_offset = le64_to_cpu(entry->offset); > > > + ret = 0; > > > + break; > > > + } > > > + } > > > + > > > + kfree(out); > > > + return ret; > > > +} > > > diff --git a/drivers/vfio/pci/virtio/cmd.h b/drivers/vfio/pci/virtio/cmd.h > > > new file mode 100644 > > > index 000000000000..c2a3645f4b90 > > > --- /dev/null > > > +++ b/drivers/vfio/pci/virtio/cmd.h > > > @@ -0,0 +1,27 @@ > > > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB > > > +/* > > > + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. > > > + */ > > > + > > > +#ifndef VIRTIO_VFIO_CMD_H > > > +#define VIRTIO_VFIO_CMD_H > > > + > > > +#include <linux/kernel.h> > > > +#include <linux/virtio.h> > > > +#include <linux/vfio_pci_core.h> > > > +#include <linux/virtio_pci.h> > > > + > > > +struct virtiovf_pci_core_device { > > > + struct vfio_pci_core_device core_device; > > > + int vf_id; > > > +}; > > > + > > > +int virtiovf_cmd_list_query(struct pci_dev *pdev, u8 *buf, int buf_size); > > > +int virtiovf_cmd_list_use(struct pci_dev *pdev, u8 *buf, int buf_size); > > > +int virtiovf_cmd_lr_write(struct virtiovf_pci_core_device *virtvdev, u16 opcode, > > > + u8 offset, u8 size, u8 *buf); > > > +int virtiovf_cmd_lr_read(struct virtiovf_pci_core_device *virtvdev, u16 opcode, > > > + u8 offset, u8 size, u8 *buf); > > > +int virtiovf_cmd_lq_read_notify(struct virtiovf_pci_core_device *virtvdev, > > > + u8 req_bar_flags, u8 *bar, u64 *bar_offset); > > > +#endif /* VIRTIO_VFIO_CMD_H */ > > > -- > > > 2.27.0 >
