Re: [Patch v1 1/3] vdpa/mlx5: Remove redundant header file inclusion

Re: [Patch v1 1/3] vdpa/mlx5: Remove redundant header file inclusion

[Jason Wang]

在 2021/8/9 下午10:07, Eli Cohen 写道:
> linux/if_vlan.h is not required.
> Remove it.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   drivers/vdpa/mlx5/core/mlx5_vdpa.h | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index 0002b2136b48..8d0a6f2cb3f0 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -5,7 +5,6 @@
>   #define __MLX5_VDPA_H__
>   
>   #include <linux/etherdevice.h>
> -#include <linux/if_vlan.h>
>   #include <linux/vdpa.h>
>   #include <linux/mlx5/driver.h>
>   

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 2/3] vdpa/mlx5: Add support for control VQ and MAC setting

[Jason Wang]

在 2021/8/9 下午10:07, Eli Cohen 写道:
> Add support to handle control virtqueue configurations per virtio
> specification. The control virtqueue is implemented in software and no
> hardware offloading is involved.
>
> Control VQ configuration need task context, therefore all configurations
> are handled in a workqueue created for the purpose.
>
> Modifications are made to the memory registration code to allow for
> saving a copy of itolb to be used by the control VQ to access the vring.
>
> The max number of data virtqueus supported by the driver has been
> updated to 2 since multiqueue is not supported at this stage and we need
> to ensure consistency of VQ indices mapping to either data or control
> VQ.
>
> v0 --> v1:
> cleanup some leftover code
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |  23 +++
>   drivers/vdpa/mlx5/core/mr.c        |  87 ++++++--
>   drivers/vdpa/mlx5/core/resources.c |  31 +++
>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 307 +++++++++++++++++++++++++----
>   4 files changed, 391 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index 8d0a6f2cb3f0..71bb29fcf4ca 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -5,6 +5,7 @@
>   #define __MLX5_VDPA_H__
>   
>   #include <linux/etherdevice.h>
> +#include <linux/vringh.h>
>   #include <linux/vdpa.h>
>   #include <linux/mlx5/driver.h>
>   
> @@ -47,6 +48,26 @@ struct mlx5_vdpa_resources {
>   	bool valid;
>   };
>   
> +struct mlx5_control_vq {
> +	struct vhost_iotlb *iotlb;
> +	/* spinlock to synchronize iommu table */
> +	spinlock_t iommu_lock;
> +	struct vringh vring;
> +	bool ready;
> +	u64 desc_addr;
> +	u64 device_addr;
> +	u64 driver_addr;
> +	struct vdpa_callback event_cb;
> +	struct vringh_kiov riov;
> +	struct vringh_kiov wiov;
> +	unsigned short head;
> +};
> +
> +struct mlx5_ctrl_wq_ent {
> +	struct work_struct work;
> +	struct mlx5_vdpa_dev *mvdev;
> +};
> +
>   struct mlx5_vdpa_dev {
>   	struct vdpa_device vdev;
>   	struct mlx5_core_dev *mdev;
> @@ -59,6 +80,8 @@ struct mlx5_vdpa_dev {
>   	u32 generation;
>   
>   	struct mlx5_vdpa_mr mr;
> +	struct mlx5_control_vq cvq;
> +	struct workqueue_struct *wq;
>   };
>   
>   int mlx5_vdpa_alloc_pd(struct mlx5_vdpa_dev *dev, u32 *pdn, u16 uid);
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index dcee6039e966..46a657ebb1df 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>   /* Copyright (c) 2020 Mellanox Technologies Ltd. */
>   
> +#include <linux/vhost_types.h>
>   #include <linux/vdpa.h>
>   #include <linux/gcd.h>
>   #include <linux/string.h>
> @@ -451,33 +452,38 @@ static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>   	mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
>   }
>   
> -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +static int dup_iotlb(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *src)
>   {
> -	struct mlx5_vdpa_mr *mr = &mvdev->mr;
> +	struct vhost_iotlb_map *map;
> +	u64 start = 0ULL, last = 0ULL - 1;
>   	int err;
>   
> -	if (mr->initialized)
> -		return 0;
> -
> -	if (iotlb)
> -		err = create_user_mr(mvdev, iotlb);
> -	else
> -		err = create_dma_mr(mvdev, mr);
> -
> -	if (!err)
> -		mr->initialized = true;
> +	if (!src) {
> +		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last, start, VHOST_ACCESS_RW);
> +		return err;
> +	}
>   
> -	return err;
> +	for (map = vhost_iotlb_itree_first(src, start, last); map;
> +		map = vhost_iotlb_itree_next(map, start, last)) {
> +		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start, map->last,
> +					    map->addr, map->perm);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
>   }
>   
> -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +static void prune_iotlb(struct mlx5_vdpa_dev *mvdev)
>   {
> -	int err;
> +	struct mlx5_vdpa_mr *mr = &mvdev->mr;
> +	u64 start = 0ULL, last = 0ULL - 1;
>   
> -	mutex_lock(&mvdev->mr.mkey_mtx);
> -	err = _mlx5_vdpa_create_mr(mvdev, iotlb);
> -	mutex_unlock(&mvdev->mr.mkey_mtx);
> -	return err;
> +	if (!mr->user_mr) {
> +		vhost_iotlb_del_range(mvdev->cvq.iotlb, start, last);
> +		return;
> +	}
> +
> +	vhost_iotlb_del_range(mvdev->cvq.iotlb, start, last);


It looks to me we don't need check of if (!mr->user_mr) here.


>   }
>   
>   static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
> @@ -501,6 +507,7 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>   	if (!mr->initialized)
>   		goto out;
>   
> +	prune_iotlb(mvdev);
>   	if (mr->user_mr)
>   		destroy_user_mr(mvdev, mr);
>   	else
> @@ -512,6 +519,48 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>   	mutex_unlock(&mr->mkey_mtx);
>   }
>   
> +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +{
> +	struct mlx5_vdpa_mr *mr = &mvdev->mr;
> +	int err;
> +
> +	if (mr->initialized)
> +		return 0;
> +
> +	if (iotlb)
> +		err = create_user_mr(mvdev, iotlb);
> +	else
> +		err = create_dma_mr(mvdev, mr);
> +
> +	if (err)
> +		return err;
> +
> +	err = dup_iotlb(mvdev, iotlb);
> +	if (err)
> +		goto out_err;
> +
> +	mr->initialized = true;
> +	return 0;
> +
> +out_err:
> +	if (iotlb)
> +		destroy_user_mr(mvdev, mr);
> +	else
> +		destroy_dma_mr(mvdev, mr);
> +
> +	return err;
> +}
> +
> +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
> +{
> +	int err;
> +
> +	mutex_lock(&mvdev->mr.mkey_mtx);
> +	err = _mlx5_vdpa_create_mr(mvdev, iotlb);
> +	mutex_unlock(&mvdev->mr.mkey_mtx);
> +	return err;
> +}
> +
>   static bool map_empty(struct vhost_iotlb *iotlb)
>   {
>   	return !vhost_iotlb_itree_first(iotlb, 0, U64_MAX);
> diff --git a/drivers/vdpa/mlx5/core/resources.c b/drivers/vdpa/mlx5/core/resources.c
> index d4606213f88a..d24ae1a85159 100644
> --- a/drivers/vdpa/mlx5/core/resources.c
> +++ b/drivers/vdpa/mlx5/core/resources.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>   /* Copyright (c) 2020 Mellanox Technologies Ltd. */
>   
> +#include <linux/iova.h>
>   #include <linux/mlx5/driver.h>
>   #include "mlx5_vdpa.h"
>   
> @@ -221,6 +222,28 @@ int mlx5_vdpa_destroy_mkey(struct mlx5_vdpa_dev *mvdev, struct mlx5_core_mkey *m
>   	return mlx5_cmd_exec_in(mvdev->mdev, destroy_mkey, in);
>   }
>   
> +static int init_ctrl_vq(struct mlx5_vdpa_dev *mvdev)
> +{
> +	int err;
> +
> +	mvdev->cvq.iotlb = vhost_iotlb_alloc(0, 0);
> +	if (!mvdev->cvq.iotlb)
> +		return -ENOMEM;
> +
> +	vringh_set_iotlb(&mvdev->cvq.vring, mvdev->cvq.iotlb, &mvdev->cvq.iommu_lock);
> +	err = iova_cache_get();
> +	if (err)
> +		vhost_iotlb_free(mvdev->cvq.iotlb);
> +
> +	return err;
> +}
> +
> +static void cleanup_ctrl_vq(struct mlx5_vdpa_dev *mvdev)
> +{
> +	iova_cache_put();
> +	vhost_iotlb_free(mvdev->cvq.iotlb);
> +}
> +
>   int mlx5_vdpa_alloc_resources(struct mlx5_vdpa_dev *mvdev)
>   {
>   	u64 offset = MLX5_CAP64_DEV_VDPA_EMULATION(mvdev->mdev, doorbell_bar_offset);
> @@ -260,10 +283,17 @@ int mlx5_vdpa_alloc_resources(struct mlx5_vdpa_dev *mvdev)
>   		err = -ENOMEM;
>   		goto err_key;
>   	}
> +
> +	err = init_ctrl_vq(mvdev);
> +	if (err)
> +		goto err_ctrl;
> +
>   	res->valid = true;
>   
>   	return 0;
>   
> +err_ctrl:
> +	iounmap(res->kick_addr);
>   err_key:
>   	dealloc_pd(mvdev, res->pdn, res->uid);
>   err_pd:
> @@ -282,6 +312,7 @@ void mlx5_vdpa_free_resources(struct mlx5_vdpa_dev *mvdev)
>   	if (!res->valid)
>   		return;
>   
> +	cleanup_ctrl_vq(mvdev);
>   	iounmap(res->kick_addr);
>   	res->kick_addr = NULL;
>   	dealloc_pd(mvdev, res->pdn, res->uid);
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 2a31467f7ac5..46448b079aca 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -133,7 +133,7 @@ struct mlx5_vdpa_virtqueue {
>   /* We will remove this limitation once mlx5_vdpa_alloc_resources()
>    * provides for driver space allocation
>    */
> -#define MLX5_MAX_SUPPORTED_VQS 16
> +#define MLX5_MAX_SUPPORTED_VQS 2
>   
>   struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
> @@ -151,15 +151,18 @@ struct mlx5_vdpa_net {
>   	struct mlx5_flow_handle *rx_rule;
>   	bool setup;
>   	u16 mtu;
> +	u32 cur_num_vqs;
>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
>   static void init_mvqs(struct mlx5_vdpa_net *ndev);
> -static int setup_driver(struct mlx5_vdpa_net *ndev);
> +static int setup_driver(struct mlx5_vdpa_dev *mvdev);
>   static void teardown_driver(struct mlx5_vdpa_net *ndev);
>   
>   static bool mlx5_vdpa_debug;
>   
> +#define MLX5_CVQ_MAX_ENT 16
> +
>   #define MLX5_LOG_VIO_FLAG(_feature)                                                                \
>   	do {                                                                                       \
>   		if (features & BIT_ULL(_feature))                                                  \
> @@ -172,11 +175,33 @@ static bool mlx5_vdpa_debug;
>   			mlx5_vdpa_info(mvdev, "%s\n", #_status);                                   \
>   	} while (0)
>   
> +/* TODO: cross-endian support */
> +static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
> +{
> +	return virtio_legacy_is_little_endian() ||
> +		(mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
> +}
> +
> +static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
> +{
> +	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> +}
> +
>   static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>   {
>   	return max_vqs / 2;
>   }
>   
> +static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> +{
> +	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> +}
> +
> +static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> +{
> +	return idx == ctrl_vq_idx(mvdev);
> +}
> +
>   static void print_status(struct mlx5_vdpa_dev *mvdev, u8 status, bool set)
>   {
>   	if (status & ~VALID_STATUS_MASK)
> @@ -1346,12 +1371,139 @@ static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   	ndev->rx_rule = NULL;
>   }
>   
> +static int update_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> +{
> +	remove_fwd_to_tir(ndev);
> +	return add_fwd_to_tir(ndev);
> +}
> +
> +virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> +{
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +	struct mlx5_core_dev *pfmdev;
> +	size_t read;
> +	u8 mac[6];
> +
> +	pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
> +	switch (cmd) {
> +	case VIRTIO_NET_CTRL_MAC_ADDR_SET:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)mac, ETH_ALEN);
> +		if (read != ETH_ALEN)
> +			break;
> +
> +		if (!memcmp(ndev->config.mac, mac, 6)) {
> +			status = VIRTIO_NET_OK;
> +			break;
> +		}
> +
> +		if (!is_zero_ether_addr(ndev->config.mac)) {
> +			if (mlx5_mpfs_del_mac(pfmdev, ndev->config.mac)) {
> +				mlx5_vdpa_warn(mvdev, "failed to delete old MAC %pM from MPFS table\n",
> +					       ndev->config.mac);
> +				break;
> +			}
> +		}
> +
> +		if (mlx5_mpfs_add_mac(pfmdev, mac)) {
> +			mlx5_vdpa_warn(mvdev, "failed to insert new MAC %pM into MPFS table\n",
> +				       mac);
> +			break;
> +		}
> +
> +		memcpy(ndev->config.mac, mac, 6);


Let's use ETH_ALEN.


> +		if (!update_fwd_to_tir(ndev))
> +			status = VIRTIO_NET_OK;


I think it's better to to update config after the succeed of 
update_fwd_to_tir().


> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return status;
> +}
> +
> +static void mlx5_cvq_kick_handler(struct work_struct *work)
> +{
> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +	struct virtio_net_ctrl_hdr ctrl;
> +	struct mlx5_ctrl_wq_ent *wqent;
> +	struct mlx5_vdpa_dev *mvdev;
> +	struct mlx5_control_vq *cvq;
> +	struct mlx5_vdpa_net *ndev;
> +	size_t read, write;
> +	int err;
> +
> +	wqent = container_of(work, struct mlx5_ctrl_wq_ent, work);
> +	mvdev = wqent->mvdev;
> +	ndev = to_mlx5_vdpa_ndev(mvdev);
> +	cvq = &mvdev->cvq;
> +	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> +		goto out;
> +
> +	if (!cvq->ready)
> +		goto out;
> +
> +	while (true) {
> +		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> +					   GFP_ATOMIC);
> +		if (err <= 0)
> +			break;
> +
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, &ctrl, sizeof(ctrl));
> +		if (read != sizeof(ctrl))
> +			break;
> +
> +		switch (ctrl.class) {
> +		case VIRTIO_NET_CTRL_MAC:
> +			status = handle_ctrl_mac(mvdev, ctrl.cmd);
> +			break;
> +
> +		default:
> +			break;
> +		}
> +
> +		/* Make sure data is written before advancing index */
> +		smp_wmb();
> +
> +		write = vringh_iov_push_iotlb(&cvq->vring, &cvq->wiov, &status, sizeof(status));
> +		vringh_complete_iotlb(&cvq->vring, cvq->head, write);
> +		vringh_kiov_cleanup(&cvq->riov);
> +		vringh_kiov_cleanup(&cvq->wiov);
> +
> +		/* Make sure used is visible before rasing the interrupt. */
> +		smp_wmb();
> +
> +		local_bh_disable();
> +		if (cvq->event_cb.callback)
> +			cvq->event_cb.callback(cvq->event_cb.private);


Let's use the vringh helper instead (it can deal with e.g event index 
stuffs):

if (vringh_need_notify_iotlb(cvq))
     vringh_notify(cvq);


> +
> +		local_bh_enable();
> +	}
> +out:
> +	kfree(wqent);
> +}
> +
>   static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> +	struct mlx5_vdpa_virtqueue *mvq;
> +	struct mlx5_ctrl_wq_ent *wqent;
> +
> +	if (unlikely(is_ctrl_vq_idx(mvdev, idx))) {
> +		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> +		if (!wqent)
> +			return;
> +
> +		wqent->mvdev = mvdev;
> +		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> +		queue_work(mvdev->wq, &wqent->work);
> +		return;
> +	}
>   
> +	mvq = &ndev->vqs[idx];
>   	if (unlikely(!mvq->ready))
>   		return;
>   
> @@ -1363,8 +1515,16 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> +	struct mlx5_vdpa_virtqueue *mvq;
>   
> +	if (is_ctrl_vq_idx(mvdev, idx)) {
> +		mvdev->cvq.desc_addr = desc_area;
> +		mvdev->cvq.device_addr = device_area;
> +		mvdev->cvq.driver_addr = driver_area;
> +		return 0;
> +	}
> +
> +	mvq = &ndev->vqs[idx];
>   	mvq->desc_addr = desc_area;
>   	mvq->device_addr = device_area;
>   	mvq->driver_addr = driver_area;
> @@ -1377,6 +1537,9 @@ static void mlx5_vdpa_set_vq_num(struct vdpa_device *vdev, u16 idx, u32 num)
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>   	struct mlx5_vdpa_virtqueue *mvq;
>   
> +	if (is_ctrl_vq_idx(mvdev, idx))
> +		return;
> +
>   	mvq = &ndev->vqs[idx];
>   	mvq->num_ent = num;
>   }
> @@ -1385,17 +1548,50 @@ static void mlx5_vdpa_set_vq_cb(struct vdpa_device *vdev, u16 idx, struct vdpa_c
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *vq = &ndev->vqs[idx];
>   
> -	vq->event_cb = *cb;
> +	if (is_ctrl_vq_idx(mvdev, idx)) {
> +		mvdev->cvq.event_cb = *cb;
> +		return;
> +	}
> +
> +	ndev->vqs[idx].event_cb = *cb;


I wonder whether we can simply treat cvq as a normal vq here and just 
use the ndev->vqs[idx].event_cb.


> +}
> +
> +static void mlx5_cvq_notify(struct vringh *vring)
> +{
> +	struct mlx5_control_vq *cvq = container_of(vring, struct mlx5_control_vq, vring);
> +
> +	if (!cvq->event_cb.callback)
> +		return;
> +
> +	cvq->event_cb.callback(cvq->event_cb.private);
> +}
> +
> +static void set_cvq_ready(struct mlx5_vdpa_dev *mvdev, bool ready)
> +{
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> +
> +	WARN_ON(cvq->ready && ready);
> +	WARN_ON(!cvq->ready && !ready);


It looks to me this is userspace trigger-able. E.g a buggy userspace 
driver can cause this. Then it's better to not warn here.


> +	cvq->ready = ready;
> +	if (!ready)
> +		return;
> +
> +	cvq->vring.notify = mlx5_cvq_notify;
>   }
>   
>   static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready)
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> +	struct mlx5_vdpa_virtqueue *mvq;
>   
> +	if (is_ctrl_vq_idx(mvdev, idx)) {
> +		set_cvq_ready(mvdev, ready);
> +		return;
> +	}
> +
> +	mvq = &ndev->vqs[idx];
>   	if (!ready)
>   		suspend_vq(ndev, mvq);
>   
> @@ -1406,9 +1602,11 @@ static bool mlx5_vdpa_get_vq_ready(struct vdpa_device *vdev, u16 idx)
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>   
> -	return mvq->ready;
> +	if (is_ctrl_vq_idx(mvdev, idx))
> +		return mvdev->cvq.ready;
> +
> +	return ndev->vqs[idx].ready;
>   }
>   
>   static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
> @@ -1416,8 +1614,14 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> +	struct mlx5_vdpa_virtqueue *mvq;
> +
> +	if (is_ctrl_vq_idx(mvdev, idx)) {
> +		mvdev->cvq.vring.last_avail_idx = state->split.avail_index;


Interesting, consider vringh can only support split ring. I wonder we 
need to fail the feature negotiation when both cvq and packed is negotiated.


> +		return 0;
> +	}
>   
> +	mvq = &ndev->vqs[idx];
>   	if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY) {
>   		mlx5_vdpa_warn(mvdev, "can't modify available index\n");
>   		return -EINVAL;
> @@ -1432,10 +1636,16 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>   	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> +	struct mlx5_vdpa_virtqueue *mvq;
>   	struct mlx5_virtq_attr attr;
>   	int err;
>   
> +	if (is_ctrl_vq_idx(mvdev, idx)) {
> +		state->split.avail_index = mvdev->cvq.vring.last_avail_idx;
> +		return 0;
> +	}
> +
> +	mvq = &ndev->vqs[idx];
>   	/* If the virtq object was destroyed, use the value saved at
>   	 * the last minute of suspend_vq. This caters for userspace
>   	 * that cares about emulating the index after vq is stopped.
> @@ -1492,10 +1702,13 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
>   	u16 dev_features;
>   
>   	dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask);
> -	ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features);
> +	ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
>   	if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
>   		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
>   	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> +	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
> +	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> +
>   	print_features(mvdev, ndev->mvdev.mlx_features, false);
>   	return ndev->mvdev.mlx_features;
>   }
> @@ -1508,8 +1721,10 @@ static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
>   	return 0;
>   }
>   
> -static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
> +static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>   {
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>   	int err;
>   	int i;
>   
> @@ -1519,6 +1734,16 @@ static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
>   			goto err_vq;
>   	}
>   
> +	if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) {
> +		err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features,
> +					MLX5_CVQ_MAX_ENT, false,
> +					(struct vring_desc *)(uintptr_t)cvq->desc_addr,
> +					(struct vring_avail *)(uintptr_t)cvq->driver_addr,
> +					(struct vring_used *)(uintptr_t)cvq->device_addr);
> +		if (err)
> +			goto err_vq;
> +	}
> +
>   	return 0;
>   
>   err_vq:
> @@ -1542,18 +1767,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>   	}
>   }
>   
> -/* TODO: cross-endian support */
> -static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
> -{
> -	return virtio_legacy_is_little_endian() ||
> -		(mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
> -}
> -
> -static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
> -{
> -	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> -}
> -
>   static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> @@ -1672,8 +1885,9 @@ static void restore_channels_info(struct mlx5_vdpa_net *ndev)
>   	}
>   }
>   
> -static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *iotlb)
> +static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>   {
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>   	int err;
>   
>   	suspend_vqs(ndev);
> @@ -1691,7 +1905,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *
>   		return 0;
>   
>   	restore_channels_info(ndev);
> -	err = setup_driver(ndev);
> +	err = setup_driver(mvdev);
>   	if (err)
>   		goto err_setup;
>   
> @@ -1703,37 +1917,38 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *
>   	return err;
>   }
>   
> -static int setup_driver(struct mlx5_vdpa_net *ndev)
> +static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>   {
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>   	int err;
>   
>   	mutex_lock(&ndev->reslock);
>   	if (ndev->setup) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "setup driver called for already setup driver\n");
> +		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");


