thr3ads.net - Virtualization - [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands [Oct 2023]

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

Yishai Hadas

2023-Oct-29 15:59 UTC

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

Initialize the supported admin commands upon activating the admin queue.

The supported commands are saved as part of the admin queue context, it
will be used by the next patches from this series.

Note:
As we don't want to let upper layers to execute admin commands before
that this initialization step was completed, we set ref count to 1 only
post of that flow and use a non ref counted version command for this
internal flow.

Signed-off-by: Yishai Hadas <yishaih at nvidia.com>
---
 drivers/virtio/virtio_pci_common.h |  1 +
 drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
index a21b9ba01a60..9e07e556a51a 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -46,6 +46,7 @@ struct virtio_pci_admin_vq {
 	struct virtio_pci_vq_info info;
 	struct completion flush_done;
 	refcount_t refcount;
+	u64 supported_cmds;
 	/* Name of the admin queue: avq.$index. */
 	char name[10];
 	u16 vq_index;
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index ccd7a4d9f57f..25e27aa79cab 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -19,6 +19,9 @@
 #define VIRTIO_RING_NO_LEGACY
 #include "virtio_pci_common.h"
 
+static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
+				    struct virtio_admin_cmd *cmd);
+
 static u64 vp_get_features(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq *admin_vq,
bool abort)
 	WRITE_ONCE(admin_vq->abort, abort);
 }
 
+static void virtio_pci_admin_init_cmd_list(struct virtio_device *virtio_dev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev);
+	struct virtio_admin_cmd cmd = {};
+	struct scatterlist result_sg;
+	struct scatterlist data_sg;
+	__le64 *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return;
+
+	sg_init_one(&result_sg, data, sizeof(*data));
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY);
+	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
+	cmd.result_sg = &result_sg;
+
+	ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
+	if (ret)
+		goto end;
+
+	sg_init_one(&data_sg, data, sizeof(*data));
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE);
+	cmd.data_sg = &data_sg;
+	cmd.result_sg = NULL;
+
+	ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
+	if (ret)
+		goto end;
+
+	vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data);
+end:
+	kfree(data);
+}
+
 static void vp_modern_avq_activate(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device
*vdev)
 	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
 		return;
 
+	virtio_pci_admin_init_cmd_list(vdev);
 	init_completion(&admin_vq->flush_done);
 	refcount_set(&admin_vq->refcount, 1);
 	vp_modern_avq_set_abort(admin_vq, false);
@@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 	return true;
 }
 
+static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
+				    struct scatterlist **sgs,
+				    unsigned int out_num,
+				    unsigned int in_num,
+				    void *data,
+				    gfp_t gfp)
+{
+	struct virtqueue *vq;
+	int ret, len;
+
+	vq = admin_vq->info.vq;
+
+	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp);
+	if (ret < 0)
+		return ret;
+
+	if (unlikely(!virtqueue_kick(vq)))
+		return -EIO;
+
+	while (!virtqueue_get_buf(vq, &len) &&
+	       !virtqueue_is_broken(vq))
+		cpu_relax();
+
+	if (virtqueue_is_broken(vq))
+		return -EIO;
+
+	return 0;
+}
+
 static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
 				    struct scatterlist **sgs,
 				    unsigned int out_num,
@@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct virtio_device
*vdev,
 		in_num++;
 	}
 
