From: "Michael S. Tsirkin" <mst@redhat.com>
To: Yishai Hadas <yishaih@nvidia.com>
Cc: kvm@vger.kernel.org, maorg@nvidia.com,
virtualization@lists.linux-foundation.org, jgg@nvidia.com,
jiri@nvidia.com, leonro@nvidia.com
Subject: Re: [PATCH V2 vfio 6/9] virtio-pci: Introduce APIs to execute legacy IO admin commands
Date: Tue, 31 Oct 2023 04:09:13 -0400 [thread overview]
Message-ID: <20231031040403-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20231029155952.67686-7-yishaih@nvidia.com>
On Sun, Oct 29, 2023 at 05:59:49PM +0200, Yishai Hadas wrote:
> Introduce APIs to execute legacy IO admin commands.
>
> It includes: io_legacy_read/write for both common and the device
> registers, io_legacy_notify_info.
>
> In addition, exposing an API to check whether the legacy IO commands are
> supported. (i.e. virtio_pci_admin_has_legacy_io()).
>
> Those APIs will be used by the next patches from this series.
>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
> drivers/virtio/virtio_pci_common.c | 11 ++
> drivers/virtio/virtio_pci_common.h | 2 +
> drivers/virtio/virtio_pci_modern.c | 241 +++++++++++++++++++++++++++++
> include/linux/virtio_pci_admin.h | 21 +++
> 4 files changed, 275 insertions(+)
> create mode 100644 include/linux/virtio_pci_admin.h
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 6b4766d5abe6..212d68401d2c 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -645,6 +645,17 @@ static struct pci_driver virtio_pci_driver = {
> .sriov_configure = virtio_pci_sriov_configure,
> };
>
> +struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev)
> +{
> + struct virtio_pci_device *pf_vp_dev;
> +
> + pf_vp_dev = pci_iov_get_pf_drvdata(pdev, &virtio_pci_driver);
> + if (IS_ERR(pf_vp_dev))
> + return NULL;
> +
> + return &pf_vp_dev->vdev;
> +}
> +
> module_pci_driver(virtio_pci_driver);
>
> MODULE_AUTHOR("Anthony Liguori <aliguori@us.ibm.com>");
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 9e07e556a51a..07d4f863ac44 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -156,4 +156,6 @@ static inline void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> int virtio_pci_modern_probe(struct virtio_pci_device *);
> void virtio_pci_modern_remove(struct virtio_pci_device *);
>
> +struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
> +
> #endif
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 25e27aa79cab..def0f2de6091 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -15,6 +15,7 @@
> */
>
> #include <linux/delay.h>
> +#include <linux/virtio_pci_admin.h>
> #define VIRTIO_PCI_NO_LEGACY
> #define VIRTIO_RING_NO_LEGACY
> #include "virtio_pci_common.h"
> @@ -794,6 +795,246 @@ static void vp_modern_destroy_avq(struct virtio_device *vdev)
> vp_dev->del_vq(&vp_dev->admin_vq.info);
> }
>
> +#define VIRTIO_LEGACY_ADMIN_CMD_BITMAP \
> + (BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
> +
> +/*
> + * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
> + * commands are supported
> + * @dev: VF pci_dev
> + *
> + * Returns true on success.
> + */
> +bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev)
> +{
> + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
> + struct virtio_pci_device *vp_dev;
> +
> + if (!virtio_dev)
> + return false;
> +
> + if (!virtio_has_feature(virtio_dev, VIRTIO_F_ADMIN_VQ))
> + return false;
> +
> + vp_dev = to_vp_device(virtio_dev);
> +
> + if ((vp_dev->admin_vq.supported_cmds & VIRTIO_LEGACY_ADMIN_CMD_BITMAP) ==
> + VIRTIO_LEGACY_ADMIN_CMD_BITMAP)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
> +
> +static int virtio_pci_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 *data;
> + struct virtio_admin_cmd cmd = {};
> + struct scatterlist data_sg;
> + int vf_id;
> + int ret;
> +
> + if (!virtio_dev)
> + return -ENODEV;
> +
> + vf_id = pci_iov_vf_id(pdev);
> + if (vf_id < 0)
> + return vf_id;
> +
> + data = kzalloc(sizeof(*data) + size, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->offset = offset;
> + memcpy(data->registers, buf, size);
> + sg_init_one(&data_sg, data, sizeof(*data) + size);
> + cmd.opcode = cpu_to_le16(opcode);
> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
> + cmd.group_member_id = cpu_to_le64(vf_id + 1);
> + cmd.data_sg = &data_sg;
> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
> +
> + kfree(data);
> + return ret;
> +}
> +
> +/*
> + * virtio_pci_admin_legacy_io_write_common - Write common legacy registers
> + * of a member device
> + * @dev: VF pci_dev
> + * @offset: starting byte offset within the registers to write to
> + * @size: size of the data to write
> + * @buf: buffer which holds the data
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int virtio_pci_admin_legacy_common_io_write(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf)
> +{
> + return virtio_pci_admin_legacy_io_write(pdev,
> + VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> + offset, size, buf);
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_legacy_common_io_write);
So consider this for example. You start with a PCI device of a VF.
Any number of these will access a PF in parallel. No locking
is taking place so admin vq can get corrupted.
And further, is caller expected not to invoke several of these
in parallel on the same device? If yes this needs to be
documented. I don't see where does vfio enforce this if yes.
> +
> +/*
> + * virtio_pci_admin_legacy_io_write_device - Write device legacy registers
> + * of a member device
> + * @dev: VF pci_dev
> + * @offset: starting byte offset within the registers to write to
> + * @size: size of the data to write
> + * @buf: buffer which holds the data
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int virtio_pci_admin_legacy_device_io_write(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf)
> +{
> + return virtio_pci_admin_legacy_io_write(pdev,
> + VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE,
> + offset, size, buf);
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_legacy_device_io_write);
> +
> +static int virtio_pci_admin_legacy_io_read(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_rd_data *data;
> + struct scatterlist data_sg, result_sg;
> + struct virtio_admin_cmd cmd = {};
> + int vf_id;
> + int ret;
> +
> + if (!virtio_dev)
> + return -ENODEV;
> +
> + vf_id = pci_iov_vf_id(pdev);
> + if (vf_id < 0)
> + return vf_id;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->offset = offset;
> + sg_init_one(&data_sg, data, sizeof(*data));
> + sg_init_one(&result_sg, buf, size);
> + cmd.opcode = cpu_to_le16(opcode);
> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
> + cmd.group_member_id = cpu_to_le64(vf_id + 1);
> + cmd.data_sg = &data_sg;
> + cmd.result_sg = &result_sg;
> + ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
> +
> + kfree(data);
> + return ret;
> +}
> +
> +/*
> + * virtio_pci_admin_legacy_device_io_read - Read legacy device registers of
> + * a member device
> + * @dev: VF pci_dev
> + * @offset: starting byte offset within the registers to read from
> + * @size: size of the data to be read
> + * @buf: buffer to hold the returned data
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int virtio_pci_admin_legacy_device_io_read(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf)
> +{
> + return virtio_pci_admin_legacy_io_read(pdev,
> + VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> + offset, size, buf);
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_legacy_device_io_read);
> +
> +/*
> + * virtio_pci_admin_legacy_common_io_read - Read legacy common registers of
> + * a member device
> + * @dev: VF pci_dev
> + * @offset: starting byte offset within the registers to read from
> + * @size: size of the data to be read
> + * @buf: buffer to hold the returned data
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int virtio_pci_admin_legacy_common_io_read(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf)
> +{
> + return virtio_pci_admin_legacy_io_read(pdev,
> + VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> + offset, size, buf);
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_legacy_common_io_read);
> +
> +/*
> + * virtio_pci_admin_legacy_io_notify_info - Read the queue notification
> + * information for legacy interface
> + * @dev: VF pci_dev
> + * @req_bar_flags: requested bar flags
> + * @bar: on output the BAR number of the member device
> + * @bar_offset: on output the offset within bar
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int virtio_pci_admin_legacy_io_notify_info(struct pci_dev *pdev,
> + u8 req_bar_flags, u8 *bar,
> + u64 *bar_offset)
> +{
> + struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
> + struct virtio_admin_cmd_notify_info_result *result;
> + struct virtio_admin_cmd cmd = {};
> + struct scatterlist result_sg;
> + int vf_id;
> + int ret;
> +
> + if (!virtio_dev)
> + return -ENODEV;
> +
> + vf_id = pci_iov_vf_id(pdev);
> + if (vf_id < 0)
> + return vf_id;
> +
> + result = kzalloc(sizeof(*result), GFP_KERNEL);
> + if (!result)
> + return -ENOMEM;
> +
> + sg_init_one(&result_sg, result, sizeof(*result));
> + cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO);
> + cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
> + cmd.group_member_id = cpu_to_le64(vf_id + 1);
> + cmd.result_sg = &result_sg;
> + ret = vp_modern_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 = &result->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(result);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(virtio_pci_admin_legacy_io_notify_info);
> +
> static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> .get = NULL,
> .set = NULL,
> diff --git a/include/linux/virtio_pci_admin.h b/include/linux/virtio_pci_admin.h
> new file mode 100644
> index 000000000000..446ced8cb050
> --- /dev/null
> +++ b/include/linux/virtio_pci_admin.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_VIRTIO_PCI_ADMIN_H
> +#define _LINUX_VIRTIO_PCI_ADMIN_H
> +
> +#include <linux/types.h>
> +#include <linux/pci.h>
> +
> +bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev);
> +int virtio_pci_admin_legacy_common_io_write(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf);
> +int virtio_pci_admin_legacy_common_io_read(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf);
> +int virtio_pci_admin_legacy_device_io_write(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf);
> +int virtio_pci_admin_legacy_device_io_read(struct pci_dev *pdev, u8 offset,
> + u8 size, u8 *buf);
> +int virtio_pci_admin_legacy_io_notify_info(struct pci_dev *pdev,
> + u8 req_bar_flags, u8 *bar,
> + u64 *bar_offset);
> +
> +#endif /* _LINUX_VIRTIO_PCI_ADMIN_H */
> --
> 2.27.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2023-10-31 8:09 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-29 15:59 [PATCH V2 vfio 0/9] Introduce a vfio driver over virtio devices Yishai Hadas via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 1/9] virtio: Define feature bit for administration virtqueue Yishai Hadas via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue Yishai Hadas via Virtualization
2023-10-29 20:22 ` Michael S. Tsirkin
2023-10-30 15:51 ` Parav Pandit via Virtualization
2023-10-30 15:59 ` Michael S. Tsirkin
2023-10-30 18:10 ` Parav Pandit via Virtualization
2023-10-30 23:31 ` Michael S. Tsirkin
2023-10-31 3:11 ` Parav Pandit via Virtualization
2023-10-31 7:59 ` Michael S. Tsirkin
2023-10-31 12:11 ` Parav Pandit via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 3/9] virtio-pci: Introduce admin command sending function Yishai Hadas via Virtualization
2023-10-29 20:26 ` Michael S. Tsirkin
2023-10-29 15:59 ` [PATCH V2 vfio 4/9] virtio-pci: Introduce admin commands Yishai Hadas via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported " Yishai Hadas via Virtualization
2023-10-29 20:17 ` Michael S. Tsirkin
2023-10-30 15:27 ` Yishai Hadas via Virtualization
2023-10-30 15:57 ` Michael S. Tsirkin
2023-10-30 16:06 ` Yishai Hadas via Virtualization
2023-10-30 23:30 ` Michael S. Tsirkin
2023-11-03 0:33 ` kernel test robot
2023-11-03 7:39 ` Michael S. Tsirkin
2023-10-29 15:59 ` [PATCH V2 vfio 6/9] virtio-pci: Introduce APIs to execute legacy IO " Yishai Hadas via Virtualization
2023-10-31 8:09 ` Michael S. Tsirkin [this message]
2023-10-31 8:30 ` Yishai Hadas via Virtualization
2023-10-31 9:00 ` Michael S. Tsirkin
2023-10-29 15:59 ` [PATCH V2 vfio 7/9] vfio/pci: Expose vfio_pci_core_setup_barmap() Yishai Hadas via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 8/9] vfio/pci: Expose vfio_pci_iowrite/read##size() Yishai Hadas via Virtualization
2023-10-29 15:59 ` [PATCH V2 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices Yishai Hadas via Virtualization
[not found] ` <4a5ab8d6-1138-4f4f-bb61-9ec0309d46e6@intel.com>
2023-10-31 8:23 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231031040403-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=jgg@nvidia.com \
--cc=jiri@nvidia.com \
--cc=kvm@vger.kernel.org \
--cc=leonro@nvidia.com \
--cc=maorg@nvidia.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=yishaih@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).