It would be better to split those tweaks into another patch. (Or it's no 
easy to infer how it is related to the control vq support).

Btw, I don't see how reset is being handled for cvq?

Thanks


>   		err = 0;
>   		goto out;
>   	}
> -	err = setup_virtqueues(ndev);
> +	err = setup_virtqueues(mvdev);
>   	if (err) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "setup_virtqueues\n");
> +		mlx5_vdpa_warn(mvdev, "setup_virtqueues\n");
>   		goto out;
>   	}
>   
>   	err = create_rqt(ndev);
>   	if (err) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "create_rqt\n");
> +		mlx5_vdpa_warn(mvdev, "create_rqt\n");
>   		goto err_rqt;
>   	}
>   
>   	err = create_tir(ndev);
>   	if (err) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "create_tir\n");
> +		mlx5_vdpa_warn(mvdev, "create_tir\n");
>   		goto err_tir;
>   	}
>   
>   	err = add_fwd_to_tir(ndev);
>   	if (err) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "add_fwd_to_tir\n");
> +		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
>   		goto err_fwd;
>   	}
>   	ndev->setup = true;
> @@ -1799,7 +2014,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>   
>   	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
>   		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> -			err = setup_driver(ndev);
> +			err = setup_driver(mvdev);
>   			if (err) {
>   				mlx5_vdpa_warn(mvdev, "failed to setup driver\n");
>   				goto err_setup;
> @@ -1849,7 +2064,6 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>   static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
>   {
>   	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> -	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>   	bool change_map;
>   	int err;
>   
> @@ -1860,7 +2074,7 @@ static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb
>   	}
>   
>   	if (change_map)
> -		return mlx5_vdpa_change_map(ndev, iotlb);
> +		return mlx5_vdpa_change_map(mvdev, iotlb);
>   
>   	return 0;
>   }
> @@ -1890,6 +2104,9 @@ static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device
>   	struct mlx5_vdpa_net *ndev;
>   	phys_addr_t addr;
>   
> +	if (is_ctrl_vq_idx(mvdev, idx))
> +		return ret;
> +
>   	/* If SF BAR size is smaller than PAGE_SIZE, do not use direct
>   	 * notification to avoid the risk of mapping pages that contain BAR of more
>   	 * than one SF
> @@ -2058,8 +2275,11 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   		err = mlx5_mpfs_add_mac(pfmdev, config->mac);
>   		if (err)
>   			goto err_mtu;
> +
> +		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>   	}
>   
> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
>   	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>   	if (err)
> @@ -2075,8 +2295,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   	if (err)
>   		goto err_mr;
>   
> +	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_ctrl_wq");
> +	if (!mvdev->wq) {
> +		err = -ENOMEM;
> +		goto err_res2;
> +	}
> +
> +	ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>   	mvdev->vdev.mdev = &mgtdev->mgtdev;
> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
> +	err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
>   	if (err)
>   		goto err_reg;
>   
> @@ -2084,6 +2311,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   	return 0;
>   
>   err_reg:
> +	destroy_workqueue(mvdev->wq);
> +err_res2:
>   	free_resources(ndev);
>   err_mr:
>   	mlx5_vdpa_destroy_mr(mvdev);
> @@ -2101,7 +2330,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
>   {
>   	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> +	struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
>   
> +	destroy_workqueue(mvdev->wq);
>   	_vdpa_unregister_device(dev);
>   	mgtdev->ndev = NULL;
>   }

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 2/3] vdpa/mlx5: Add support for control VQ and MAC setting

[Jason Wang]

在 2021/8/10 下午3:17, Eli Cohen 写道:
> On Tue, Aug 10, 2021 at 12:32:23PM +0800, Jason Wang wrote:
>> 在 2021/8/9 下午10:07, Eli Cohen 写道:
>>> Add support to handle control virtqueue configurations per virtio
>>> specification. The control virtqueue is implemented in software and no
>>> hardware offloading is involved.
>>>
>>> Control VQ configuration need task context, therefore all configurations
>>> are handled in a workqueue created for the purpose.
>>>
>>> Modifications are made to the memory registration code to allow for
>>> saving a copy of itolb to be used by the control VQ to access the vring.
>>>
>>> The max number of data virtqueus supported by the driver has been
>>> updated to 2 since multiqueue is not supported at this stage and we need
>>> to ensure consistency of VQ indices mapping to either data or control
>>> VQ.
>>>
>>> v0 --> v1:
>>> cleanup some leftover code
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/mlx5/core/mlx5_vdpa.h |  23 +++
>>>    drivers/vdpa/mlx5/core/mr.c        |  87 ++++++--
>>>    drivers/vdpa/mlx5/core/resources.c |  31 +++
>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c  | 307 +++++++++++++++++++++++++----
>>>    4 files changed, 391 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> index 8d0a6f2cb3f0..71bb29fcf4ca 100644
>>> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> @@ -5,6 +5,7 @@
>>>    #define __MLX5_VDPA_H__
>>>    #include <linux/etherdevice.h>
>>> +#include <linux/vringh.h>
>>>    #include <linux/vdpa.h>
>>>    #include <linux/mlx5/driver.h>
>>> @@ -47,6 +48,26 @@ struct mlx5_vdpa_resources {
>>>    	bool valid;
>>>    };
>>> +struct mlx5_control_vq {
>>> +	struct vhost_iotlb *iotlb;
>>> +	/* spinlock to synchronize iommu table */
>>> +	spinlock_t iommu_lock;
>>> +	struct vringh vring;
>>> +	bool ready;
>>> +	u64 desc_addr;
>>> +	u64 device_addr;
>>> +	u64 driver_addr;
>>> +	struct vdpa_callback event_cb;
>>> +	struct vringh_kiov riov;
>>> +	struct vringh_kiov wiov;
>>> +	unsigned short head;
>>> +};
>>> +
>>> +struct mlx5_ctrl_wq_ent {
>>> +	struct work_struct work;
>>> +	struct mlx5_vdpa_dev *mvdev;
>>> +};
>>> +
>>>    struct mlx5_vdpa_dev {
>>>    	struct vdpa_device vdev;
>>>    	struct mlx5_core_dev *mdev;
>>> @@ -59,6 +80,8 @@ struct mlx5_vdpa_dev {
>>>    	u32 generation;
>>>    	struct mlx5_vdpa_mr mr;
>>> +	struct mlx5_control_vq cvq;
>>> +	struct workqueue_struct *wq;
>>>    };
>>>    int mlx5_vdpa_alloc_pd(struct mlx5_vdpa_dev *dev, u32 *pdn, u16 uid);
>>> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
>>> index dcee6039e966..46a657ebb1df 100644
>>> --- a/drivers/vdpa/mlx5/core/mr.c
>>> +++ b/drivers/vdpa/mlx5/core/mr.c
>>> @@ -1,6 +1,7 @@
>>>    // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>>    /* Copyright (c) 2020 Mellanox Technologies Ltd. */
>>> +#include <linux/vhost_types.h>
>>>    #include <linux/vdpa.h>
>>>    #include <linux/gcd.h>
>>>    #include <linux/string.h>
>>> @@ -451,33 +452,38 @@ static void destroy_dma_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>>>    	mlx5_vdpa_destroy_mkey(mvdev, &mr->mkey);
>>>    }
>>> -static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +static int dup_iotlb(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *src)
>>>    {
>>> -	struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> +	struct vhost_iotlb_map *map;
>>> +	u64 start = 0ULL, last = 0ULL - 1;
>>>    	int err;
>>> -	if (mr->initialized)
>>> -		return 0;
>>> -
>>> -	if (iotlb)
>>> -		err = create_user_mr(mvdev, iotlb);
>>> -	else
>>> -		err = create_dma_mr(mvdev, mr);
>>> -
>>> -	if (!err)
>>> -		mr->initialized = true;
>>> +	if (!src) {
>>> +		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last, start, VHOST_ACCESS_RW);
>>> +		return err;
>>> +	}
>>> -	return err;
>>> +	for (map = vhost_iotlb_itree_first(src, start, last); map;
>>> +		map = vhost_iotlb_itree_next(map, start, last)) {
>>> +		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start, map->last,
>>> +					    map->addr, map->perm);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +	return 0;
>>>    }
>>> -int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +static void prune_iotlb(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>> -	int err;
>>> +	struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> +	u64 start = 0ULL, last = 0ULL - 1;
>>> -	mutex_lock(&mvdev->mr.mkey_mtx);
>>> -	err = _mlx5_vdpa_create_mr(mvdev, iotlb);
>>> -	mutex_unlock(&mvdev->mr.mkey_mtx);
>>> -	return err;
>>> +	if (!mr->user_mr) {
>>> +		vhost_iotlb_del_range(mvdev->cvq.iotlb, start, last);
>>> +		return;
>>> +	}
>>> +
>>> +	vhost_iotlb_del_range(mvdev->cvq.iotlb, start, last);
>>
>> It looks to me we don't need check of if (!mr->user_mr) here.
> Right, will fix.
>
>>
>>>    }
>>>    static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr)
>>> @@ -501,6 +507,7 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>>>    	if (!mr->initialized)
>>>    		goto out;
>>> +	prune_iotlb(mvdev);
>>>    	if (mr->user_mr)
>>>    		destroy_user_mr(mvdev, mr);
>>>    	else
>>> @@ -512,6 +519,48 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>>>    	mutex_unlock(&mr->mkey_mtx);
>>>    }
>>> +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +{
>>> +	struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>> +	int err;
>>> +
>>> +	if (mr->initialized)
>>> +		return 0;
>>> +
>>> +	if (iotlb)
>>> +		err = create_user_mr(mvdev, iotlb);
>>> +	else
>>> +		err = create_dma_mr(mvdev, mr);
>>> +
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = dup_iotlb(mvdev, iotlb);
>>> +	if (err)
>>> +		goto out_err;
>>> +
>>> +	mr->initialized = true;
>>> +	return 0;
>>> +
>>> +out_err:
>>> +	if (iotlb)
>>> +		destroy_user_mr(mvdev, mr);
>>> +	else
>>> +		destroy_dma_mr(mvdev, mr);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>> +{
>>> +	int err;
>>> +
>>> +	mutex_lock(&mvdev->mr.mkey_mtx);
>>> +	err = _mlx5_vdpa_create_mr(mvdev, iotlb);
>>> +	mutex_unlock(&mvdev->mr.mkey_mtx);
>>> +	return err;
>>> +}
>>> +
>>>    static bool map_empty(struct vhost_iotlb *iotlb)
>>>    {
>>>    	return !vhost_iotlb_itree_first(iotlb, 0, U64_MAX);
>>> diff --git a/drivers/vdpa/mlx5/core/resources.c b/drivers/vdpa/mlx5/core/resources.c
>>> index d4606213f88a..d24ae1a85159 100644
>>> --- a/drivers/vdpa/mlx5/core/resources.c
>>> +++ b/drivers/vdpa/mlx5/core/resources.c
>>> @@ -1,6 +1,7 @@
>>>    // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>>    /* Copyright (c) 2020 Mellanox Technologies Ltd. */
>>> +#include <linux/iova.h>
>>>    #include <linux/mlx5/driver.h>
>>>    #include "mlx5_vdpa.h"
>>> @@ -221,6 +222,28 @@ int mlx5_vdpa_destroy_mkey(struct mlx5_vdpa_dev *mvdev, struct mlx5_core_mkey *m
>>>    	return mlx5_cmd_exec_in(mvdev->mdev, destroy_mkey, in);
>>>    }
>>> +static int init_ctrl_vq(struct mlx5_vdpa_dev *mvdev)
>>> +{
>>> +	int err;
>>> +
>>> +	mvdev->cvq.iotlb = vhost_iotlb_alloc(0, 0);
>>> +	if (!mvdev->cvq.iotlb)
>>> +		return -ENOMEM;
>>> +
>>> +	vringh_set_iotlb(&mvdev->cvq.vring, mvdev->cvq.iotlb, &mvdev->cvq.iommu_lock);
>>> +	err = iova_cache_get();
>>> +	if (err)
>>> +		vhost_iotlb_free(mvdev->cvq.iotlb);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void cleanup_ctrl_vq(struct mlx5_vdpa_dev *mvdev)
>>> +{
>>> +	iova_cache_put();
>>> +	vhost_iotlb_free(mvdev->cvq.iotlb);
>>> +}
>>> +
>>>    int mlx5_vdpa_alloc_resources(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>>    	u64 offset = MLX5_CAP64_DEV_VDPA_EMULATION(mvdev->mdev, doorbell_bar_offset);
>>> @@ -260,10 +283,17 @@ int mlx5_vdpa_alloc_resources(struct mlx5_vdpa_dev *mvdev)
>>>    		err = -ENOMEM;
>>>    		goto err_key;
>>>    	}
>>> +
>>> +	err = init_ctrl_vq(mvdev);
>>> +	if (err)
>>> +		goto err_ctrl;
>>> +
>>>    	res->valid = true;
>>>    	return 0;
>>> +err_ctrl:
>>> +	iounmap(res->kick_addr);
>>>    err_key:
>>>    	dealloc_pd(mvdev, res->pdn, res->uid);
>>>    err_pd:
>>> @@ -282,6 +312,7 @@ void mlx5_vdpa_free_resources(struct mlx5_vdpa_dev *mvdev)
>>>    	if (!res->valid)
>>>    		return;
>>> +	cleanup_ctrl_vq(mvdev);
>>>    	iounmap(res->kick_addr);
>>>    	res->kick_addr = NULL;
>>>    	dealloc_pd(mvdev, res->pdn, res->uid);
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 2a31467f7ac5..46448b079aca 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -133,7 +133,7 @@ struct mlx5_vdpa_virtqueue {
>>>    /* We will remove this limitation once mlx5_vdpa_alloc_resources()
>>>     * provides for driver space allocation
>>>     */
>>> -#define MLX5_MAX_SUPPORTED_VQS 16
>>> +#define MLX5_MAX_SUPPORTED_VQS 2
>>>    struct mlx5_vdpa_net {
>>>    	struct mlx5_vdpa_dev mvdev;
>>> @@ -151,15 +151,18 @@ struct mlx5_vdpa_net {
>>>    	struct mlx5_flow_handle *rx_rule;
>>>    	bool setup;
>>>    	u16 mtu;
>>> +	u32 cur_num_vqs;
>>>    };
>>>    static void free_resources(struct mlx5_vdpa_net *ndev);
>>>    static void init_mvqs(struct mlx5_vdpa_net *ndev);
>>> -static int setup_driver(struct mlx5_vdpa_net *ndev);
>>> +static int setup_driver(struct mlx5_vdpa_dev *mvdev);
>>>    static void teardown_driver(struct mlx5_vdpa_net *ndev);
>>>    static bool mlx5_vdpa_debug;
>>> +#define MLX5_CVQ_MAX_ENT 16
>>> +
>>>    #define MLX5_LOG_VIO_FLAG(_feature)                                                                \
>>>    	do {                                                                                       \
>>>    		if (features & BIT_ULL(_feature))                                                  \
>>> @@ -172,11 +175,33 @@ static bool mlx5_vdpa_debug;
>>>    			mlx5_vdpa_info(mvdev, "%s\n", #_status);                                   \
>>>    	} while (0)
>>> +/* TODO: cross-endian support */
>>> +static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>>> +{
>>> +	return virtio_legacy_is_little_endian() ||
>>> +		(mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
>>> +}
>>> +
>>> +static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>>> +{
>>> +	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>>> +}
>>> +
>>>    static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>>>    {
>>>    	return max_vqs / 2;
>>>    }
>>> +static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>> +{
>>> +	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
>>> +}
>>> +
>>> +static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
>>> +{
>>> +	return idx == ctrl_vq_idx(mvdev);
>>> +}
>>> +
>>>    static void print_status(struct mlx5_vdpa_dev *mvdev, u8 status, bool set)
>>>    {
>>>    	if (status & ~VALID_STATUS_MASK)
>>> @@ -1346,12 +1371,139 @@ static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>>>    	ndev->rx_rule = NULL;
>>>    }
>>> +static int update_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>>> +{
>>> +	remove_fwd_to_tir(ndev);
>>> +	return add_fwd_to_tir(ndev);
>>> +}
>>> +
>>> +virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>> +{
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>>> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>>> +	struct mlx5_core_dev *pfmdev;
>>> +	size_t read;
>>> +	u8 mac[6];
>>> +
>>> +	pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
>>> +	switch (cmd) {
>>> +	case VIRTIO_NET_CTRL_MAC_ADDR_SET:
>>> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)mac, ETH_ALEN);
>>> +		if (read != ETH_ALEN)
>>> +			break;
>>> +
>>> +		if (!memcmp(ndev->config.mac, mac, 6)) {
>>> +			status = VIRTIO_NET_OK;
>>> +			break;
>>> +		}
>>> +
>>> +		if (!is_zero_ether_addr(ndev->config.mac)) {
>>> +			if (mlx5_mpfs_del_mac(pfmdev, ndev->config.mac)) {
>>> +				mlx5_vdpa_warn(mvdev, "failed to delete old MAC %pM from MPFS table\n",
>>> +					       ndev->config.mac);
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		if (mlx5_mpfs_add_mac(pfmdev, mac)) {
>>> +			mlx5_vdpa_warn(mvdev, "failed to insert new MAC %pM into MPFS table\n",
>>> +				       mac);
>>> +			break;
>>> +		}
>>> +
>>> +		memcpy(ndev->config.mac, mac, 6);
>>
>> Let's use ETH_ALEN.
> Will do, here and in the local variable at the begining of the function.
>
>>
>>> +		if (!update_fwd_to_tir(ndev))
>>> +			status = VIRTIO_NET_OK;
>>
>> I think it's better to to update config after the succeed of
>> update_fwd_to_tir().
> add_fwd_to_tir() relies on whatever value the config has. I will modify
> the code to pass the mac as an argument to update_fwd_to_tir().


That should work.


>
>>
>>> +		break;
>>> +
>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +static void mlx5_cvq_kick_handler(struct work_struct *work)
>>> +{
>>> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>>> +	struct virtio_net_ctrl_hdr ctrl;
>>> +	struct mlx5_ctrl_wq_ent *wqent;
>>> +	struct mlx5_vdpa_dev *mvdev;
>>> +	struct mlx5_control_vq *cvq;
>>> +	struct mlx5_vdpa_net *ndev;
>>> +	size_t read, write;
>>> +	int err;
>>> +
>>> +	wqent = container_of(work, struct mlx5_ctrl_wq_ent, work);
>>> +	mvdev = wqent->mvdev;
>>> +	ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +	cvq = &mvdev->cvq;
>>> +	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
>>> +		goto out;
>>> +
>>> +	if (!cvq->ready)
>>> +		goto out;
>>> +
>>> +	while (true) {
>>> +		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
>>> +					   GFP_ATOMIC);
>>> +		if (err <= 0)
>>> +			break;
>>> +
>>> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, &ctrl, sizeof(ctrl));
>>> +		if (read != sizeof(ctrl))
>>> +			break;
>>> +
>>> +		switch (ctrl.class) {
>>> +		case VIRTIO_NET_CTRL_MAC:
>>> +			status = handle_ctrl_mac(mvdev, ctrl.cmd);
>>> +			break;
>>> +
>>> +		default:
>>> +			break;
>>> +		}
>>> +
>>> +		/* Make sure data is written before advancing index */
>>> +		smp_wmb();
>>> +
>>> +		write = vringh_iov_push_iotlb(&cvq->vring, &cvq->wiov, &status, sizeof(status));
>>> +		vringh_complete_iotlb(&cvq->vring, cvq->head, write);
>>> +		vringh_kiov_cleanup(&cvq->riov);
>>> +		vringh_kiov_cleanup(&cvq->wiov);
>>> +
>>> +		/* Make sure used is visible before rasing the interrupt. */
>>> +		smp_wmb();
>>> +
>>> +		local_bh_disable();
>>> +		if (cvq->event_cb.callback)
>>> +			cvq->event_cb.callback(cvq->event_cb.private);
>>
>> Let's use the vringh helper instead (it can deal with e.g event index
>> stuffs):
>>
>> if (vringh_need_notify_iotlb(cvq))
>>      vringh_notify(cvq);
>>
> Will look at this.
>
>>> +
>>> +		local_bh_enable();
>>> +	}
>>> +out:
>>> +	kfree(wqent);
>>> +}
>>> +
>>>    static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> +	struct mlx5_vdpa_virtqueue *mvq;
>>> +	struct mlx5_ctrl_wq_ent *wqent;
>>> +
>>> +	if (unlikely(is_ctrl_vq_idx(mvdev, idx))) {
>>> +		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
>>> +		if (!wqent)
>>> +			return;
>>> +
>>> +		wqent->mvdev = mvdev;
>>> +		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
>>> +		queue_work(mvdev->wq, &wqent->work);
>>> +		return;
>>> +	}
>>> +	mvq = &ndev->vqs[idx];
>>>    	if (unlikely(!mvq->ready))
>>>    		return;
>>> @@ -1363,8 +1515,16 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> +	struct mlx5_vdpa_virtqueue *mvq;
>>> +	if (is_ctrl_vq_idx(mvdev, idx)) {
>>> +		mvdev->cvq.desc_addr = desc_area;
>>> +		mvdev->cvq.device_addr = device_area;
>>> +		mvdev->cvq.driver_addr = driver_area;
>>> +		return 0;
>>> +	}
>>> +
>>> +	mvq = &ndev->vqs[idx];
>>>    	mvq->desc_addr = desc_area;
>>>    	mvq->device_addr = device_area;
>>>    	mvq->driver_addr = driver_area;
>>> @@ -1377,6 +1537,9 @@ static void mlx5_vdpa_set_vq_num(struct vdpa_device *vdev, u16 idx, u32 num)
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>    	struct mlx5_vdpa_virtqueue *mvq;
>>> +	if (is_ctrl_vq_idx(mvdev, idx))
>>> +		return;
>>> +
>>>    	mvq = &ndev->vqs[idx];
>>>    	mvq->num_ent = num;
>>>    }
>>> @@ -1385,17 +1548,50 @@ static void mlx5_vdpa_set_vq_cb(struct vdpa_device *vdev, u16 idx, struct vdpa_c
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *vq = &ndev->vqs[idx];
>>> -	vq->event_cb = *cb;
>>> +	if (is_ctrl_vq_idx(mvdev, idx)) {
>>> +		mvdev->cvq.event_cb = *cb;
>>> +		return;
>>> +	}
>>> +
>>> +	ndev->vqs[idx].event_cb = *cb;
>>
>> I wonder whether we can simply treat cvq as a normal vq here and just use
>> the ndev->vqs[idx].event_cb.
>>
> Not really. CVQ is a software vq while the data VQs are hardware VQs so
> we have different data structs to hold their contexts.


Right, rethink about this, actually it's even better since the index of 
cvq is not fixed.


>
>>> +}
>>> +
>>> +static void mlx5_cvq_notify(struct vringh *vring)
>>> +{
>>> +	struct mlx5_control_vq *cvq = container_of(vring, struct mlx5_control_vq, vring);
>>> +
>>> +	if (!cvq->event_cb.callback)
>>> +		return;
>>> +
>>> +	cvq->event_cb.callback(cvq->event_cb.private);
>>> +}
>>> +
>>> +static void set_cvq_ready(struct mlx5_vdpa_dev *mvdev, bool ready)
>>> +{
>>> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>>> +
>>> +	WARN_ON(cvq->ready && ready);
>>> +	WARN_ON(!cvq->ready && !ready);
>>
>> It looks to me this is userspace trigger-able. E.g a buggy userspace driver
>> can cause this. Then it's better to not warn here.
>>
> Agree, will remove.
>>> +	cvq->ready = ready;
>>> +	if (!ready)
>>> +		return;
>>> +
>>> +	cvq->vring.notify = mlx5_cvq_notify;
>>>    }
>>>    static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready)
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> +	struct mlx5_vdpa_virtqueue *mvq;
>>> +	if (is_ctrl_vq_idx(mvdev, idx)) {
>>> +		set_cvq_ready(mvdev, ready);
>>> +		return;
>>> +	}
>>> +
>>> +	mvq = &ndev->vqs[idx];
>>>    	if (!ready)
>>>    		suspend_vq(ndev, mvq);
>>> @@ -1406,9 +1602,11 @@ static bool mlx5_vdpa_get_vq_ready(struct vdpa_device *vdev, u16 idx)
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> -	return mvq->ready;
>>> +	if (is_ctrl_vq_idx(mvdev, idx))
>>> +		return mvdev->cvq.ready;
>>> +
>>> +	return ndev->vqs[idx].ready;
>>>    }
>>>    static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>>> @@ -1416,8 +1614,14 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> +	struct mlx5_vdpa_virtqueue *mvq;
>>> +
>>> +	if (is_ctrl_vq_idx(mvdev, idx)) {
>>> +		mvdev->cvq.vring.last_avail_idx = state->split.avail_index;
>>
>> Interesting, consider vringh can only support split ring. I wonder we need
>> to fail the feature negotiation when both cvq and packed is negotiated.
>>
> Why do that? In the case of mlx5 we can live with it. Although I
> currently do not advertise split virtqueue support, the hardware could
> support in the future packed virtqueue and I could live with packed
> data VQs and split for control. So it should be up to the vdpa driver to
> take care of that.


I think you are using vringh to decode commands in the software control 
virtqueue now. Since vringh doesn't support packed ring, so when guest 
is using packed ring for control virtqueue, we breaks.

(I think mlx5 support packed ring, but if it doesn't the code is fine).


>
>>> +		return 0;
>>> +	}
>>> +	mvq = &ndev->vqs[idx];
>>>    	if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY) {
>>>    		mlx5_vdpa_warn(mvdev, "can't modify available index\n");
>>>    		return -EINVAL;
>>> @@ -1432,10 +1636,16 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>    	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> -	struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
>>> +	struct mlx5_vdpa_virtqueue *mvq;
>>>    	struct mlx5_virtq_attr attr;
>>>    	int err;
>>> +	if (is_ctrl_vq_idx(mvdev, idx)) {
>>> +		state->split.avail_index = mvdev->cvq.vring.last_avail_idx;
>>> +		return 0;
>>> +	}
>>> +
>>> +	mvq = &ndev->vqs[idx];
>>>    	/* If the virtq object was destroyed, use the value saved at
>>>    	 * the last minute of suspend_vq. This caters for userspace
>>>    	 * that cares about emulating the index after vq is stopped.
>>> @@ -1492,10 +1702,13 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
>>>    	u16 dev_features;
>>>    	dev_features = MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, device_features_bits_mask);
>>> -	ndev->mvdev.mlx_features = mlx_to_vritio_features(dev_features);
>>> +	ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
>>>    	if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
>>>    		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
>>>    	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
>>> +	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
>>> +	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
>>> +
>>>    	print_features(mvdev, ndev->mvdev.mlx_features, false);
>>>    	return ndev->mvdev.mlx_features;
>>>    }
>>> @@ -1508,8 +1721,10 @@ static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
>>>    	return 0;
>>>    }
>>> -static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
>>> +static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>>>    	int err;
>>>    	int i;
>>> @@ -1519,6 +1734,16 @@ static int setup_virtqueues(struct mlx5_vdpa_net *ndev)
>>>    			goto err_vq;
>>>    	}
>>> +	if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) {
>>> +		err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features,
>>> +					MLX5_CVQ_MAX_ENT, false,
>>> +					(struct vring_desc *)(uintptr_t)cvq->desc_addr,
>>> +					(struct vring_avail *)(uintptr_t)cvq->driver_addr,
>>> +					(struct vring_used *)(uintptr_t)cvq->device_addr);
>>> +		if (err)
>>> +			goto err_vq;
>>> +	}
>>> +
>>>    	return 0;
>>>    err_vq:
>>> @@ -1542,18 +1767,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>>>    	}
>>>    }
>>> -/* TODO: cross-endian support */
>>> -static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>>> -{
>>> -	return virtio_legacy_is_little_endian() ||
>>> -		(mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
>>> -}
>>> -
>>> -static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>>> -{
>>> -	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>>> -}
>>> -
>>>    static int mlx5_vdpa_set_features(struct vdpa_device *vdev, u64 features)
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> @@ -1672,8 +1885,9 @@ static void restore_channels_info(struct mlx5_vdpa_net *ndev)
>>>    	}
>>>    }
>>> -static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *iotlb)
>>> +static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb)
>>>    {
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>    	int err;
>>>    	suspend_vqs(ndev);
>>> @@ -1691,7 +1905,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *
>>>    		return 0;
>>>    	restore_channels_info(ndev);
>>> -	err = setup_driver(ndev);
>>> +	err = setup_driver(mvdev);
>>>    	if (err)
>>>    		goto err_setup;
>>> @@ -1703,37 +1917,38 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb *
>>>    	return err;
>>>    }
>>> -static int setup_driver(struct mlx5_vdpa_net *ndev)
>>> +static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>    	int err;
>>>    	mutex_lock(&ndev->reslock);
>>>    	if (ndev->setup) {
>>> -		mlx5_vdpa_warn(&ndev->mvdev, "setup driver called for already setup driver\n");
>>> +		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
>>
>> It would be better to split those tweaks into another patch. (Or it's no
>> easy to infer how it is related to the control vq support).
> OK, the point is that the CVQ belongs to mlx5_vdpa_dev so I hade to pass
> it and make these changes but will do it.
>
>> Btw, I don't see how reset is being handled for cvq?
> I guess you mean the oposite operation to vringh_init_iotlb() issued in
> setup_virtqueues(). Is there such cleanup call?


I think it should be something similar to what vdpa_sim did (recall 
vringh_init_iotlb()) to reset the vringh internal states:

         vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
                           VDPASIM_QUEUE_MAX, false, NULL, NULL, NULL);

Thanks


>> Thanks
>>
>>
>>>    		err = 0;
>>>    		goto out;
>>>    	}
>>> -	err = setup_virtqueues(ndev);
>>> +	err = setup_virtqueues(mvdev);
>>>    	if (err) {
>>> -		mlx5_vdpa_warn(&ndev->mvdev, "setup_virtqueues\n");
>>> +		mlx5_vdpa_warn(mvdev, "setup_virtqueues\n");
>>>    		goto out;
>>>    	}
>>>    	err = create_rqt(ndev);
>>>    	if (err) {
>>> -		mlx5_vdpa_warn(&ndev->mvdev, "create_rqt\n");
>>> +		mlx5_vdpa_warn(mvdev, "create_rqt\n");
>>>    		goto err_rqt;
>>>    	}
>>>    	err = create_tir(ndev);
>>>    	if (err) {
>>> -		mlx5_vdpa_warn(&ndev->mvdev, "create_tir\n");
>>> +		mlx5_vdpa_warn(mvdev, "create_tir\n");
>>>    		goto err_tir;
>>>    	}
>>>    	err = add_fwd_to_tir(ndev);
>>>    	if (err) {
>>> -		mlx5_vdpa_warn(&ndev->mvdev, "add_fwd_to_tir\n");
>>> +		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
>>>    		goto err_fwd;
>>>    	}
>>>    	ndev->setup = true;
>>> @@ -1799,7 +2014,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>>    	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>    		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>> -			err = setup_driver(ndev);
>>> +			err = setup_driver(mvdev);
>>>    			if (err) {
>>>    				mlx5_vdpa_warn(mvdev, "failed to setup driver\n");
>>>    				goto err_setup;
>>> @@ -1849,7 +2064,6 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>>>    static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
>>>    {
>>>    	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> -	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>    	bool change_map;
>>>    	int err;
>>> @@ -1860,7 +2074,7 @@ static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb
>>>    	}
>>>    	if (change_map)
>>> -		return mlx5_vdpa_change_map(ndev, iotlb);
>>> +		return mlx5_vdpa_change_map(mvdev, iotlb);
>>>    	return 0;
>>>    }
>>> @@ -1890,6 +2104,9 @@ static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device
>>>    	struct mlx5_vdpa_net *ndev;
>>>    	phys_addr_t addr;
>>> +	if (is_ctrl_vq_idx(mvdev, idx))
>>> +		return ret;
>>> +
>>>    	/* If SF BAR size is smaller than PAGE_SIZE, do not use direct
>>>    	 * notification to avoid the risk of mapping pages that contain BAR of more
>>>    	 * than one SF
>>> @@ -2058,8 +2275,11 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>>    		err = mlx5_mpfs_add_mac(pfmdev, config->mac);
>>>    		if (err)
>>>    			goto err_mtu;
>>> +
>>> +		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>>>    	}
>>> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
>>>    	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>>>    	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>    	if (err)
>>> @@ -2075,8 +2295,15 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>>    	if (err)
>>>    		goto err_mr;
>>> +	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_ctrl_wq");
>>> +	if (!mvdev->wq) {
>>> +		err = -ENOMEM;
>>> +		goto err_res2;
>>> +	}
>>> +
>>> +	ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>>>    	mvdev->vdev.mdev = &mgtdev->mgtdev;
>>> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs));
>>> +	err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
>>>    	if (err)
>>>    		goto err_reg;
>>> @@ -2084,6 +2311,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>>    	return 0;
>>>    err_reg:
>>> +	destroy_workqueue(mvdev->wq);
>>> +err_res2:
>>>    	free_resources(ndev);
>>>    err_mr:
>>>    	mlx5_vdpa_destroy_mr(mvdev);
>>> @@ -2101,7 +2330,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>>    static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
>>>    {
>>>    	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
>>> +	struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
>>> +	destroy_workqueue(mvdev->wq);
>>>    	_vdpa_unregister_device(dev);
>>>    	mgtdev->ndev = NULL;
>>>    }

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/9 下午10:08, Eli Cohen 写道:
> Multiqueue support requires additional virtio_net_q objects to be added
> or removed per the configured number of queue pairs. In addition the RQ
> tables needs to be modified to match the number of configured receive
> queues so the packets are dispatched to the right virtqueue according to
> the hash result.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |   1 +
>   drivers/vdpa/mlx5/core/resources.c |  10 ++
>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 162 +++++++++++++++++++++++++----
>   3 files changed, 154 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index 71bb29fcf4ca..41718dad6e4a 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -90,6 +90,7 @@ int mlx5_vdpa_get_null_mkey(struct mlx5_vdpa_dev *dev, u32 *null_mkey);
>   int mlx5_vdpa_create_tis(struct mlx5_vdpa_dev *mvdev, void *in, u32 *tisn);
>   void mlx5_vdpa_destroy_tis(struct mlx5_vdpa_dev *mvdev, u32 tisn);
>   int mlx5_vdpa_create_rqt(struct mlx5_vdpa_dev *mvdev, void *in, int inlen, u32 *rqtn);
> +int mlx5_vdpa_modify_rqt(struct mlx5_vdpa_dev *mvdev, void *in, int inlen, u32 rqtn);
>   void mlx5_vdpa_destroy_rqt(struct mlx5_vdpa_dev *mvdev, u32 rqtn);
>   int mlx5_vdpa_create_tir(struct mlx5_vdpa_dev *mvdev, void *in, u32 *tirn);
>   void mlx5_vdpa_destroy_tir(struct mlx5_vdpa_dev *mvdev, u32 tirn);
> diff --git a/drivers/vdpa/mlx5/core/resources.c b/drivers/vdpa/mlx5/core/resources.c
> index d24ae1a85159..bbdcf9a01a6d 100644
> --- a/drivers/vdpa/mlx5/core/resources.c
> +++ b/drivers/vdpa/mlx5/core/resources.c
> @@ -129,6 +129,16 @@ int mlx5_vdpa_create_rqt(struct mlx5_vdpa_dev *mvdev, void *in, int inlen, u32 *
>   	return err;
>   }
>   
> +int mlx5_vdpa_modify_rqt(struct mlx5_vdpa_dev *mvdev, void *in, int inlen, u32 rqtn)
> +{
> +	u32 out[MLX5_ST_SZ_DW(create_rqt_out)] = {};
> +
> +	MLX5_SET(modify_rqt_in, in, uid, mvdev->res.uid);
> +	MLX5_SET(modify_rqt_in, in, rqtn, rqtn);
> +	MLX5_SET(modify_rqt_in, in, opcode, MLX5_CMD_OP_MODIFY_RQT);
> +	return mlx5_cmd_exec(mvdev->mdev, in, inlen, out, sizeof(out));
> +}
> +
>   void mlx5_vdpa_destroy_rqt(struct mlx5_vdpa_dev *mvdev, u32 rqtn)
>   {
>   	u32 in[MLX5_ST_SZ_DW(destroy_rqt_in)] = {};
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 46448b079aca..0b5ea0386a41 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -133,7 +133,7 @@ struct mlx5_vdpa_virtqueue {
>   /* We will remove this limitation once mlx5_vdpa_alloc_resources()
>    * provides for driver space allocation
>    */
> -#define MLX5_MAX_SUPPORTED_VQS 2
> +#define MLX5_MAX_SUPPORTED_VQS 16
>   
>   struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
> @@ -182,6 +182,11 @@ static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>   		(mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
>   }
>   
> +static u16 mlx5vdpa16_to_cpu(struct mlx5_vdpa_dev *mvdev, __virtio16 val)
> +{
> +	return __virtio16_to_cpu(mlx5_vdpa_is_little_endian(mvdev), val);
> +}
> +
>   static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>   {
>   	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> @@ -1126,10 +1131,8 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   	if (!mvq->num_ent)
>   		return 0;
>   
> -	if (mvq->initialized) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "attempt re init\n");
> -		return -EINVAL;
> -	}
> +	if (mvq->initialized)
> +		return 0;