-	ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
+	if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY ||
+	    cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE)
+		ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
+				       out_num, in_num,
+				       sgs, GFP_KERNEL);
+	else
+		ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
 				       out_num, in_num,
 				       sgs, GFP_KERNEL);
 	if (ret) {
-- 
2.27.0

Michael S. Tsirkin

2023-Oct-29 20:17 UTC

head link

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

On Sun, Oct 29, 2023 at 05:59:48PM +0200, Yishai Hadas
wrote:> Initialize the supported admin commands upon activating the admin queue.
> 
> The supported commands are saved as part of the admin queue context, it
> will be used by the next patches from this series.
> 
> Note:
> As we don't want to let upper layers to execute admin commands before
> that this initialization step was completed, we set ref count to 1 only
> post of that flow and use a non ref counted version command for this
> internal flow.
> 
> Signed-off-by: Yishai Hadas <yishaih at nvidia.com>
> ---
>  drivers/virtio/virtio_pci_common.h |  1 +
>  drivers/virtio/virtio_pci_modern.c | 77 +++++++++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
> index a21b9ba01a60..9e07e556a51a 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -46,6 +46,7 @@ struct virtio_pci_admin_vq {
>  	struct virtio_pci_vq_info info;
>  	struct completion flush_done;
>  	refcount_t refcount;
> +	u64 supported_cmds;
>  	/* Name of the admin queue: avq.$index. */
>  	char name[10];
>  	u16 vq_index;
> diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
> index ccd7a4d9f57f..25e27aa79cab 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -19,6 +19,9 @@
>  #define VIRTIO_RING_NO_LEGACY
>  #include "virtio_pci_common.h"
>  
> +static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
> +				    struct virtio_admin_cmd *cmd);
> +
I don't much like forward declarations. Just order functions sensibly
and they will not be needed.
>  static u64 vp_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> @@ -59,6 +62,42 @@ vp_modern_avq_set_abort(struct virtio_pci_admin_vq
*admin_vq, bool abort)
>  	WRITE_ONCE(admin_vq->abort, abort);
>  }
>  
> +static void virtio_pci_admin_init_cmd_list(struct virtio_device
*virtio_dev)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev);
> +	struct virtio_admin_cmd cmd = {};
> +	struct scatterlist result_sg;
> +	struct scatterlist data_sg;
> +	__le64 *data;
> +	int ret;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return;
> +
> +	sg_init_one(&result_sg, data, sizeof(*data));
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_QUERY);
> +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SRIOV);
> +	cmd.result_sg = &result_sg;
> +
> +	ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
> +	if (ret)
> +		goto end;
> +
> +	sg_init_one(&data_sg, data, sizeof(*data));
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_LIST_USE);
> +	cmd.data_sg = &data_sg;
> +	cmd.result_sg = NULL;
> +
> +	ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
> +	if (ret)
> +		goto end;
> +
> +	vp_dev->admin_vq.supported_cmds = le64_to_cpu(*data);
> +end:
> +	kfree(data);
> +}
> +
>  static void vp_modern_avq_activate(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> @@ -67,6 +106,7 @@ static void vp_modern_avq_activate(struct virtio_device
*vdev)
>  	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
>  		return;
>  
> +	virtio_pci_admin_init_cmd_list(vdev);
>  	init_completion(&admin_vq->flush_done);
>  	refcount_set(&admin_vq->refcount, 1);
>  	vp_modern_avq_set_abort(admin_vq, false);
> @@ -562,6 +602,35 @@ static bool vp_get_shm_region(struct virtio_device
*vdev,
>  	return true;
>  }
>  
> +static int __virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq
*admin_vq,
> +				    struct scatterlist **sgs,
> +				    unsigned int out_num,
> +				    unsigned int in_num,
> +				    void *data,
> +				    gfp_t gfp)
> +{
> +	struct virtqueue *vq;
> +	int ret, len;
> +
> +	vq = admin_vq->info.vq;
> +
> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, gfp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (unlikely(!virtqueue_kick(vq)))
> +		return -EIO;
> +
> +	while (!virtqueue_get_buf(vq, &len) &&
> +	       !virtqueue_is_broken(vq))
> +		cpu_relax();
> +
> +	if (virtqueue_is_broken(vq))
> +		return -EIO;
> +
> +	return 0;
> +}
> +

