Michael S. Tsirkin
2023-Sep-21 20:34 UTC
[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device
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 at nvidia.com> > --- > 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.> +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?> + 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?> + 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.> + > +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?> + 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
Yishai Hadas
2023-Sep-26 10:51 UTC
[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device
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 at nvidia.com> >> --- >> 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