Let's use a separated patch for this.


>   
>   	err = cq_create(ndev, idx, mvq->num_ent);
>   	if (err)
> @@ -1216,19 +1219,20 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>   
>   static int create_rqt(struct mlx5_vdpa_net *ndev)
>   {
> -	int log_max_rqt;
>   	__be32 *list;
> +	int max_rqt;
>   	void *rqtc;
>   	int inlen;
>   	void *in;
>   	int i, j;
>   	int err;
>   
> -	log_max_rqt = min_t(int, 1, MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> -	if (log_max_rqt < 1)
> +	max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
> +			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> +	if (max_rqt < 1)
>   		return -EOPNOTSUPP;
>   
> -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + (1 << log_max_rqt) * MLX5_ST_SZ_BYTES(rq_num);
> +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
>   	in = kzalloc(inlen, GFP_KERNEL);
>   	if (!in)
>   		return -ENOMEM;
> @@ -1237,10 +1241,9 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>   
>   	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> -	MLX5_SET(rqtc, rqtc, rqt_max_size, 1 << log_max_rqt);
> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, 1);
> +	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> -	for (i = 0, j = 0; j < ndev->mvdev.max_vqs; j++) {
> +	for (i = 0, j = 0; j < max_rqt; j++) {
>   		if (!ndev->vqs[j].initialized)
>   			continue;
>   
> @@ -1249,6 +1252,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   			i++;
>   		}
>   	}
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
>   
>   	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>   	kfree(in);
> @@ -1258,6 +1262,52 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   	return 0;
>   }
>   
> +#define MLX5_MODIFY_RQT_NUM_RQS ((u64)1)
> +
> +int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> +{
> +	__be32 *list;
> +	int max_rqt;
> +	void *rqtc;
> +	int inlen;
> +	void *in;
> +	int i, j;
> +	int err;
> +
> +	max_rqt = min_t(int, ndev->cur_num_vqs / 2,
> +			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> +	if (max_rqt < 1)
> +		return -EOPNOTSUPP;
> +
> +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> +	in = kzalloc(inlen, GFP_KERNEL);
> +	if (!in)
> +		return -ENOMEM;
> +
> +	MLX5_SET(modify_rqt_in, in, uid, ndev->mvdev.res.uid);
> +	MLX5_SET64(modify_rqt_in, in, bitmask, MLX5_MODIFY_RQT_NUM_RQS);
> +	rqtc = MLX5_ADDR_OF(modify_rqt_in, in, ctx);
> +	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> +
> +	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> +	for (i = 0, j = 0; j < num; j++) {
> +		if (!ndev->vqs[j].initialized)
> +			continue;
> +
> +		if (!vq_is_tx(ndev->vqs[j].index)) {
> +			list[i] = cpu_to_be32(ndev->vqs[j].virtq_id);
> +			i++;
> +		}
> +	}
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, i);
> +	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
> +	kfree(in);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>   static void destroy_rqt(struct mlx5_vdpa_net *ndev)
>   {
>   	mlx5_vdpa_destroy_rqt(&ndev->mvdev, ndev->res.rqtn);
> @@ -1424,6 +1474,77 @@ virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   	return status;
>   }
>   
> +static int change_num_qps(struct mlx5_vdpa_dev *mvdev, int newqps)
> +{
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	int cur_qps = ndev->cur_num_vqs / 2;
> +	int err;
> +	int i;
> +
> +	if (cur_qps > newqps) {
> +		err = modify_rqt(ndev, 2 * newqps);
> +		if (err)
> +			return err;
> +
> +		for (i = ndev->cur_num_vqs - 1; i >= 2 * newqps; i--)
> +			teardown_vq(ndev, &ndev->vqs[i]);
> +
> +		ndev->cur_num_vqs = 2 * newqps;
> +	} else {
> +		ndev->cur_num_vqs = 2 * newqps;
> +		for (i = cur_qps * 2; i < 2 * newqps; i++) {
> +			err = setup_vq(ndev, &ndev->vqs[i]);
> +			if (err)
> +				goto clean_added;
> +		}
> +		err = modify_rqt(ndev, 2 * newqps);
> +		if (err)
> +			goto clean_added;
> +	}
> +	return 0;
> +
> +clean_added:
> +	for (--i; i >= cur_qps; --i)
> +		teardown_vq(ndev, &ndev->vqs[i]);
> +
> +	return err;
> +}
> +
> +virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> +{
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> +	struct virtio_net_ctrl_mq mq;
> +	size_t read;
> +	u16 newqps;
> +
> +	switch (cmd) {
> +	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
> +		if (read != sizeof(mq))
> +			break;
> +
> +		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> +		if (ndev->cur_num_vqs == 2 * newqps) {
> +			status = VIRTIO_NET_OK;
> +			break;
> +		}
> +
> +		if (newqps & (newqps - 1))
> +			break;
> +
> +		if (!change_num_qps(mvdev, newqps))
> +			status = VIRTIO_NET_OK;
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return status;
> +}
> +
>   static void mlx5_cvq_kick_handler(struct work_struct *work)
>   {
>   	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> @@ -1459,6 +1580,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		case VIRTIO_NET_CTRL_MAC:
>   			status = handle_ctrl_mac(mvdev, ctrl.cmd);
>   			break;
> +		case VIRTIO_NET_CTRL_MQ:
> +			status = handle_ctrl_mq(mvdev, ctrl.cmd);
> +			break;
>   
>   		default:
>   			break;
> @@ -1708,6 +1832,7 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device *vdev)
>   	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
>   	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
>   	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> +	ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
>   
>   	print_features(mvdev, ndev->mvdev.mlx_features, false);
>   	return ndev->mvdev.mlx_features;
> @@ -1819,15 +1944,14 @@ static u8 mlx5_vdpa_get_status(struct vdpa_device *vdev)
>   static int save_channel_info(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   {
>   	struct mlx5_vq_restore_info *ri = &mvq->ri;
> -	struct mlx5_virtq_attr attr;
> +	struct mlx5_virtq_attr attr = {};
>   	int err;
>   
> -	if (!mvq->initialized)
> -		return 0;
> -
> -	err = query_virtqueue(ndev, mvq, &attr);
> -	if (err)
> -		return err;
> +	if (mvq->initialized) {
> 	
> +		if (err)
> +			return err;
> +	}


One thing need to solve for mq is that the:


> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> +{
> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> +}

We should handle the case when MQ is supported by the device but not the 
driver.

E.g in the case when we have 2 queue pairs:

When MQ is enabled, cvq is queue 4

When MQ is not enabled, cvq is queue 2

Thanks


>   
>   	ri->avail_index = attr.available_index;
>   	ri->used_index = attr.used_index;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/11 下午3:53, Eli Cohen 写道:
>> One thing need to solve for mq is that the:
>>
>>
>>> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
>>> +{
>>> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
>>> +}
>> We should handle the case when MQ is supported by the device but not the
>> driver.
>>
>> E.g in the case when we have 2 queue pairs:
>>
>> When MQ is enabled, cvq is queue 4
>>
>> When MQ is not enabled, cvq is queue 2
>>
> There's some issue with this. I get callbacks targeting specific
> virtqueues before features negotiation has been completed.
>
> Specifically, I get set_vq_cb() calls. At this point I must know the
> control vq index.


So I think we need do both:

1) At one hand, it's a bug for the userspace to use vq_index before 
feature is negotiated

(looks like a bug in my cvq series that will call SET_VRING_CALL before 
feature is negotiate, which I will look).

2) At the other hand, the driver should be able to deal with that


>
> I think the CVQ index must not depend on the negotiated features but
> rather depend of the value the device driver provides in the call to
> _vdpa_register_device().


At the virtio level, it's too late to change that and it breaks the 
backward compatibility.

But at the vDPA level, the under layer device can map virtio cvq to any 
of it's virtqueue.

E.g map cvq (index 2) to mlx5 cvq (the last).

Thanks


>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/11 下午7:04, Eli Cohen 写道:
> On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
>> 在 2021/8/11 下午3:53, Eli Cohen 写道:
>>>> One thing need to solve for mq is that the:
>>>>
>>>>
>>>>> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
>>>>> +{
>>>>> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>> +}
>>>> We should handle the case when MQ is supported by the device but not the
>>>> driver.
>>>>
>>>> E.g in the case when we have 2 queue pairs:
>>>>
>>>> When MQ is enabled, cvq is queue 4
>>>>
>>>> When MQ is not enabled, cvq is queue 2
>>>>
>>> There's some issue with this. I get callbacks targeting specific
>>> virtqueues before features negotiation has been completed.
>>>
>>> Specifically, I get set_vq_cb() calls. At this point I must know the
>>> control vq index.
>>
>> So I think we need do both:
>>
>> 1) At one hand, it's a bug for the userspace to use vq_index before feature
>> is negotiated
>>
>> (looks like a bug in my cvq series that will call SET_VRING_CALL before
>> feature is negotiate, which I will look).
>>
>> 2) At the other hand, the driver should be able to deal with that
>>
> All I can do is drop callbacks for VQs before features negotation has
> been completed.


Or just leave queue index 0, 1 work.

Since it is not expected to be changed.


>
>>> I think the CVQ index must not depend on the negotiated features but
>>> rather depend of the value the device driver provides in the call to
>>> _vdpa_register_device().
>>
>> At the virtio level, it's too late to change that and it breaks the backward
>> compatibility.
>>
>> But at the vDPA level, the under layer device can map virtio cvq to any of
>> it's virtqueue.
>>
>> E.g map cvq (index 2) to mlx5 cvq (the last).
>>
> I am not following you here. I still don't know what index is cvq.


Right, we still need to wait for the feature being negotiated in order 
to proceed.

Thanks


>
>> Thanks
>>
>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
> >
> > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
> > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > One thing need to solve for mq is that the:
> > > > > >
> > > > > >
> > > > > > > +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> > > > > > > +{
> > > > > > > +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > +}
> > > > > > We should handle the case when MQ is supported by the device but not the
> > > > > > driver.
> > > > > >
> > > > > > E.g in the case when we have 2 queue pairs:
> > > > > >
> > > > > > When MQ is enabled, cvq is queue 4
> > > > > >
> > > > > > When MQ is not enabled, cvq is queue 2
> > > > > >
> > > > > There's some issue with this. I get callbacks targeting specific
> > > > > virtqueues before features negotiation has been completed.
> > > > >
> > > > > Specifically, I get set_vq_cb() calls. At this point I must know the
> > > > > control vq index.
> > > >
> > > > So I think we need do both:
> > > >
> > > > 1) At one hand, it's a bug for the userspace to use vq_index before feature
> > > > is negotiated
> > > >
> > > > (looks like a bug in my cvq series that will call SET_VRING_CALL before
> > > > feature is negotiate, which I will look).
> > > >
> > > > 2) At the other hand, the driver should be able to deal with that
> > > >
> > > All I can do is drop callbacks for VQs before features negotation has
> > > been completed.
> >
> >
> > Or just leave queue index 0, 1 work.
> >
> > Since it is not expected to be changed.
> >
>
> Right, will do.
>
> >
> > >
> > > > > I think the CVQ index must not depend on the negotiated features but
> > > > > rather depend of the value the device driver provides in the call to
> > > > > _vdpa_register_device().
> > > >
> > > > At the virtio level, it's too late to change that and it breaks the backward
> > > > compatibility.
> > > >
> > > > But at the vDPA level, the under layer device can map virtio cvq to any of
> > > > it's virtqueue.
> > > >
> > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > >
> > > I am not following you here. I still don't know what index is cvq.
> >
> >
> > Right, we still need to wait for the feature being negotiated in order to
> > proceed.
> >
>
> So to summarise, before feature negotiation complete, I accept calls
> referring to VQs only for indices 0 and 1.
> After feature negotiation complete I know CVQ index and will accept
> indices 0 to cvq index.

I don't get this "accept indices 0 to cvq index".

Thanks

>
> > Thanks
> >
> >
> > >
> > > > Thanks
> > > >
> > > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/12 下午3:01, Eli Cohen 写道:
> On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
>> On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
>>> On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
>>>> 在 2021/8/11 下午7:04, Eli Cohen 写道:
>>>>> On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
>>>>>> 在 2021/8/11 下午3:53, Eli Cohen 写道:
>>>>>>>> One thing need to solve for mq is that the:
>>>>>>>>
>>>>>>>>
>>>>>>>>> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
>>>>>>>>> +{
>>>>>>>>> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>>>>>> +}
>>>>>>>> We should handle the case when MQ is supported by the device but not the
>>>>>>>> driver.
>>>>>>>>
>>>>>>>> E.g in the case when we have 2 queue pairs:
>>>>>>>>
>>>>>>>> When MQ is enabled, cvq is queue 4
>>>>>>>>
>>>>>>>> When MQ is not enabled, cvq is queue 2
>>>>>>>>
>>>>>>> There's some issue with this. I get callbacks targeting specific
>>>>>>> virtqueues before features negotiation has been completed.
>>>>>>>
>>>>>>> Specifically, I get set_vq_cb() calls. At this point I must know the
>>>>>>> control vq index.
>>>>>> So I think we need do both:
>>>>>>
>>>>>> 1) At one hand, it's a bug for the userspace to use vq_index before feature
>>>>>> is negotiated
>>>>>>
>>>>>> (looks like a bug in my cvq series that will call SET_VRING_CALL before
>>>>>> feature is negotiate, which I will look).
>>>>>>
>>>>>> 2) At the other hand, the driver should be able to deal with that
>>>>>>
>>>>> All I can do is drop callbacks for VQs before features negotation has
>>>>> been completed.
>>>>
>>>> Or just leave queue index 0, 1 work.
>>>>
>>>> Since it is not expected to be changed.
>>>>
>>> Right, will do.
>>>
>>>>>>> I think the CVQ index must not depend on the negotiated features but
>>>>>>> rather depend of the value the device driver provides in the call to
>>>>>>> _vdpa_register_device().
>>>>>> At the virtio level, it's too late to change that and it breaks the backward
>>>>>> compatibility.
>>>>>>
>>>>>> But at the vDPA level, the under layer device can map virtio cvq to any of
>>>>>> it's virtqueue.
>>>>>>
>>>>>> E.g map cvq (index 2) to mlx5 cvq (the last).
>>>>>>
>>>>> I am not following you here. I still don't know what index is cvq.
>>>>
>>>> Right, we still need to wait for the feature being negotiated in order to
>>>> proceed.
>>>>
>>> So to summarise, before feature negotiation complete, I accept calls
>>> referring to VQs only for indices 0 and 1.
>>> After feature negotiation complete I know CVQ index and will accept
>>> indices 0 to cvq index.
>> I don't get this "accept indices 0 to cvq index".
> What I meant to say is that there are several callbacks that refer to
> specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
> accept virtqueue index as an argument.
>
> What we want to do is verify wheather the index provided is valid or
> not. If it is not valid, either return error (if the callback can return
> a value) or just avoid processing it. If the index is valid then we
> process it normally.
>
> Now we need to decide which index is valid or not. We need something
> like this to identifiy valid indexes range:
>
> CVQ clear: 0 and 1
> CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> CVQ set, MQ set: 0..nvq where nvq is whatever provided to
> _vdpa_register_device()


Yes.


>
> And while writing this, I think this logic does not belog in mlx5_vdpa
> but probably in vdpa.c


The problem is that vdpa should be unaware of a specific device type. 
E.g the above indices may work only for virtio-net but not other.

Thanks


> 	
>> Thanks
>>
>>>> Thanks
>>>>
>>>>
>>>>>> Thanks
>>>>>>
>>>>>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/12 下午5:50, Eli Cohen 写道:
> On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
>> 在 2021/8/12 下午3:01, Eli Cohen 写道:
>>> On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
>>>> On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
>>>>> On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
>>>>>> 在 2021/8/11 下午7:04, Eli Cohen 写道:
>>>>>>> On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
>>>>>>>> 在 2021/8/11 下午3:53, Eli Cohen 写道:
>>>>>>>>>> One thing need to solve for mq is that the:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
>>>>>>>>>>> +{
>>>>>>>>>>> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>>>>>>>> +}
>>>>>>>>>> We should handle the case when MQ is supported by the device but not the
>>>>>>>>>> driver.
>>>>>>>>>>
>>>>>>>>>> E.g in the case when we have 2 queue pairs:
>>>>>>>>>>
>>>>>>>>>> When MQ is enabled, cvq is queue 4
>>>>>>>>>>
>>>>>>>>>> When MQ is not enabled, cvq is queue 2
>>>>>>>>>>
>>>>>>>>> There's some issue with this. I get callbacks targeting specific
>>>>>>>>> virtqueues before features negotiation has been completed.
>>>>>>>>>
>>>>>>>>> Specifically, I get set_vq_cb() calls. At this point I must know the
>>>>>>>>> control vq index.
>>>>>>>> So I think we need do both:
>>>>>>>>
>>>>>>>> 1) At one hand, it's a bug for the userspace to use vq_index before feature
>>>>>>>> is negotiated
>>>>>>>>
>>>>>>>> (looks like a bug in my cvq series that will call SET_VRING_CALL before
>>>>>>>> feature is negotiate, which I will look).
>>>>>>>>
>>>>>>>> 2) At the other hand, the driver should be able to deal with that
>>>>>>>>
>>>>>>> All I can do is drop callbacks for VQs before features negotation has
>>>>>>> been completed.
>>>>>> Or just leave queue index 0, 1 work.
>>>>>>
>>>>>> Since it is not expected to be changed.
>>>>>>
>>>>> Right, will do.
>>>>>
>>>>>>>>> I think the CVQ index must not depend on the negotiated features but
>>>>>>>>> rather depend of the value the device driver provides in the call to
>>>>>>>>> _vdpa_register_device().
>>>>>>>> At the virtio level, it's too late to change that and it breaks the backward
>>>>>>>> compatibility.
>>>>>>>>
>>>>>>>> But at the vDPA level, the under layer device can map virtio cvq to any of
>>>>>>>> it's virtqueue.
>>>>>>>>
>>>>>>>> E.g map cvq (index 2) to mlx5 cvq (the last).
>>>>>>>>
>>>>>>> I am not following you here. I still don't know what index is cvq.
>>>>>> Right, we still need to wait for the feature being negotiated in order to
>>>>>> proceed.
>>>>>>
>>>>> So to summarise, before feature negotiation complete, I accept calls
>>>>> referring to VQs only for indices 0 and 1.
>>>>> After feature negotiation complete I know CVQ index and will accept
>>>>> indices 0 to cvq index.
>>>> I don't get this "accept indices 0 to cvq index".
>>> What I meant to say is that there are several callbacks that refer to
>>> specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
>>> accept virtqueue index as an argument.
>>>
>>> What we want to do is verify wheather the index provided is valid or
>>> not. If it is not valid, either return error (if the callback can return
>>> a value) or just avoid processing it. If the index is valid then we
>>> process it normally.
>>>
>>> Now we need to decide which index is valid or not. We need something
>>> like this to identifiy valid indexes range:
>>>
>>> CVQ clear: 0 and 1
>>> CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
>>> CVQ set, MQ set: 0..nvq where nvq is whatever provided to
>>> _vdpa_register_device()
>>
>> Yes.
>>
> Unfortunately it does not work.
> set_vq_cb() for all the multiqueues is called beofre feature
> negotiation. If I apply the above logic, I will lose these settings.
>
> I can make an exception for set_vq_cb(), save callbacks and restore
> them afterwards. This looks too convoluted and maybe we should seek
> another solution.


I agree.


>
> Let me know what you think.


Rethink about this issue. It looks to the only issue we face is the 
set_vq_cb().

With the assumption that the userspace can use the index correctly (even 
before set_features). I wonder the following works.

Instead of checking whether the index is cvq in set_vq_cb() how about:

1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
2) have a dedicated event_cb array in mlx5_vdpa_net
3) then we can do

ndev->event_cbs[index] = *cb;

in mlx5_vdpa_set_vq_cb()

4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and 
we can use the correct index there.

In the mean time, I will look at Qemu code to see if we can guarantee 
that set_features is called before set_vq_callback. (At first glance, 
it's not trivial but let's see).

Thanks


>
>>> And while writing this, I think this logic does not belog in mlx5_vdpa
>>> but probably in vdpa.c
>>
>> The problem is that vdpa should be unaware of a specific device type. E.g
>> the above indices may work only for virtio-net but not other.
>>
>> Thanks
>>
>>
>>> 	
>>>> Thanks
>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/16 下午1:47, Eli Cohen 写道:
> On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
>> 在 2021/8/12 下午5:50, Eli Cohen 写道:
>>> On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
>>>> 在 2021/8/12 下午3:01, Eli Cohen 写道:
>>>>> On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
>>>>>> On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
>>>>>>> On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
>>>>>>>> 在 2021/8/11 下午7:04, Eli Cohen 写道:
>>>>>>>>> On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
>>>>>>>>>> 在 2021/8/11 下午3:53, Eli Cohen 写道:
>>>>>>>>>>>> One thing need to solve for mq is that the:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>>>>>>>>>> +}
>>>>>>>>>>>> We should handle the case when MQ is supported by the device but not the
>>>>>>>>>>>> driver.
>>>>>>>>>>>>
>>>>>>>>>>>> E.g in the case when we have 2 queue pairs:
>>>>>>>>>>>>
>>>>>>>>>>>> When MQ is enabled, cvq is queue 4
>>>>>>>>>>>>
>>>>>>>>>>>> When MQ is not enabled, cvq is queue 2
>>>>>>>>>>>>
>>>>>>>>>>> There's some issue with this. I get callbacks targeting specific
>>>>>>>>>>> virtqueues before features negotiation has been completed.
>>>>>>>>>>>
>>>>>>>>>>> Specifically, I get set_vq_cb() calls. At this point I must know the
>>>>>>>>>>> control vq index.
>>>>>>>>>> So I think we need do both:
>>>>>>>>>>
>>>>>>>>>> 1) At one hand, it's a bug for the userspace to use vq_index before feature
>>>>>>>>>> is negotiated
>>>>>>>>>>
>>>>>>>>>> (looks like a bug in my cvq series that will call SET_VRING_CALL before
>>>>>>>>>> feature is negotiate, which I will look).
>>>>>>>>>>
>>>>>>>>>> 2) At the other hand, the driver should be able to deal with that
>>>>>>>>>>
>>>>>>>>> All I can do is drop callbacks for VQs before features negotation has
>>>>>>>>> been completed.
>>>>>>>> Or just leave queue index 0, 1 work.
>>>>>>>>
>>>>>>>> Since it is not expected to be changed.
>>>>>>>>
>>>>>>> Right, will do.
>>>>>>>
>>>>>>>>>>> I think the CVQ index must not depend on the negotiated features but
>>>>>>>>>>> rather depend of the value the device driver provides in the call to
>>>>>>>>>>> _vdpa_register_device().
>>>>>>>>>> At the virtio level, it's too late to change that and it breaks the backward
>>>>>>>>>> compatibility.
>>>>>>>>>>
>>>>>>>>>> But at the vDPA level, the under layer device can map virtio cvq to any of
>>>>>>>>>> it's virtqueue.
>>>>>>>>>>
>>>>>>>>>> E.g map cvq (index 2) to mlx5 cvq (the last).
>>>>>>>>>>
>>>>>>>>> I am not following you here. I still don't know what index is cvq.
>>>>>>>> Right, we still need to wait for the feature being negotiated in order to
>>>>>>>> proceed.
>>>>>>>>
>>>>>>> So to summarise, before feature negotiation complete, I accept calls
>>>>>>> referring to VQs only for indices 0 and 1.
>>>>>>> After feature negotiation complete I know CVQ index and will accept
>>>>>>> indices 0 to cvq index.
>>>>>> I don't get this "accept indices 0 to cvq index".
>>>>> What I meant to say is that there are several callbacks that refer to
>>>>> specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
>>>>> accept virtqueue index as an argument.
>>>>>
>>>>> What we want to do is verify wheather the index provided is valid or
>>>>> not. If it is not valid, either return error (if the callback can return
>>>>> a value) or just avoid processing it. If the index is valid then we
>>>>> process it normally.
>>>>>
>>>>> Now we need to decide which index is valid or not. We need something
>>>>> like this to identifiy valid indexes range:
>>>>>
>>>>> CVQ clear: 0 and 1
>>>>> CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
>>>>> CVQ set, MQ set: 0..nvq where nvq is whatever provided to
>>>>> _vdpa_register_device()
>>>> Yes.
>>>>
>>> Unfortunately it does not work.
>>> set_vq_cb() for all the multiqueues is called beofre feature
>>> negotiation. If I apply the above logic, I will lose these settings.
>>>
>>> I can make an exception for set_vq_cb(), save callbacks and restore
>>> them afterwards. This looks too convoluted and maybe we should seek
>>> another solution.
>>
>> I agree.
>>
>>
>>> Let me know what you think.
>>
>> Rethink about this issue. It looks to the only issue we face is the
>> set_vq_cb().
>>
>> With the assumption that the userspace can use the index correctly (even
>> before set_features). I wonder the following works.
>>
>> Instead of checking whether the index is cvq in set_vq_cb() how about:
>>
>> 1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
>> 2) have a dedicated event_cb array in mlx5_vdpa_net
>> 3) then we can do
>>
>> ndev->event_cbs[index] = *cb;
>>
> So actually you're suggesting to save all the callabck configurations in
> an array and evaluate cvq index after feature negotiation has been
> completed.


Yes.


>
> I think that could work. I will code this and update.


Thanks.


>
>> in mlx5_vdpa_set_vq_cb()
>>
>> 4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and we
>> can use the correct index there.
>>
>> In the mean time, I will look at Qemu code to see if we can guarantee that
>> set_features is called before set_vq_callback. (At first glance, it's not
>> trivial but let's see).
>>
>> Thanks
>>
>>
>>>>> And while writing this, I think this logic does not belog in mlx5_vdpa
>>>>> but probably in vdpa.c
>>>> The problem is that vdpa should be unaware of a specific device type. E.g
>>>> the above indices may work only for virtio-net but not other.
>>>>
>>>> Thanks
>>>>
>>>>
>>>>> 	
>>>>>> Thanks
>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Monday, August 16, 2021 10:27 PM
> 
> It works fine when I am working with your version of qemu with support for
> multi queue.
> 
> The problem is that it is broken on qemu v6.0.0. If I register my vdpa device
> with more than 2 data virtqueues, qemu won't even create a netdevice in the
> VM.
> 
> I am not sure how to handle this. Is there some kind of indication I can get as
> to the version of qemu so I can avoid using multiqueue for versions I know
> are problematic?

When user is creating the vdpa device, user should mention how many VQs to use.
We already expose max vqs that device can support.
Instead of some driver arbitrary value, it should be user specified.
At least for Netdev, it make sense to have number of VQs to be <= vcpus.
This knowledge is with QEMU users.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > 
> > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
> > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
> > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > We should handle the case when MQ is supported by the device but not the
> > > > > > > > > > > > > > driver.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > There's some issue with this. I get callbacks targeting specific
> > > > > > > > > > > > > virtqueues before features negotiation has been completed.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At this point I must know the
> > > > > > > > > > > > > control vq index.
> > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > 
> > > > > > > > > > > > 1) At one hand, it's a bug for the userspace to use vq_index before feature
> > > > > > > > > > > > is negotiated
> > > > > > > > > > > > 
> > > > > > > > > > > > (looks like a bug in my cvq series that will call SET_VRING_CALL before
> > > > > > > > > > > > feature is negotiate, which I will look).
> > > > > > > > > > > > 
> > > > > > > > > > > > 2) At the other hand, the driver should be able to deal with that
> > > > > > > > > > > > 
> > > > > > > > > > > All I can do is drop callbacks for VQs before features negotation has
> > > > > > > > > > > been completed.
> > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > 
> > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > 
> > > > > > > > > Right, will do.
> > > > > > > > > 
> > > > > > > > > > > > > I think the CVQ index must not depend on the negotiated features but
> > > > > > > > > > > > > rather depend of the value the device driver provides in the call to
> > > > > > > > > > > > > _vdpa_register_device().
> > > > > > > > > > > > At the virtio level, it's too late to change that and it breaks the backward
> > > > > > > > > > > > compatibility.
> > > > > > > > > > > > 
> > > > > > > > > > > > But at the vDPA level, the under layer device can map virtio cvq to any of
> > > > > > > > > > > > it's virtqueue.
> > > > > > > > > > > > 
> > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > 
> > > > > > > > > > > I am not following you here. I still don't know what index is cvq.
> > > > > > > > > > Right, we still need to wait for the feature being negotiated in order to
> > > > > > > > > > proceed.
> > > > > > > > > > 
> > > > > > > > > So to summarise, before feature negotiation complete, I accept calls
> > > > > > > > > referring to VQs only for indices 0 and 1.
> > > > > > > > > After feature negotiation complete I know CVQ index and will accept
> > > > > > > > > indices 0 to cvq index.
> > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > What I meant to say is that there are several callbacks that refer to
> > > > > > > specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
> > > > > > > accept virtqueue index as an argument.
> > > > > > > 
> > > > > > > What we want to do is verify wheather the index provided is valid or
> > > > > > > not. If it is not valid, either return error (if the callback can return
> > > > > > > a value) or just avoid processing it. If the index is valid then we
> > > > > > > process it normally.
> > > > > > > 
> > > > > > > Now we need to decide which index is valid or not. We need something
> > > > > > > like this to identifiy valid indexes range:
> > > > > > > 
> > > > > > > CVQ clear: 0 and 1
> > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided to
> > > > > > > _vdpa_register_device()
> > > > > > Yes.
> > > > > > 
> > > > > Unfortunately it does not work.
> > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > 
> > > > > I can make an exception for set_vq_cb(), save callbacks and restore
> > > > > them afterwards. This looks too convoluted and maybe we should seek
> > > > > another solution.
> > > > 
> > > > I agree.
> > > > 
> > > > 
> > > > > Let me know what you think.
> > > > 
> > > > Rethink about this issue. It looks to the only issue we face is the
> > > > set_vq_cb().
> > > > 
> > > > With the assumption that the userspace can use the index correctly (even
> > > > before set_features). I wonder the following works.
> > > > 
> > > > Instead of checking whether the index is cvq in set_vq_cb() how about:
> > > > 
> > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
> > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > 3) then we can do
> > > > 
> > > > ndev->event_cbs[index] = *cb;
> > > > 
> > > So actually you're suggesting to save all the callabck configurations in
> > > an array and evaluate cvq index after feature negotiation has been
> > > completed.
> > 
> > 
> > Yes.
> > 
> > 
> > > 
> > > I think that could work. I will code this and update.
> > 
> 
> It works fine when I am working with your version of qemu with support
> for multi queue.
> 
> The problem is that it is broken on qemu v6.0.0. If I register my vdpa
> device with more than 2 data virtqueues, qemu won't even create a
> netdevice in the VM.
> 
> I am not sure how to handle this. Is there some kind of indication I can
> get as to the version of qemu so I can avoid using multiqueue for
> versions I know are problematic?

No versions ;) This is what feature bits are for ...

> > 
> > Thanks.
> > 
> > 
> > > 
> > > > in mlx5_vdpa_set_vq_cb()
> > > > 
> > > > 4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and we
> > > > can use the correct index there.
> > > > 
> > > > In the mean time, I will look at Qemu code to see if we can guarantee that
> > > > set_features is called before set_vq_callback. (At first glance, it's not
> > > > trivial but let's see).
> > > > 
> > > > Thanks
> > > > 
> > > > 
> > > > > > > And while writing this, I think this logic does not belog in mlx5_vdpa
> > > > > > > but probably in vdpa.c
> > > > > > The problem is that vdpa should be unaware of a specific device type. E.g
> > > > > > the above indices may work only for virtio-net but not other.
> > > > > > 
> > > > > > Thanks
> > > > > > 
> > > > > > 
> > > > > > > 	
> > > > > > > > Thanks
> > > > > > > > 
> > > > > > > > > > Thanks
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > > Thanks
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > >
> > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
> > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > We should handle the case when MQ is supported by the device but not the
> > > > > > > > > > > > > > > driver.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > There's some issue with this. I get callbacks targeting specific
> > > > > > > > > > > > > > virtqueues before features negotiation has been completed.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At this point I must know the
> > > > > > > > > > > > > > control vq index.
> > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > >
> > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace to use vq_index before feature
> > > > > > > > > > > > > is negotiated
> > > > > > > > > > > > >
> > > > > > > > > > > > > (looks like a bug in my cvq series that will call SET_VRING_CALL before
> > > > > > > > > > > > > feature is negotiate, which I will look).
> > > > > > > > > > > > >
> > > > > > > > > > > > > 2) At the other hand, the driver should be able to deal with that
> > > > > > > > > > > > >
> > > > > > > > > > > > All I can do is drop callbacks for VQs before features negotation has
> > > > > > > > > > > > been completed.
> > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > >
> > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > >
> > > > > > > > > > Right, will do.
> > > > > > > > > >
> > > > > > > > > > > > > > I think the CVQ index must not depend on the negotiated features but
> > > > > > > > > > > > > > rather depend of the value the device driver provides in the call to
> > > > > > > > > > > > > > _vdpa_register_device().
> > > > > > > > > > > > > At the virtio level, it's too late to change that and it breaks the backward
> > > > > > > > > > > > > compatibility.
> > > > > > > > > > > > >
> > > > > > > > > > > > > But at the vDPA level, the under layer device can map virtio cvq to any of
> > > > > > > > > > > > > it's virtqueue.
> > > > > > > > > > > > >
> > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > >
> > > > > > > > > > > > I am not following you here. I still don't know what index is cvq.
> > > > > > > > > > > Right, we still need to wait for the feature being negotiated in order to
> > > > > > > > > > > proceed.
> > > > > > > > > > >
> > > > > > > > > > So to summarise, before feature negotiation complete, I accept calls
> > > > > > > > > > referring to VQs only for indices 0 and 1.
> > > > > > > > > > After feature negotiation complete I know CVQ index and will accept
> > > > > > > > > > indices 0 to cvq index.
> > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > What I meant to say is that there are several callbacks that refer to
> > > > > > > > specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
> > > > > > > > accept virtqueue index as an argument.
> > > > > > > >
> > > > > > > > What we want to do is verify wheather the index provided is valid or
> > > > > > > > not. If it is not valid, either return error (if the callback can return
> > > > > > > > a value) or just avoid processing it. If the index is valid then we
> > > > > > > > process it normally.
> > > > > > > >
> > > > > > > > Now we need to decide which index is valid or not. We need something
> > > > > > > > like this to identifiy valid indexes range:
> > > > > > > >
> > > > > > > > CVQ clear: 0 and 1
> > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided to
> > > > > > > > _vdpa_register_device()
> > > > > > > Yes.
> > > > > > >
> > > > > > Unfortunately it does not work.
> > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > >
> > > > > > I can make an exception for set_vq_cb(), save callbacks and restore
> > > > > > them afterwards. This looks too convoluted and maybe we should seek
> > > > > > another solution.
> > > > >
> > > > > I agree.
> > > > >
> > > > >
> > > > > > Let me know what you think.
> > > > >
> > > > > Rethink about this issue. It looks to the only issue we face is the
> > > > > set_vq_cb().
> > > > >
> > > > > With the assumption that the userspace can use the index correctly (even
> > > > > before set_features). I wonder the following works.
> > > > >
> > > > > Instead of checking whether the index is cvq in set_vq_cb() how about:
> > > > >
> > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
> > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > 3) then we can do
> > > > >
> > > > > ndev->event_cbs[index] = *cb;
> > > > >
> > > > So actually you're suggesting to save all the callabck configurations in
> > > > an array and evaluate cvq index after feature negotiation has been
> > > > completed.
> > >
> > >
> > > Yes.
> > >
> > >
> > > >
> > > > I think that could work. I will code this and update.
> > >
> >
> > It works fine when I am working with your version of qemu with support
> > for multi queue.
> >
> > The problem is that it is broken on qemu v6.0.0. If I register my vdpa
> > device with more than 2 data virtqueues, qemu won't even create a
> > netdevice in the VM.

Qemu should hide MQ feature in this case but looks like it doesn't.

Will have a look.

> >
> > I am not sure how to handle this. Is there some kind of indication I can
> > get as to the version of qemu so I can avoid using multiqueue for
> > versions I know are problematic?
>
> No versions ;) This is what feature bits are for ...

Yes.

So does it work if "mq=off" is specified in the command line?

Thanks


>
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > > in mlx5_vdpa_set_vq_cb()
> > > > >
> > > > > 4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and we
> > > > > can use the correct index there.
> > > > >
> > > > > In the mean time, I will look at Qemu code to see if we can guarantee that
> > > > > set_features is called before set_vq_callback. (At first glance, it's not
> > > > > trivial but let's see).
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > > > > And while writing this, I think this logic does not belog in mlx5_vdpa
> > > > > > > > but probably in vdpa.c
> > > > > > > The problem is that vdpa should be unaware of a specific device type. E.g
> > > > > > > the above indices may work only for virtio-net but not other.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > Thanks
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, August 17, 2021 9:26 AM
> 
> On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > >
> > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
> <elic@nvidia.com> wrote:
> > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
> wrote:
> > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
> Wang wrote:
> > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct
> > > > > > > > > > > > > > > > > +mlx5_vdpa_dev *mvdev) {
> > > > > > > > > > > > > > > > > +     return 2 *
> > > > > > > > > > > > > > > > > +mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > We should handle the case when MQ is
> > > > > > > > > > > > > > > > supported by the device but not the driver.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > There's some issue with this. I get
> > > > > > > > > > > > > > > callbacks targeting specific virtqueues before
> features negotiation has been completed.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At
> > > > > > > > > > > > > > > this point I must know the control vq index.
> > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace
> > > > > > > > > > > > > > to use vq_index before feature is negotiated
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > (looks like a bug in my cvq series that will
> > > > > > > > > > > > > > call SET_VRING_CALL before feature is negotiate,
> which I will look).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2) At the other hand, the driver should be
> > > > > > > > > > > > > > able to deal with that
> > > > > > > > > > > > > >
> > > > > > > > > > > > > All I can do is drop callbacks for VQs before
> > > > > > > > > > > > > features negotation has been completed.
> > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > >
> > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > >
> > > > > > > > > > > Right, will do.
> > > > > > > > > > >
> > > > > > > > > > > > > > > I think the CVQ index must not depend on the
> > > > > > > > > > > > > > > negotiated features but rather depend of the
> > > > > > > > > > > > > > > value the device driver provides in the call to
> _vdpa_register_device().
> > > > > > > > > > > > > > At the virtio level, it's too late to change
> > > > > > > > > > > > > > that and it breaks the backward compatibility.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > But at the vDPA level, the under layer device
> > > > > > > > > > > > > > can map virtio cvq to any of it's virtqueue.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > I am not following you here. I still don't know what
> index is cvq.
> > > > > > > > > > > > Right, we still need to wait for the feature being
> > > > > > > > > > > > negotiated in order to proceed.
> > > > > > > > > > > >
> > > > > > > > > > > So to summarise, before feature negotiation
> > > > > > > > > > > complete, I accept calls referring to VQs only for indices 0
> and 1.
> > > > > > > > > > > After feature negotiation complete I know CVQ index
> > > > > > > > > > > and will accept indices 0 to cvq index.
> > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > What I meant to say is that there are several callbacks
> > > > > > > > > that refer to specific virtqueues, e.g.
> > > > > > > > > set_vq_address(), set_vq_num() etc. They all accept virtqueue
> index as an argument.
> > > > > > > > >
> > > > > > > > > What we want to do is verify wheather the index provided
> > > > > > > > > is valid or not. If it is not valid, either return error
> > > > > > > > > (if the callback can return a value) or just avoid
> > > > > > > > > processing it. If the index is valid then we process it normally.
> > > > > > > > >
> > > > > > > > > Now we need to decide which index is valid or not. We
> > > > > > > > > need something like this to identifiy valid indexes range:
> > > > > > > > >
> > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided
> > > > > > > > > to
> > > > > > > > > _vdpa_register_device()
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > Unfortunately it does not work.
> > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > >
> > > > > > > I can make an exception for set_vq_cb(), save callbacks and
> > > > > > > restore them afterwards. This looks too convoluted and maybe
> > > > > > > we should seek another solution.
> > > > > >
> > > > > > I agree.
> > > > > >
> > > > > >
> > > > > > > Let me know what you think.
> > > > > >
> > > > > > Rethink about this issue. It looks to the only issue we face
> > > > > > is the set_vq_cb().
> > > > > >
> > > > > > With the assumption that the userspace can use the index
> > > > > > correctly (even before set_features). I wonder the following works.
> > > > > >
> > > > > > Instead of checking whether the index is cvq in set_vq_cb() how
> about:
> > > > > >
> > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and
> > > > > > mlx5_congro_vq
> > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > 3) then we can do
> > > > > >
> > > > > > ndev->event_cbs[index] = *cb;
> > > > > >
> > > > > So actually you're suggesting to save all the callabck
> > > > > configurations in an array and evaluate cvq index after feature
> > > > > negotiation has been completed.
> > > >
> > > >
> > > > Yes.
> > > >
> > > >
> > > > >
> > > > > I think that could work. I will code this and update.
> > > >
> > >
> > > It works fine when I am working with your version of qemu with
> > > support for multi queue.
> > >
> > > The problem is that it is broken on qemu v6.0.0. If I register my
> > > vdpa device with more than 2 data virtqueues, qemu won't even create
> > > a netdevice in the VM.
> 
> Qemu should hide MQ feature in this case but looks like it doesn't.
> 
> Will have a look.
> 
> > >
> > > I am not sure how to handle this. Is there some kind of indication I
> > > can get as to the version of qemu so I can avoid using multiqueue
> > > for versions I know are problematic?
> >
> > No versions ;) This is what feature bits are for ...
> 
> Yes.
> 
> So does it work if "mq=off" is specified in the command line?
> 
We should not add driver module parameter.
We anyway need number of VQs to be driven by the number of vcpus used by the VM.
So why not specify this when creating a vdpa device?

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/17 下午12:03, Parav Pandit 写道:
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Tuesday, August 17, 2021 9:26 AM
>>
>> On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com>
>> wrote:
>>> On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
>>>> On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
>>>>> 在 2021/8/16 下午1:47, Eli Cohen 写道:
>>>>>> On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
>>>>>>> 在 2021/8/12 下午5:50, Eli Cohen 写道:
>>>>>>>> On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
>>>>>>>>> 在 2021/8/12 下午3:01, Eli Cohen 写道:
>>>>>>>>>> On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
>>>>>>>>>>> On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
>> <elic@nvidia.com> wrote:
>>>>>>>>>>>> On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
>> wrote:
>>>>>>>>>>>>> 在 2021/8/11 下午7:04, Eli Cohen 写道:
>>>>>>>>>>>>>> On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
>> Wang wrote:
>>>>>>>>>>>>>>> 在 2021/8/11 下午3:53, Eli Cohen 写道:
>>>>>>>>>>>>>>>>> One thing need to solve for mq is that the:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +static u16 ctrl_vq_idx(struct
>>>>>>>>>>>>>>>>>> +mlx5_vdpa_dev *mvdev) {
>>>>>>>>>>>>>>>>>> +     return 2 *
>>>>>>>>>>>>>>>>>> +mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> We should handle the case when MQ is
>>>>>>>>>>>>>>>>> supported by the device but not the driver.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> E.g in the case when we have 2 queue pairs:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> When MQ is enabled, cvq is queue 4
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> When MQ is not enabled, cvq is queue 2
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> There's some issue with this. I get
>>>>>>>>>>>>>>>> callbacks targeting specific virtqueues before
>> features negotiation has been completed.
>>>>>>>>>>>>>>>> Specifically, I get set_vq_cb() calls. At
>>>>>>>>>>>>>>>> this point I must know the control vq index.
>>>>>>>>>>>>>>> So I think we need do both:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 1) At one hand, it's a bug for the userspace
>>>>>>>>>>>>>>> to use vq_index before feature is negotiated
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> (looks like a bug in my cvq series that will
>>>>>>>>>>>>>>> call SET_VRING_CALL before feature is negotiate,
>> which I will look).
>>>>>>>>>>>>>>> 2) At the other hand, the driver should be
>>>>>>>>>>>>>>> able to deal with that
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All I can do is drop callbacks for VQs before
>>>>>>>>>>>>>> features negotation has been completed.
>>>>>>>>>>>>> Or just leave queue index 0, 1 work.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Since it is not expected to be changed.
>>>>>>>>>>>>>
>>>>>>>>>>>> Right, will do.
>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I think the CVQ index must not depend on the
>>>>>>>>>>>>>>>> negotiated features but rather depend of the
>>>>>>>>>>>>>>>> value the device driver provides in the call to
>> _vdpa_register_device().
>>>>>>>>>>>>>>> At the virtio level, it's too late to change
>>>>>>>>>>>>>>> that and it breaks the backward compatibility.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But at the vDPA level, the under layer device
>>>>>>>>>>>>>>> can map virtio cvq to any of it's virtqueue.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> E.g map cvq (index 2) to mlx5 cvq (the last).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not following you here. I still don't know what
>> index is cvq.
>>>>>>>>>>>>> Right, we still need to wait for the feature being
>>>>>>>>>>>>> negotiated in order to proceed.
>>>>>>>>>>>>>
>>>>>>>>>>>> So to summarise, before feature negotiation
>>>>>>>>>>>> complete, I accept calls referring to VQs only for indices 0
>> and 1.
>>>>>>>>>>>> After feature negotiation complete I know CVQ index
>>>>>>>>>>>> and will accept indices 0 to cvq index.
>>>>>>>>>>> I don't get this "accept indices 0 to cvq index".
>>>>>>>>>> What I meant to say is that there are several callbacks
>>>>>>>>>> that refer to specific virtqueues, e.g.
>>>>>>>>>> set_vq_address(), set_vq_num() etc. They all accept virtqueue
>> index as an argument.
>>>>>>>>>> What we want to do is verify wheather the index provided
>>>>>>>>>> is valid or not. If it is not valid, either return error
>>>>>>>>>> (if the callback can return a value) or just avoid
>>>>>>>>>> processing it. If the index is valid then we process it normally.
>>>>>>>>>>
>>>>>>>>>> Now we need to decide which index is valid or not. We
>>>>>>>>>> need something like this to identifiy valid indexes range:
>>>>>>>>>>
>>>>>>>>>> CVQ clear: 0 and 1
>>>>>>>>>> CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
>>>>>>>>>> CVQ set, MQ set: 0..nvq where nvq is whatever provided
>>>>>>>>>> to
>>>>>>>>>> _vdpa_register_device()
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>> Unfortunately it does not work.
>>>>>>>> set_vq_cb() for all the multiqueues is called beofre feature
>>>>>>>> negotiation. If I apply the above logic, I will lose these settings.
>>>>>>>>
>>>>>>>> I can make an exception for set_vq_cb(), save callbacks and
>>>>>>>> restore them afterwards. This looks too convoluted and maybe
>>>>>>>> we should seek another solution.
>>>>>>> I agree.
>>>>>>>
>>>>>>>
>>>>>>>> Let me know what you think.
>>>>>>> Rethink about this issue. It looks to the only issue we face
>>>>>>> is the set_vq_cb().
>>>>>>>
>>>>>>> With the assumption that the userspace can use the index
>>>>>>> correctly (even before set_features). I wonder the following works.
>>>>>>>
>>>>>>> Instead of checking whether the index is cvq in set_vq_cb() how
>> about:
>>>>>>> 1) decouple event_cb out of mlx5_vdpa_virtqueue and
>>>>>>> mlx5_congro_vq
>>>>>>> 2) have a dedicated event_cb array in mlx5_vdpa_net
>>>>>>> 3) then we can do
>>>>>>>
>>>>>>> ndev->event_cbs[index] = *cb;
>>>>>>>
>>>>>> So actually you're suggesting to save all the callabck
>>>>>> configurations in an array and evaluate cvq index after feature
>>>>>> negotiation has been completed.
>>>>>
>>>>> Yes.
>>>>>
>>>>>
>>>>>> I think that could work. I will code this and update.
>>>> It works fine when I am working with your version of qemu with
>>>> support for multi queue.
>>>>
>>>> The problem is that it is broken on qemu v6.0.0. If I register my
>>>> vdpa device with more than 2 data virtqueues, qemu won't even create
>>>> a netdevice in the VM.
>> Qemu should hide MQ feature in this case but looks like it doesn't.
>>
>> Will have a look.
>>
>>>> I am not sure how to handle this. Is there some kind of indication I
>>>> can get as to the version of qemu so I can avoid using multiqueue
>>>> for versions I know are problematic?
>>> No versions ;) This is what feature bits are for ...
>> Yes.
>>
>> So does it work if "mq=off" is specified in the command line?
>>
> We should not add driver module parameter.


Note that, it's not a module parameter but a qemu command line to 
disable mq feature.


> We anyway need number of VQs to be driven by the number of vcpus used by the VM.
> So why not specify this when creating a vdpa device?


Yes, I think it should work as well.

So management need either:

1) disable multiqueue via "mq=off"

or

2) using netlink API to create a single queue pair device

for the qemu <=6.1.

Thanks


>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Tue, Aug 17, 2021 at 1:26 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote:
> >
> > 在 2021/8/17 下午12:03, Parav Pandit 写道:
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, August 17, 2021 9:26 AM
> > > >
> > > > On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com>
> > > > wrote:
> > > > > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > > > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > > > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
> > > > <elic@nvidia.com> wrote:
> > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
> > > > wrote:
> > > > > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
> > > > Wang wrote:
> > > > > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct
> > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_dev *mvdev) {
> > > > > > > > > > > > > > > > > > > > +     return 2 *
> > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > > > > We should handle the case when MQ is
> > > > > > > > > > > > > > > > > > > supported by the device but not the driver.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > There's some issue with this. I get
> > > > > > > > > > > > > > > > > > callbacks targeting specific virtqueues before
> > > > features negotiation has been completed.
> > > > > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At
> > > > > > > > > > > > > > > > > > this point I must know the control vq index.
> > > > > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace
> > > > > > > > > > > > > > > > > to use vq_index before feature is negotiated
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > (looks like a bug in my cvq series that will
> > > > > > > > > > > > > > > > > call SET_VRING_CALL before feature is negotiate,
> > > > which I will look).
> > > > > > > > > > > > > > > > > 2) At the other hand, the driver should be
> > > > > > > > > > > > > > > > > able to deal with that
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > All I can do is drop callbacks for VQs before
> > > > > > > > > > > > > > > > features negotation has been completed.
> > > > > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > Right, will do.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I think the CVQ index must not depend on the
> > > > > > > > > > > > > > > > > > negotiated features but rather depend of the
> > > > > > > > > > > > > > > > > > value the device driver provides in the call to
> > > > _vdpa_register_device().
> > > > > > > > > > > > > > > > > At the virtio level, it's too late to change
> > > > > > > > > > > > > > > > > that and it breaks the backward compatibility.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > But at the vDPA level, the under layer device
> > > > > > > > > > > > > > > > > can map virtio cvq to any of it's virtqueue.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I am not following you here. I still don't know what
> > > > index is cvq.
> > > > > > > > > > > > > > > Right, we still need to wait for the feature being
> > > > > > > > > > > > > > > negotiated in order to proceed.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > So to summarise, before feature negotiation
> > > > > > > > > > > > > > complete, I accept calls referring to VQs only for indices 0
> > > > and 1.
> > > > > > > > > > > > > > After feature negotiation complete I know CVQ index
> > > > > > > > > > > > > > and will accept indices 0 to cvq index.
> > > > > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > > > > What I meant to say is that there are several callbacks
> > > > > > > > > > > > that refer to specific virtqueues, e.g.
> > > > > > > > > > > > set_vq_address(), set_vq_num() etc. They all accept virtqueue
> > > > index as an argument.
> > > > > > > > > > > > What we want to do is verify wheather the index provided
> > > > > > > > > > > > is valid or not. If it is not valid, either return error
> > > > > > > > > > > > (if the callback can return a value) or just avoid
> > > > > > > > > > > > processing it. If the index is valid then we process it normally.
> > > > > > > > > > > >
> > > > > > > > > > > > Now we need to decide which index is valid or not. We
> > > > > > > > > > > > need something like this to identifiy valid indexes range:
> > > > > > > > > > > >
> > > > > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided
> > > > > > > > > > > > to
> > > > > > > > > > > > _vdpa_register_device()
> > > > > > > > > > > Yes.
> > > > > > > > > > >
> > > > > > > > > > Unfortunately it does not work.
> > > > > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > > > > >
> > > > > > > > > > I can make an exception for set_vq_cb(), save callbacks and
> > > > > > > > > > restore them afterwards. This looks too convoluted and maybe
> > > > > > > > > > we should seek another solution.
> > > > > > > > > I agree.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Let me know what you think.
> > > > > > > > > Rethink about this issue. It looks to the only issue we face
> > > > > > > > > is the set_vq_cb().
> > > > > > > > >
> > > > > > > > > With the assumption that the userspace can use the index
> > > > > > > > > correctly (even before set_features). I wonder the following works.
> > > > > > > > >
> > > > > > > > > Instead of checking whether the index is cvq in set_vq_cb() how
> > > > about:
> > > > > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and
> > > > > > > > > mlx5_congro_vq
> > > > > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > > > > 3) then we can do
> > > > > > > > >
> > > > > > > > > ndev->event_cbs[index] = *cb;
> > > > > > > > >
> > > > > > > > So actually you're suggesting to save all the callabck
> > > > > > > > configurations in an array and evaluate cvq index after feature
> > > > > > > > negotiation has been completed.
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > >
> > > > > > > > I think that could work. I will code this and update.
> > > > > > It works fine when I am working with your version of qemu with
> > > > > > support for multi queue.
> > > > > >
> > > > > > The problem is that it is broken on qemu v6.0.0. If I register my
> > > > > > vdpa device with more than 2 data virtqueues, qemu won't even create
> > > > > > a netdevice in the VM.
> > > > Qemu should hide MQ feature in this case but looks like it doesn't.
> > > >
> > > > Will have a look.
> > > >
> > > > > > I am not sure how to handle this. Is there some kind of indication I
> > > > > > can get as to the version of qemu so I can avoid using multiqueue
> > > > > > for versions I know are problematic?
> > > > > No versions ;) This is what feature bits are for ...
> > > > Yes.
> > > >
> > > > So does it work if "mq=off" is specified in the command line?
> > > >
> > > We should not add driver module parameter.
> >
> >
> > Note that, it's not a module parameter but a qemu command line to disable mq
> > feature.
> >
> >
> > > We anyway need number of VQs to be driven by the number of vcpus used by the VM.
> > > So why not specify this when creating a vdpa device?
> >
> >
> > Yes, I think it should work as well.
> >
> > So management need either:
> >
> > 1) disable multiqueue via "mq=off"
> >
> > or
> >
> > 2) using netlink API to create a single queue pair device
> >
> > for the qemu <=6.1.
> >
>
> Which management entity are you referring to here?

The one that launches Qemu. (E.g libvirt or other).

Thanks

>
> > Thanks
> >
> >
> > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Tue, Aug 17, 2021 at 2:01 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Tue, Aug 17, 2021 at 01:48:17PM +0800, Jason Wang wrote:
> > On Tue, Aug 17, 2021 at 1:26 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote:
> > > >
> > > > 在 2021/8/17 下午12:03, Parav Pandit 写道:
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Tuesday, August 17, 2021 9:26 AM
> > > > > >
> > > > > > On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com>
> > > > > > wrote:
> > > > > > > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > > > > > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > > > > > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > > > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
> > > > > > <elic@nvidia.com> wrote:
> > > > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
> > > > > > wrote:
> > > > > > > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
> > > > > > Wang wrote:
> > > > > > > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct
> > > > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_dev *mvdev) {
> > > > > > > > > > > > > > > > > > > > > > +     return 2 *
> > > > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > > > > > > We should handle the case when MQ is
> > > > > > > > > > > > > > > > > > > > > supported by the device but not the driver.
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > There's some issue with this. I get
> > > > > > > > > > > > > > > > > > > > callbacks targeting specific virtqueues before
> > > > > > features negotiation has been completed.
> > > > > > > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At
> > > > > > > > > > > > > > > > > > > > this point I must know the control vq index.
> > > > > > > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace
> > > > > > > > > > > > > > > > > > > to use vq_index before feature is negotiated
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > (looks like a bug in my cvq series that will
> > > > > > > > > > > > > > > > > > > call SET_VRING_CALL before feature is negotiate,
> > > > > > which I will look).
> > > > > > > > > > > > > > > > > > > 2) At the other hand, the driver should be
> > > > > > > > > > > > > > > > > > > able to deal with that
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > All I can do is drop callbacks for VQs before
> > > > > > > > > > > > > > > > > > features negotation has been completed.
> > > > > > > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Right, will do.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > I think the CVQ index must not depend on the
> > > > > > > > > > > > > > > > > > > > negotiated features but rather depend of the
> > > > > > > > > > > > > > > > > > > > value the device driver provides in the call to
> > > > > > _vdpa_register_device().
> > > > > > > > > > > > > > > > > > > At the virtio level, it's too late to change
> > > > > > > > > > > > > > > > > > > that and it breaks the backward compatibility.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > But at the vDPA level, the under layer device
> > > > > > > > > > > > > > > > > > > can map virtio cvq to any of it's virtqueue.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I am not following you here. I still don't know what
> > > > > > index is cvq.
> > > > > > > > > > > > > > > > > Right, we still need to wait for the feature being
> > > > > > > > > > > > > > > > > negotiated in order to proceed.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > So to summarise, before feature negotiation
> > > > > > > > > > > > > > > > complete, I accept calls referring to VQs only for indices 0
> > > > > > and 1.
> > > > > > > > > > > > > > > > After feature negotiation complete I know CVQ index
> > > > > > > > > > > > > > > > and will accept indices 0 to cvq index.
> > > > > > > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > > > > > > What I meant to say is that there are several callbacks
> > > > > > > > > > > > > > that refer to specific virtqueues, e.g.
> > > > > > > > > > > > > > set_vq_address(), set_vq_num() etc. They all accept virtqueue
> > > > > > index as an argument.
> > > > > > > > > > > > > > What we want to do is verify wheather the index provided
> > > > > > > > > > > > > > is valid or not. If it is not valid, either return error
> > > > > > > > > > > > > > (if the callback can return a value) or just avoid
> > > > > > > > > > > > > > processing it. If the index is valid then we process it normally.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Now we need to decide which index is valid or not. We
> > > > > > > > > > > > > > need something like this to identifiy valid indexes range:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > _vdpa_register_device()
> > > > > > > > > > > > > Yes.
> > > > > > > > > > > > >
> > > > > > > > > > > > Unfortunately it does not work.
> > > > > > > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > > > > > > >
> > > > > > > > > > > > I can make an exception for set_vq_cb(), save callbacks and
> > > > > > > > > > > > restore them afterwards. This looks too convoluted and maybe
> > > > > > > > > > > > we should seek another solution.
> > > > > > > > > > > I agree.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Let me know what you think.
> > > > > > > > > > > Rethink about this issue. It looks to the only issue we face
> > > > > > > > > > > is the set_vq_cb().
> > > > > > > > > > >
> > > > > > > > > > > With the assumption that the userspace can use the index
> > > > > > > > > > > correctly (even before set_features). I wonder the following works.
> > > > > > > > > > >
> > > > > > > > > > > Instead of checking whether the index is cvq in set_vq_cb() how
> > > > > > about:
> > > > > > > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and
> > > > > > > > > > > mlx5_congro_vq
> > > > > > > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > > > > > > 3) then we can do
> > > > > > > > > > >
> > > > > > > > > > > ndev->event_cbs[index] = *cb;
> > > > > > > > > > >
> > > > > > > > > > So actually you're suggesting to save all the callabck
> > > > > > > > > > configurations in an array and evaluate cvq index after feature
> > > > > > > > > > negotiation has been completed.
> > > > > > > > >
> > > > > > > > > Yes.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > I think that could work. I will code this and update.
> > > > > > > > It works fine when I am working with your version of qemu with
> > > > > > > > support for multi queue.
> > > > > > > >
> > > > > > > > The problem is that it is broken on qemu v6.0.0. If I register my
> > > > > > > > vdpa device with more than 2 data virtqueues, qemu won't even create
> > > > > > > > a netdevice in the VM.
> > > > > > Qemu should hide MQ feature in this case but looks like it doesn't.
> > > > > >
> > > > > > Will have a look.
> > > > > >
> > > > > > > > I am not sure how to handle this. Is there some kind of indication I
> > > > > > > > can get as to the version of qemu so I can avoid using multiqueue
> > > > > > > > for versions I know are problematic?
> > > > > > > No versions ;) This is what feature bits are for ...
> > > > > > Yes.
> > > > > >
> > > > > > So does it work if "mq=off" is specified in the command line?
> > > > > >
> > > > > We should not add driver module parameter.
> > > >
> > > >
> > > > Note that, it's not a module parameter but a qemu command line to disable mq
> > > > feature.
> > > >
> > > >
> > > > > We anyway need number of VQs to be driven by the number of vcpus used by the VM.
> > > > > So why not specify this when creating a vdpa device?
> > > >
> > > >
> > > > Yes, I think it should work as well.
> > > >
> > > > So management need either:
> > > >
> > > > 1) disable multiqueue via "mq=off"
> > > >
> > > > or
> > > >
> > > > 2) using netlink API to create a single queue pair device
> > > >
> > > > for the qemu <=6.1.
> > > >
> > >
> > > Which management entity are you referring to here?
> >
> > The one that launches Qemu. (E.g libvirt or other).
> >
>
> But we want to find a solution with existing packages. If modifying code
> existing code is the solution, we could fix qemu.

Right.

>
> As I reported in another email, mq=off avoids this problem. So users
> will have to use this when using new driver with old qemu.

Yes, so this is actually the option 1) above.

Thanks

>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > >
> > > > >
> > > >
> > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Tue, Aug 17, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Aug 17, 2021 at 2:01 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> > On Tue, Aug 17, 2021 at 01:48:17PM +0800, Jason Wang wrote:
> > > On Tue, Aug 17, 2021 at 1:26 PM Eli Cohen <elic@nvidia.com> wrote:
> > > >
> > > > On Tue, Aug 17, 2021 at 12:19:55PM +0800, Jason Wang wrote:
> > > > >
> > > > > 在 2021/8/17 下午12:03, Parav Pandit 写道:
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Tuesday, August 17, 2021 9:26 AM
> > > > > > >
> > > > > > > On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com>
> > > > > > > wrote:
> > > > > > > > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > > > > > > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > > > > > > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > > > > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > > > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen
> > > > > > > <elic@nvidia.com> wrote:
> > > > > > > > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang
> > > > > > > wrote:
> > > > > > > > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason
> > > > > > > Wang wrote:
> > > > > > > > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct
> > > > > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_dev *mvdev) {
> > > > > > > > > > > > > > > > > > > > > > > +     return 2 *
> > > > > > > > > > > > > > > > > > > > > > > +mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > > > > > > > We should handle the case when MQ is
> > > > > > > > > > > > > > > > > > > > > > supported by the device but not the driver.
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > There's some issue with this. I get
> > > > > > > > > > > > > > > > > > > > > callbacks targeting specific virtqueues before
> > > > > > > features negotiation has been completed.
> > > > > > > > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At
> > > > > > > > > > > > > > > > > > > > > this point I must know the control vq index.
> > > > > > > > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace
> > > > > > > > > > > > > > > > > > > > to use vq_index before feature is negotiated
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > (looks like a bug in my cvq series that will
> > > > > > > > > > > > > > > > > > > > call SET_VRING_CALL before feature is negotiate,
> > > > > > > which I will look).
> > > > > > > > > > > > > > > > > > > > 2) At the other hand, the driver should be
> > > > > > > > > > > > > > > > > > > > able to deal with that
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > All I can do is drop callbacks for VQs before
> > > > > > > > > > > > > > > > > > > features negotation has been completed.
> > > > > > > > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Right, will do.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > I think the CVQ index must not depend on the
> > > > > > > > > > > > > > > > > > > > > negotiated features but rather depend of the
> > > > > > > > > > > > > > > > > > > > > value the device driver provides in the call to
> > > > > > > _vdpa_register_device().
> > > > > > > > > > > > > > > > > > > > At the virtio level, it's too late to change
> > > > > > > > > > > > > > > > > > > > that and it breaks the backward compatibility.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > But at the vDPA level, the under layer device
> > > > > > > > > > > > > > > > > > > > can map virtio cvq to any of it's virtqueue.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > I am not following you here. I still don't know what
> > > > > > > index is cvq.
> > > > > > > > > > > > > > > > > > Right, we still need to wait for the feature being
> > > > > > > > > > > > > > > > > > negotiated in order to proceed.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > So to summarise, before feature negotiation
> > > > > > > > > > > > > > > > > complete, I accept calls referring to VQs only for indices 0
> > > > > > > and 1.
> > > > > > > > > > > > > > > > > After feature negotiation complete I know CVQ index
> > > > > > > > > > > > > > > > > and will accept indices 0 to cvq index.
> > > > > > > > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > > > > > > > What I meant to say is that there are several callbacks
> > > > > > > > > > > > > > > that refer to specific virtqueues, e.g.
> > > > > > > > > > > > > > > set_vq_address(), set_vq_num() etc. They all accept virtqueue
> > > > > > > index as an argument.
> > > > > > > > > > > > > > > What we want to do is verify wheather the index provided
> > > > > > > > > > > > > > > is valid or not. If it is not valid, either return error
> > > > > > > > > > > > > > > (if the callback can return a value) or just avoid
> > > > > > > > > > > > > > > processing it. If the index is valid then we process it normally.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Now we need to decide which index is valid or not. We
> > > > > > > > > > > > > > > need something like this to identifiy valid indexes range:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided
> > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > _vdpa_register_device()
> > > > > > > > > > > > > > Yes.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > Unfortunately it does not work.
> > > > > > > > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I can make an exception for set_vq_cb(), save callbacks and
> > > > > > > > > > > > > restore them afterwards. This looks too convoluted and maybe
> > > > > > > > > > > > > we should seek another solution.
> > > > > > > > > > > > I agree.
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > Let me know what you think.
> > > > > > > > > > > > Rethink about this issue. It looks to the only issue we face
> > > > > > > > > > > > is the set_vq_cb().
> > > > > > > > > > > >
> > > > > > > > > > > > With the assumption that the userspace can use the index
> > > > > > > > > > > > correctly (even before set_features). I wonder the following works.
> > > > > > > > > > > >
> > > > > > > > > > > > Instead of checking whether the index is cvq in set_vq_cb() how
> > > > > > > about:
> > > > > > > > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and
> > > > > > > > > > > > mlx5_congro_vq
> > > > > > > > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > > > > > > > 3) then we can do
> > > > > > > > > > > >
> > > > > > > > > > > > ndev->event_cbs[index] = *cb;
> > > > > > > > > > > >
> > > > > > > > > > > So actually you're suggesting to save all the callabck
> > > > > > > > > > > configurations in an array and evaluate cvq index after feature
> > > > > > > > > > > negotiation has been completed.
> > > > > > > > > >
> > > > > > > > > > Yes.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > I think that could work. I will code this and update.
> > > > > > > > > It works fine when I am working with your version of qemu with
> > > > > > > > > support for multi queue.
> > > > > > > > >
> > > > > > > > > The problem is that it is broken on qemu v6.0.0. If I register my
> > > > > > > > > vdpa device with more than 2 data virtqueues, qemu won't even create
> > > > > > > > > a netdevice in the VM.
> > > > > > > Qemu should hide MQ feature in this case but looks like it doesn't.
> > > > > > >
> > > > > > > Will have a look.
> > > > > > >
> > > > > > > > > I am not sure how to handle this. Is there some kind of indication I
> > > > > > > > > can get as to the version of qemu so I can avoid using multiqueue
> > > > > > > > > for versions I know are problematic?
> > > > > > > > No versions ;) This is what feature bits are for ...
> > > > > > > Yes.
> > > > > > >
> > > > > > > So does it work if "mq=off" is specified in the command line?
> > > > > > >
> > > > > > We should not add driver module parameter.
> > > > >
> > > > >
> > > > > Note that, it's not a module parameter but a qemu command line to disable mq
> > > > > feature.
> > > > >
> > > > >
> > > > > > We anyway need number of VQs to be driven by the number of vcpus used by the VM.
> > > > > > So why not specify this when creating a vdpa device?
> > > > >
> > > > >
> > > > > Yes, I think it should work as well.
> > > > >
> > > > > So management need either:
> > > > >
> > > > > 1) disable multiqueue via "mq=off"
> > > > >
> > > > > or
> > > > >
> > > > > 2) using netlink API to create a single queue pair device
> > > > >
> > > > > for the qemu <=6.1.
> > > > >
> > > >
> > > > Which management entity are you referring to here?
> > >
> > > The one that launches Qemu. (E.g libvirt or other).
> > >
> >
> > But we want to find a solution with existing packages. If modifying code
> > existing code is the solution, we could fix qemu.
>
> Right.
>
> >
> > As I reported in another email, mq=off avoids this problem. So users
> > will have to use this when using new driver with old qemu.
>
> Yes, so this is actually the option 1) above.

Note that actually, mq=off is the default option. So we are probably fine here.

Thanks

>
> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > >
> > > > >
> > > >
> > >
> >

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

在 2021/8/17 上午11:55, Jason Wang 写道:
>>>>> I think that could work. I will code this and update.
>>> It works fine when I am working with your version of qemu with support
>>> for multi queue.
>>>
>>> The problem is that it is broken on qemu v6.0.0. If I register my vdpa
>>> device with more than 2 data virtqueues, qemu won't even create a
>>> netdevice in the VM.
> Qemu should hide MQ feature in this case but looks like it doesn't.
>
> Will have a look.


So it looks to me current Qemu code doesn't take care about the case 
when a feature (e.g MQ):

1) supported by the vhost
2) enabled via the command line
3) but not supported by the Qemu

Not sure it's worth to bother.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Patch v1 3/3] vdpa/mlx5: Add multiqueue support

[Jason Wang]

On Tue, Aug 17, 2021 at 1:24 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Tue, Aug 17, 2021 at 11:55:42AM +0800, Jason Wang wrote:
> > On Tue, Aug 17, 2021 at 3:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Aug 16, 2021 at 07:56:59PM +0300, Eli Cohen wrote:
> > > > On Mon, Aug 16, 2021 at 01:58:06PM +0800, Jason Wang wrote:
> > > > >
> > > > > 在 2021/8/16 下午1:47, Eli Cohen 写道:
> > > > > > On Mon, Aug 16, 2021 at 12:16:14PM +0800, Jason Wang wrote:
> > > > > > > 在 2021/8/12 下午5:50, Eli Cohen 写道:
> > > > > > > > On Thu, Aug 12, 2021 at 03:04:35PM +0800, Jason Wang wrote:
> > > > > > > > > 在 2021/8/12 下午3:01, Eli Cohen 写道:
> > > > > > > > > > On Thu, Aug 12, 2021 at 02:47:06PM +0800, Jason Wang wrote:
> > > > > > > > > > > On Thu, Aug 12, 2021 at 12:55 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > > > > > On Thu, Aug 12, 2021 at 11:19:19AM +0800, Jason Wang wrote:
> > > > > > > > > > > > > 在 2021/8/11 下午7:04, Eli Cohen 写道:
> > > > > > > > > > > > > > On Wed, Aug 11, 2021 at 04:37:44PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > > > 在 2021/8/11 下午3:53, Eli Cohen 写道:
> > > > > > > > > > > > > > > > > One thing need to solve for mq is that the:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > +static u16 ctrl_vq_idx(struct  mlx5_vdpa_dev *mvdev)
> > > > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > > > +     return 2 *  mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > > We should handle the case when MQ is supported by the device but not the
> > > > > > > > > > > > > > > > > driver.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > E.g in the case when we have 2 queue pairs:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > When MQ is enabled, cvq is queue 4
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > When MQ is not enabled, cvq is queue 2
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > There's some issue with this. I get callbacks targeting specific
> > > > > > > > > > > > > > > > virtqueues before features negotiation has been completed.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Specifically, I get set_vq_cb() calls. At this point I must know the
> > > > > > > > > > > > > > > > control vq index.
> > > > > > > > > > > > > > > So I think we need do both:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 1) At one hand, it's a bug for the userspace to use vq_index before feature
> > > > > > > > > > > > > > > is negotiated
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > (looks like a bug in my cvq series that will call SET_VRING_CALL before
> > > > > > > > > > > > > > > feature is negotiate, which I will look).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 2) At the other hand, the driver should be able to deal with that
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > All I can do is drop callbacks for VQs before features negotation has
> > > > > > > > > > > > > > been completed.
> > > > > > > > > > > > > Or just leave queue index 0, 1 work.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Since it is not expected to be changed.
> > > > > > > > > > > > >
> > > > > > > > > > > > Right, will do.
> > > > > > > > > > > >
> > > > > > > > > > > > > > > > I think the CVQ index must not depend on the negotiated features but
> > > > > > > > > > > > > > > > rather depend of the value the device driver provides in the call to
> > > > > > > > > > > > > > > > _vdpa_register_device().
> > > > > > > > > > > > > > > At the virtio level, it's too late to change that and it breaks the backward
> > > > > > > > > > > > > > > compatibility.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > But at the vDPA level, the under layer device can map virtio cvq to any of
> > > > > > > > > > > > > > > it's virtqueue.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > E.g map cvq (index 2) to mlx5 cvq (the last).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > I am not following you here. I still don't know what index is cvq.
> > > > > > > > > > > > > Right, we still need to wait for the feature being negotiated in order to
> > > > > > > > > > > > > proceed.
> > > > > > > > > > > > >
> > > > > > > > > > > > So to summarise, before feature negotiation complete, I accept calls
> > > > > > > > > > > > referring to VQs only for indices 0 and 1.
> > > > > > > > > > > > After feature negotiation complete I know CVQ index and will accept
> > > > > > > > > > > > indices 0 to cvq index.
> > > > > > > > > > > I don't get this "accept indices 0 to cvq index".
> > > > > > > > > > What I meant to say is that there are several callbacks that refer to
> > > > > > > > > > specific virtqueues, e.g. set_vq_address(), set_vq_num() etc. They all
> > > > > > > > > > accept virtqueue index as an argument.
> > > > > > > > > >
> > > > > > > > > > What we want to do is verify wheather the index provided is valid or
> > > > > > > > > > not. If it is not valid, either return error (if the callback can return
> > > > > > > > > > a value) or just avoid processing it. If the index is valid then we
> > > > > > > > > > process it normally.
> > > > > > > > > >
> > > > > > > > > > Now we need to decide which index is valid or not. We need something
> > > > > > > > > > like this to identifiy valid indexes range:
> > > > > > > > > >
> > > > > > > > > > CVQ clear: 0 and 1
> > > > > > > > > > CVQ set, MQ clear: 0, 1 and 2 (for CVQ).
> > > > > > > > > > CVQ set, MQ set: 0..nvq where nvq is whatever provided to
> > > > > > > > > > _vdpa_register_device()
> > > > > > > > > Yes.
> > > > > > > > >
> > > > > > > > Unfortunately it does not work.
> > > > > > > > set_vq_cb() for all the multiqueues is called beofre feature
> > > > > > > > negotiation. If I apply the above logic, I will lose these settings.
> > > > > > > >
> > > > > > > > I can make an exception for set_vq_cb(), save callbacks and restore
> > > > > > > > them afterwards. This looks too convoluted and maybe we should seek
> > > > > > > > another solution.
> > > > > > >
> > > > > > > I agree.
> > > > > > >
> > > > > > >
> > > > > > > > Let me know what you think.
> > > > > > >
> > > > > > > Rethink about this issue. It looks to the only issue we face is the
> > > > > > > set_vq_cb().
> > > > > > >
> > > > > > > With the assumption that the userspace can use the index correctly (even
> > > > > > > before set_features). I wonder the following works.
> > > > > > >
> > > > > > > Instead of checking whether the index is cvq in set_vq_cb() how about:
> > > > > > >
> > > > > > > 1) decouple event_cb out of mlx5_vdpa_virtqueue and mlx5_congro_vq
> > > > > > > 2) have a dedicated event_cb array in mlx5_vdpa_net
> > > > > > > 3) then we can do
> > > > > > >
> > > > > > > ndev->event_cbs[index] = *cb;
> > > > > > >
> > > > > > So actually you're suggesting to save all the callabck configurations in
> > > > > > an array and evaluate cvq index after feature negotiation has been
> > > > > > completed.
> > > > >
> > > > >
> > > > > Yes.
> > > > >
> > > > >
> > > > > >
> > > > > > I think that could work. I will code this and update.
> > > > >
> > > >
> > > > It works fine when I am working with your version of qemu with support
> > > > for multi queue.
> > > >
> > > > The problem is that it is broken on qemu v6.0.0. If I register my vdpa
> > > > device with more than 2 data virtqueues, qemu won't even create a
> > > > netdevice in the VM.
> >
> > Qemu should hide MQ feature in this case but looks like it doesn't.
> >
>
> Not sure I understand what do you think is expected from qemu. The
> device publishes MQ, qemu should either accept or decline.

Probably not. It works like disable MQ silently (without cli) in qemu.

Thanks

>
> > Will have a look.
> >
> > > >
> > > > I am not sure how to handle this. Is there some kind of indication I can
> > > > get as to the version of qemu so I can avoid using multiqueue for
> > > > versions I know are problematic?
> > >
> > > No versions ;) This is what feature bits are for ...
> >
> > Yes.
> >
> > So does it work if "mq=off" is specified in the command line?
> >
> > Thanks
> >
> >
> > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > > >
> > > > > > > in mlx5_vdpa_set_vq_cb()
> > > > > > >
> > > > > > > 4) in the mlx5_cvq_kick_handler(), we know the feature is negotiated and we
> > > > > > > can use the correct index there.
> > > > > > >
> > > > > > > In the mean time, I will look at Qemu code to see if we can guarantee that
> > > > > > > set_features is called before set_vq_callback. (At first glance, it's not
> > > > > > > trivial but let's see).
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > > > > And while writing this, I think this logic does not belog in mlx5_vdpa
> > > > > > > > > > but probably in vdpa.c
> > > > > > > > > The problem is that vdpa should be unaware of a specific device type. E.g
> > > > > > > > > the above indices may work only for virtio-net but not other.
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > > > > Thanks
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thanks
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > >
> > >
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization