thr3ads.net - Virtualization - [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device [Sep 2023]

If this information is useful, please help other people find it:
Share via:

Yishai Hadas

2023-Sep-26 11:14 UTC

[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

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 at nvidia.com>
>
> 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.

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).

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 way, we can enable further commands to be added/extended 
easily/cleanly.

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

Michael S. Tsirkin

2023-Sep-26 11:41 UTC

head link

[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

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 at nvidia.com>
> > 
> > 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
>

Virtualization - Sep 2023 - [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

[PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device