On 21/09/2023 23:34, 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>
---
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);
+}
+
in/out seem all wrong here. In virtio terminology, in means from
device to driver, out means from driver to device.
I referred here to in/out from vfio POV who prepares the command.
However, I can replace it to follow the virtio terminology as you
suggested if this more makes sense.
Please see also my coming answer on your suggestion to put all of this
in the virtio layer.
+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,
what is _lr short for?
This was an acronym to legacy_read.
The actual command is according to the given opcode which can be one
among LEGACY_COMMON_CFG_READ, LEGACY_DEV_CFG_READ.
I can rename it to '_legacy_read' (i.e. virtiovf_issue_legacy_read_cmd)
to be clearer.
+ 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;
weird. why + 1?
This follows the virtio spec in that area.
"When sending commands with the SR-IOV group type, the driver specify a
value for group_member_id
between 1 and NumVFs inclusive."
The 'virtvdev->vf_id' was set upon vfio/virtio driver initialization by
pci_iov_vf_id() which its first index is 0.
+ cmd.data_sg = &in_sg;
+ ret = virtio_admin_cmd_exec(virtio_dev, &cmd);
+
+ kfree(in);
+ return ret;
+}
How do you know it's safe to send this command, in particular at
this time? This seems to be doing zero checks, and zero synchronization
with the PF driver.
The virtiovf_cmd_lr_read()/other gets a virtio VF and it gets its PF by
calling virtio_pci_vf_get_pf_dev().
The VF can't gone by 'disable sriov' as it's owned/used by vfio.
The PF can't gone by rmmod/modprobe -r of virtio, as of the 'module in
use'/dependencies between VFIO to VIRTIO.
The below check [1] was done only from a clean code perspective, which
might theoretically fail in case the given VF doesn't use a virtio driver.
[1] if (!virtio_dev)
return -ENOTCONN;
So, it looks safe as is.
+
+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,
and what is lq short for?
To be more explicit, I may replace to virtiovf_cmd_legacy_notify_info()
to follow the spec opcode.
Yishai
+ 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