This is tolerable I guess but it might pin the CPU for a long time.
The difficulty is handling suprize removal well which we currently
don't do with interrupts. I would say it's ok as is but add
a TODO comments along the lines of /* TODO: use interrupts once these
virtqueue_is_broken */
>  static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
>  				    struct scatterlist **sgs,
>  				    unsigned int out_num,
> @@ -653,7 +722,13 @@ static int vp_modern_admin_cmd_exec(struct
virtio_device *vdev,
>  		in_num++;
>  	}
>  
> -	ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
> +	if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY ||
> +	    cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE)
> +		ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
> +				       out_num, in_num,
> +				       sgs, GFP_KERNEL);
> +	else
> +		ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
>  				       out_num, in_num,
>  				       sgs, GFP_KERNEL);
>  	if (ret) {
> -- 
> 2.27.0

kernel test robot

2023-Nov-03 00:33 UTC

head link

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

Hi Yishai,

kernel test robot noticed the following build warnings:

[auto build test WARNING on awilliam-vfio/for-linus]
[also build test WARNING on linus/master v6.6]
[cannot apply to awilliam-vfio/next mst-vhost/linux-next next-20231102]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:   
https://github.com/intel-lab-lkp/linux/commits/Yishai-Hadas/virtio-Define-feature-bit-for-administration-virtqueue/20231030-000414
base:   https://github.com/awilliam/linux-vfio.git for-linus
patch link:   
https://lore.kernel.org/r/20231029155952.67686-6-yishaih%40nvidia.com
patch subject: [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin
commands
config: i386-randconfig-061-20231102
(https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp at
intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20231103/202311030838.GjyaBTjM-lkp at
intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp at intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311030838.GjyaBTjM-lkp at
intel.com/

sparse warnings: (new ones prefixed by >>)>> drivers/virtio/virtio_pci_modern.c:726:16: sparse: sparse: restricted
__le16 degrades to integer
vim +726 drivers/virtio/virtio_pci_modern.c

   673	
   674	static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
   675					    struct virtio_admin_cmd *cmd)
   676	{
   677		struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat;
   678		struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   679		struct virtio_admin_cmd_status *va_status;
   680		unsigned int out_num = 0, in_num = 0;
   681		struct virtio_admin_cmd_hdr *va_hdr;
   682		struct virtqueue *avq;
   683		u16 status;
   684		int ret;
   685	
   686		avq = virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ) ?
   687			vp_dev->admin_vq.info.vq : NULL;
   688		if (!avq)
   689			return -EOPNOTSUPP;
   690	
   691		va_status = kzalloc(sizeof(*va_status), GFP_KERNEL);
   692		if (!va_status)
   693			return -ENOMEM;
   694	
   695		va_hdr = kzalloc(sizeof(*va_hdr), GFP_KERNEL);
   696		if (!va_hdr) {
   697			ret = -ENOMEM;
   698			goto err_alloc;
   699		}
   700	
   701		va_hdr->opcode = cmd->opcode;
   702		va_hdr->group_type = cmd->group_type;
   703		va_hdr->group_member_id = cmd->group_member_id;
   704	
   705		/* Add header */
   706		sg_init_one(&hdr, va_hdr, sizeof(*va_hdr));
   707		sgs[out_num] = &hdr;
   708		out_num++;
   709	
   710		if (cmd->data_sg) {
   711			sgs[out_num] = cmd->data_sg;
   712			out_num++;
   713		}
   714	
   715		/* Add return status */
   716		sg_init_one(&stat, va_status, sizeof(*va_status));
   717		sgs[out_num + in_num] = &stat;
   718		in_num++;
   719	
   720		if (cmd->result_sg) {
   721			sgs[out_num + in_num] = cmd->result_sg;
   722			in_num++;
   723		}
   724	
   725		if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY ||
 > 726		    cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE)
   727			ret = __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
   728					       out_num, in_num,
   729					       sgs, GFP_KERNEL);
   730		else
   731			ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
   732					       out_num, in_num,
   733					       sgs, GFP_KERNEL);
   734		if (ret) {
   735			dev_err(&vdev->dev,
   736				"Failed to execute command on admin vq: %d\n.", ret);
   737			goto err_cmd_exec;
   738		}
   739	
   740		status = le16_to_cpu(va_status->status);
   741		if (status != VIRTIO_ADMIN_STATUS_OK) {
   742			dev_err(&vdev->dev,
   743				"admin command error: status(%#x) qualifier(%#x)\n",
   744				status, le16_to_cpu(va_status->status_qualifier));
   745			ret = -status;
   746		}
   747	
   748	err_cmd_exec:
   749		kfree(va_hdr);
   750	err_alloc:
   751		kfree(va_status);
   752		return ret;
   753	}
   754	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Virtualization - Oct 2023 - [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands

[PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands