All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status
       [not found] ` <20220111183400.38418-2-elic@nvidia.com>
@ 2022-01-11 21:46   ` Si-Wei Liu
  2022-01-12  2:32   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-11 21:46 UTC (permalink / raw)
  To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma



On 1/11/2022 10:33 AM, Eli Cohen wrote:
> Avoid the wrapper holding cf_mutex since it is not protecting anything.
> To avoid confusion and unnecessary overhead incurred by it, remove.
>
> Fixes: f489f27bc0ab ("vdpa: Sync calls set/get config/status with cf_mutex")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/vdpa.c  | 11 -----------
>   drivers/vhost/vdpa.c |  5 +++--
>   include/linux/vdpa.h |  1 -
>   3 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 4380367d00b5..9846c9de4bfa 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -21,17 +21,6 @@ static LIST_HEAD(mdev_head);
>   static DEFINE_MUTEX(vdpa_dev_mutex);
>   static DEFINE_IDA(vdpa_index_ida);
>   
> -u8 vdpa_get_status(struct vdpa_device *vdev)
> -{
> -	u8 status;
> -
> -	mutex_lock(&vdev->cf_mutex);
> -	status = vdev->config->get_status(vdev);
> -	mutex_unlock(&vdev->cf_mutex);
> -	return status;
> -}
> -EXPORT_SYMBOL(vdpa_get_status);
> -
>   void vdpa_set_status(struct vdpa_device *vdev, u8 status)
>   {
>   	mutex_lock(&vdev->cf_mutex);
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..0ed6cbadb52d 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -142,9 +142,10 @@ static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
>   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>   {
>   	struct vdpa_device *vdpa = v->vdpa;
> +	const struct vdpa_config_ops *ops = vdpa->config;
>   	u8 status;
>   
> -	status = vdpa_get_status(vdpa);
> +	status = ops->get_status(vdpa);
>   
>   	if (copy_to_user(statusp, &status, sizeof(status)))
>   		return -EFAULT;
> @@ -163,7 +164,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   	if (copy_from_user(&status, statusp, sizeof(status)))
>   		return -EFAULT;
>   
> -	status_old = vdpa_get_status(vdpa);
> +	status_old = ops->get_status(vdpa);
>   
>   	/*
>   	 * Userspace shouldn't remove status bits unless reset the
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index a6047fd6cf12..2de442ececae 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -421,7 +421,6 @@ void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>   		     void *buf, unsigned int len);
>   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
>   		     const void *buf, unsigned int length);
> -u8 vdpa_get_status(struct vdpa_device *vdev);
>   void vdpa_set_status(struct vdpa_device *vdev, u8 status);
>   
>   /**

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex
       [not found] ` <20220111183400.38418-3-elic@nvidia.com>
@ 2022-01-11 21:47   ` Si-Wei Liu
  2022-01-12  2:34   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-11 21:47 UTC (permalink / raw)
  To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma



On 1/11/2022 10:33 AM, Eli Cohen wrote:
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
>
> This comes to protect from the following possible scenario:
>
> vhost_vdpa_set_status() could call the reset op. Since the call is not
> protected by cf_mutex, a netlink thread calling vdpa_dev_config_fill
> could get passed the VIRTIO_CONFIG_S_FEATURES_OK check in
> vdpa_dev_config_fill() and end up reporting wrong features.
>
> Fixes: 5f6e85953d8f ("vdpa: Read device configuration only if FEATURES_OK")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> ---
>   drivers/vhost/vdpa.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 0ed6cbadb52d..851539807bc9 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -178,7 +178,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>   			vhost_vdpa_unsetup_vq_irq(v, i);
>   
>   	if (status == 0) {
> -		ret = ops->reset(vdpa);
> +		ret = vdpa_reset(vdpa);
>   		if (ret)
>   			return ret;
>   	} else

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features
       [not found] ` <20220111183400.38418-4-elic@nvidia.com>
@ 2022-01-11 21:47   ` Si-Wei Liu
  2022-01-12  2:35   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-11 21:47 UTC (permalink / raw)
  To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma



On 1/11/2022 10:33 AM, Eli Cohen wrote:
> Make sure the decision whether an index received through a callback is
> valid or not consults the negotiated features.
>
> The motivation for this was due to a case encountered where I shut down
> the VM. After the reset operation was called features were already
> clear, I got get_vq_state() call which caused out array bounds
> access since is_index_valid() reported the index value.
>
> So this is more of not hit a bug since the call shouldn't have been made
> first place.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d1ff65065fb1..9eacfdb48434 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -133,10 +133,14 @@ struct mlx5_vdpa_virtqueue {
>   
>   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>   {
> -	if (unlikely(idx > mvdev->max_idx))
> -		return false;
> +	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> +		if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> +			return idx < 2;
> +		else
> +			return idx < 3;
> +	}
>   
> -	return true;
> +	return idx <= mvdev->max_idx;
>   }
>   
>   struct mlx5_vdpa_net {

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
       [not found] ` <20220111183400.38418-5-elic@nvidia.com>
@ 2022-01-11 22:14   ` Si-Wei Liu
  2022-01-12  2:29     ` Jason Wang
                       ` (2 more replies)
  2022-01-12  2:35   ` Jason Wang
  1 sibling, 3 replies; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-11 22:14 UTC (permalink / raw)
  To: Eli Cohen, mst, jasowang, virtualization; +Cc: lvivier, eperezma



On 1/11/2022 10:34 AM, Eli Cohen wrote:
> Modify the code such that ndev->cur_num_vqs better reflects the actual
> number of data virtqueues. The value can be accurately realized after
> features have been negotiated.
>
> This is to prevent possible failures when modifying the RQT object if
> the cur_num_vqs bears invalid value.
>
> No issue was actually encountered but this also makes the code more
> readable.
>
> Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 9eacfdb48434..b53603d94082 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>   		num = 1;
>   	else
> -		num = mlx5vdpa16_to_cpu(&ndev->mvdev,
> -					ndev->config.max_virtqueue_pairs);
> +		num = ndev->cur_num_vqs / 2;
Nit: the if branch can be consolidated

>   
>   	max_rqt = min_t(int, roundup_pow_of_two(num),
>   			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>   		return err;
>   
>   	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> +	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> +		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
Hmmm, not this patch, but there should've been validation done in the 
upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes 
with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ) 
|  BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?

otherwise it looks good to me.

Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> +	else
> +		ndev->cur_num_vqs = 2;
> +
>   	update_cvq_info(mvdev);
>   	return err;
>   }
> @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	clear_vqs_ready(ndev);
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
> +	ndev->cur_num_vqs = 0;
>   	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>   	ndev->mvdev.actual_features = 0;
>   	++mvdev->generation;
> @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   
>   	ndev->nb.notifier_call = event_handler;
>   	mlx5_notifier_register(mdev, &ndev->nb);
> -	ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>   	mvdev->vdev.mdev = &mgtdev->mgtdev;
> -	err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
> +	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>   	if (err)
>   		goto err_reg;
>   

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
  2022-01-11 22:14   ` [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs Si-Wei Liu
@ 2022-01-12  2:29     ` Jason Wang
  2022-01-12  3:12       ` Si-Wei Liu
       [not found]     ` <20220112063732.GA196570@mtl-vdi-166.wap.labs.mlnx>
  2022-01-14 20:25     ` Michael S. Tsirkin
  2 siblings, 1 reply; 13+ messages in thread
From: Jason Wang @ 2022-01-12  2:29 UTC (permalink / raw)
  To: Si-Wei Liu; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen

On Wed, Jan 12, 2022 at 6:15 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 1/11/2022 10:34 AM, Eli Cohen wrote:
> > Modify the code such that ndev->cur_num_vqs better reflects the actual
> > number of data virtqueues. The value can be accurately realized after
> > features have been negotiated.
> >
> > This is to prevent possible failures when modifying the RQT object if
> > the cur_num_vqs bears invalid value.
> >
> > No issue was actually encountered but this also makes the code more
> > readable.
> >
> > Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 9eacfdb48434..b53603d94082 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> >       if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >               num = 1;
> >       else
> > -             num = mlx5vdpa16_to_cpu(&ndev->mvdev,
> > -                                     ndev->config.max_virtqueue_pairs);
> > +             num = ndev->cur_num_vqs / 2;
> Nit: the if branch can be consolidated
>
> >
> >       max_rqt = min_t(int, roundup_pow_of_two(num),
> >                       1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >               return err;
> >
> >       ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> > +     if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> > +             ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> Hmmm, not this patch, but there should've been validation done in the
> upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes
> with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ)
> |  BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?

So the upper layer is unaware of the device type. It's better to do
that in mlx5's set_features() according to the spec:

The device MUST NOT offer a feature which requires another feature
which was not offered.

Thanks

>
> otherwise it looks good to me.
>
> Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> > +     else
> > +             ndev->cur_num_vqs = 2;
> > +
> >       update_cvq_info(mvdev);
> >       return err;
> >   }
> > @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> >       clear_vqs_ready(ndev);
> >       mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >       ndev->mvdev.status = 0;
> > +     ndev->cur_num_vqs = 0;
> >       memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
> >       ndev->mvdev.actual_features = 0;
> >       ++mvdev->generation;
> > @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >
> >       ndev->nb.notifier_call = event_handler;
> >       mlx5_notifier_register(mdev, &ndev->nb);
> > -     ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
> >       mvdev->vdev.mdev = &mgtdev->mgtdev;
> > -     err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
> > +     err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> >       if (err)
> >               goto err_reg;
> >
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status
       [not found] ` <20220111183400.38418-2-elic@nvidia.com>
  2022-01-11 21:46   ` [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status Si-Wei Liu
@ 2022-01-12  2:32   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-01-12  2:32 UTC (permalink / raw)
  To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu

On Wed, Jan 12, 2022 at 2:34 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Avoid the wrapper holding cf_mutex since it is not protecting anything.
> To avoid confusion and unnecessary overhead incurred by it, remove.
>
> Fixes: f489f27bc0ab ("vdpa: Sync calls set/get config/status with cf_mutex")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

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

> ---
>  drivers/vdpa/vdpa.c  | 11 -----------
>  drivers/vhost/vdpa.c |  5 +++--
>  include/linux/vdpa.h |  1 -
>  3 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 4380367d00b5..9846c9de4bfa 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -21,17 +21,6 @@ static LIST_HEAD(mdev_head);
>  static DEFINE_MUTEX(vdpa_dev_mutex);
>  static DEFINE_IDA(vdpa_index_ida);
>
> -u8 vdpa_get_status(struct vdpa_device *vdev)
> -{
> -       u8 status;
> -
> -       mutex_lock(&vdev->cf_mutex);
> -       status = vdev->config->get_status(vdev);
> -       mutex_unlock(&vdev->cf_mutex);
> -       return status;
> -}
> -EXPORT_SYMBOL(vdpa_get_status);
> -
>  void vdpa_set_status(struct vdpa_device *vdev, u8 status)
>  {
>         mutex_lock(&vdev->cf_mutex);
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6e7edaf2472b..0ed6cbadb52d 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -142,9 +142,10 @@ static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
>  static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
>  {
>         struct vdpa_device *vdpa = v->vdpa;
> +       const struct vdpa_config_ops *ops = vdpa->config;
>         u8 status;
>
> -       status = vdpa_get_status(vdpa);
> +       status = ops->get_status(vdpa);
>
>         if (copy_to_user(statusp, &status, sizeof(status)))
>                 return -EFAULT;
> @@ -163,7 +164,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>         if (copy_from_user(&status, statusp, sizeof(status)))
>                 return -EFAULT;
>
> -       status_old = vdpa_get_status(vdpa);
> +       status_old = ops->get_status(vdpa);
>
>         /*
>          * Userspace shouldn't remove status bits unless reset the
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index a6047fd6cf12..2de442ececae 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -421,7 +421,6 @@ void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>                      void *buf, unsigned int len);
>  void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
>                      const void *buf, unsigned int length);
> -u8 vdpa_get_status(struct vdpa_device *vdev);
>  void vdpa_set_status(struct vdpa_device *vdev, u8 status);
>
>  /**
> --
> 2.34.1
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex
       [not found] ` <20220111183400.38418-3-elic@nvidia.com>
  2022-01-11 21:47   ` [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex Si-Wei Liu
@ 2022-01-12  2:34   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-01-12  2:34 UTC (permalink / raw)
  To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu

On Wed, Jan 12, 2022 at 2:34 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Call reset using the wrapper function vdpa_reset() to make sure the
> operation is serialized with cf_mutex.
>
> This comes to protect from the following possible scenario:
>
> vhost_vdpa_set_status() could call the reset op. Since the call is not
> protected by cf_mutex, a netlink thread calling vdpa_dev_config_fill
> could get passed the VIRTIO_CONFIG_S_FEATURES_OK check in
> vdpa_dev_config_fill() and end up reporting wrong features.
>
> Fixes: 5f6e85953d8f ("vdpa: Read device configuration only if FEATURES_OK")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

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

> ---
>  drivers/vhost/vdpa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 0ed6cbadb52d..851539807bc9 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -178,7 +178,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
>                         vhost_vdpa_unsetup_vq_irq(v, i);
>
>         if (status == 0) {
> -               ret = ops->reset(vdpa);
> +               ret = vdpa_reset(vdpa);
>                 if (ret)
>                         return ret;
>         } else
> --
> 2.34.1
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features
       [not found] ` <20220111183400.38418-4-elic@nvidia.com>
  2022-01-11 21:47   ` [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features Si-Wei Liu
@ 2022-01-12  2:35   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-01-12  2:35 UTC (permalink / raw)
  To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu

On Wed, Jan 12, 2022 at 2:34 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Make sure the decision whether an index received through a callback is
> valid or not consults the negotiated features.
>
> The motivation for this was due to a case encountered where I shut down
> the VM. After the reset operation was called features were already
> clear, I got get_vq_state() call which caused out array bounds
> access since is_index_valid() reported the index value.
>
> So this is more of not hit a bug since the call shouldn't have been made
> first place.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>

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

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d1ff65065fb1..9eacfdb48434 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -133,10 +133,14 @@ struct mlx5_vdpa_virtqueue {
>
>  static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>  {
> -       if (unlikely(idx > mvdev->max_idx))
> -               return false;
> +       if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> +               if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> +                       return idx < 2;
> +               else
> +                       return idx < 3;
> +       }
>
> -       return true;
> +       return idx <= mvdev->max_idx;
>  }
>
>  struct mlx5_vdpa_net {
> --
> 2.34.1
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
       [not found] ` <20220111183400.38418-5-elic@nvidia.com>
  2022-01-11 22:14   ` [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs Si-Wei Liu
@ 2022-01-12  2:35   ` Jason Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-01-12  2:35 UTC (permalink / raw)
  To: Eli Cohen; +Cc: Laurent Vivier, mst, virtualization, eperezma, Si-Wei Liu

On Wed, Jan 12, 2022 at 2:34 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Modify the code such that ndev->cur_num_vqs better reflects the actual
> number of data virtqueues. The value can be accurately realized after
> features have been negotiated.
>
> This is to prevent possible failures when modifying the RQT object if
> the cur_num_vqs bears invalid value.
>
> No issue was actually encountered but this also makes the code more
> readable.
>
> Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

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

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 9eacfdb48434..b53603d94082 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>         if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>                 num = 1;
>         else
> -               num = mlx5vdpa16_to_cpu(&ndev->mvdev,
> -                                       ndev->config.max_virtqueue_pairs);
> +               num = ndev->cur_num_vqs / 2;
>
>         max_rqt = min_t(int, roundup_pow_of_two(num),
>                         1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>                 return err;
>
>         ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> +       if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> +               ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> +       else
> +               ndev->cur_num_vqs = 2;
> +
>         update_cvq_info(mvdev);
>         return err;
>  }
> @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>         clear_vqs_ready(ndev);
>         mlx5_vdpa_destroy_mr(&ndev->mvdev);
>         ndev->mvdev.status = 0;
> +       ndev->cur_num_vqs = 0;
>         memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>         ndev->mvdev.actual_features = 0;
>         ++mvdev->generation;
> @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>
>         ndev->nb.notifier_call = event_handler;
>         mlx5_notifier_register(mdev, &ndev->nb);
> -       ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>         mvdev->vdev.mdev = &mgtdev->mgtdev;
> -       err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
> +       err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>         if (err)
>                 goto err_reg;
>
> --
> 2.34.1
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
  2022-01-12  2:29     ` Jason Wang
@ 2022-01-12  3:12       ` Si-Wei Liu
  2022-01-12  3:57         ` Jason Wang
  0 siblings, 1 reply; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-12  3:12 UTC (permalink / raw)
  To: Jason Wang; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen



On 1/11/2022 6:29 PM, Jason Wang wrote:
> On Wed, Jan 12, 2022 at 6:15 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 1/11/2022 10:34 AM, Eli Cohen wrote:
>>> Modify the code such that ndev->cur_num_vqs better reflects the actual
>>> number of data virtqueues. The value can be accurately realized after
>>> features have been negotiated.
>>>
>>> This is to prevent possible failures when modifying the RQT object if
>>> the cur_num_vqs bears invalid value.
>>>
>>> No issue was actually encountered but this also makes the code more
>>> readable.
>>>
>>> Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 9eacfdb48434..b53603d94082 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>        if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>                num = 1;
>>>        else
>>> -             num = mlx5vdpa16_to_cpu(&ndev->mvdev,
>>> -                                     ndev->config.max_virtqueue_pairs);
>>> +             num = ndev->cur_num_vqs / 2;
>> Nit: the if branch can be consolidated
>>
>>>        max_rqt = min_t(int, roundup_pow_of_two(num),
>>>                        1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>> @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>                return err;
>>>
>>>        ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>>> +     if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>> +             ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>> Hmmm, not this patch, but there should've been validation done in the
>> upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes
>> with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ)
>> |  BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?
> So the upper layer is unaware of the device type. It's better to do
> that in mlx5's set_features()
That'll be fine. I thought the upper layer can be made device type aware 
and consolidate it to common library routines avoiding duplicated code 
in every individual driver of the same type. If this is against the goal 
of making vdpa core device type agnostic, then it's perhaps not needed.

-Siwei

> according to the spec:
>
> The device MUST NOT offer a feature which requires another feature
> which was not offered.
>
> Thanks
>
>> otherwise it looks good to me.
>>
>> Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
>>> +     else
>>> +             ndev->cur_num_vqs = 2;
>>> +
>>>        update_cvq_info(mvdev);
>>>        return err;
>>>    }
>>> @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>>>        clear_vqs_ready(ndev);
>>>        mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>>        ndev->mvdev.status = 0;
>>> +     ndev->cur_num_vqs = 0;
>>>        memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>>>        ndev->mvdev.actual_features = 0;
>>>        ++mvdev->generation;
>>> @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>
>>>        ndev->nb.notifier_call = event_handler;
>>>        mlx5_notifier_register(mdev, &ndev->nb);
>>> -     ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>>>        mvdev->vdev.mdev = &mgtdev->mgtdev;
>>> -     err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
>>> +     err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>>>        if (err)
>>>                goto err_reg;
>>>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
  2022-01-12  3:12       ` Si-Wei Liu
@ 2022-01-12  3:57         ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-01-12  3:57 UTC (permalink / raw)
  To: Si-Wei Liu; +Cc: Laurent Vivier, mst, virtualization, eperezma, Eli Cohen

On Wed, Jan 12, 2022 at 11:12 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 1/11/2022 6:29 PM, Jason Wang wrote:
> > On Wed, Jan 12, 2022 at 6:15 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 1/11/2022 10:34 AM, Eli Cohen wrote:
> >>> Modify the code such that ndev->cur_num_vqs better reflects the actual
> >>> number of data virtqueues. The value can be accurately realized after
> >>> features have been negotiated.
> >>>
> >>> This is to prevent possible failures when modifying the RQT object if
> >>> the cur_num_vqs bears invalid value.
> >>>
> >>> No issue was actually encountered but this also makes the code more
> >>> readable.
> >>>
> >>> Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
> >>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>> ---
> >>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
> >>>    1 file changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> index 9eacfdb48434..b53603d94082 100644
> >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> >>>        if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >>>                num = 1;
> >>>        else
> >>> -             num = mlx5vdpa16_to_cpu(&ndev->mvdev,
> >>> -                                     ndev->config.max_virtqueue_pairs);
> >>> +             num = ndev->cur_num_vqs / 2;
> >> Nit: the if branch can be consolidated
> >>
> >>>        max_rqt = min_t(int, roundup_pow_of_two(num),
> >>>                        1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> >>> @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >>>                return err;
> >>>
> >>>        ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> >>> +     if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> >>> +             ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> >> Hmmm, not this patch, but there should've been validation done in the
> >> upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes
> >> with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ)
> >> |  BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?
> > So the upper layer is unaware of the device type. It's better to do
> > that in mlx5's set_features()
> That'll be fine. I thought the upper layer can be made device type aware
> and consolidate it to common library routines avoiding duplicated code
> in every individual driver of the same type.

This is possible but not implemented so far. It looks more like an
intermediate layer between vdpa and it's parent.

> If this is against the goal
> of making vdpa core device type agnostic, then it's perhaps not needed.

Yes, that's the goal when developing vDPA core.

Thanks

>
> -Siwei
>
> > according to the spec:
> >
> > The device MUST NOT offer a feature which requires another feature
> > which was not offered.
> >
> > Thanks
> >
> >> otherwise it looks good to me.
> >>
> >> Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
> >>> +     else
> >>> +             ndev->cur_num_vqs = 2;
> >>> +
> >>>        update_cvq_info(mvdev);
> >>>        return err;
> >>>    }
> >>> @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> >>>        clear_vqs_ready(ndev);
> >>>        mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >>>        ndev->mvdev.status = 0;
> >>> +     ndev->cur_num_vqs = 0;
> >>>        memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
> >>>        ndev->mvdev.actual_features = 0;
> >>>        ++mvdev->generation;
> >>> @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >>>
> >>>        ndev->nb.notifier_call = event_handler;
> >>>        mlx5_notifier_register(mdev, &ndev->nb);
> >>> -     ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
> >>>        mvdev->vdev.mdev = &mgtdev->mgtdev;
> >>> -     err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
> >>> +     err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> >>>        if (err)
> >>>                goto err_reg;
> >>>
>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
       [not found]     ` <20220112063732.GA196570@mtl-vdi-166.wap.labs.mlnx>
@ 2022-01-12  7:46       ` Si-Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Si-Wei Liu @ 2022-01-12  7:46 UTC (permalink / raw)
  To: Eli Cohen; +Cc: lvivier, mst, virtualization, eperezma



On 1/11/2022 10:37 PM, Eli Cohen wrote:
> On Tue, Jan 11, 2022 at 02:14:47PM -0800, Si-Wei Liu wrote:
>>
>> On 1/11/2022 10:34 AM, Eli Cohen wrote:
>>> Modify the code such that ndev->cur_num_vqs better reflects the actual
>>> number of data virtqueues. The value can be accurately realized after
>>> features have been negotiated.
>>>
>>> This is to prevent possible failures when modifying the RQT object if
>>> the cur_num_vqs bears invalid value.
>>>
>>> No issue was actually encountered but this also makes the code more
>>> readable.
>>>
>>> Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue")
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 9eacfdb48434..b53603d94082 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>    	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>    		num = 1;
>>>    	else
>>> -		num = mlx5vdpa16_to_cpu(&ndev->mvdev,
>>> -					ndev->config.max_virtqueue_pairs);
>>> +		num = ndev->cur_num_vqs / 2;
>> Nit: the if branch can be consolidated
>>
>>>    	max_rqt = min_t(int, roundup_pow_of_two(num),
>>>    			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>> @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>    		return err;
>>>    	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>>> +	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>> +		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>> Hmmm, not this patch, but there should've been validation done in the upper
>> layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes with
>> VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ) |
>> BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?
> Not sure I follow. It's valid to have only CTRL_VQ feature but not MQ.
> In that case we should have only two data VQs but then cur_num_vqs would
> be set to whatever was configured by the user which could more than 2.
In that case (CTRL_VQ && !MQ), it will end up with just two data VQs 
(plus one control queue which is not accounted in 'ethtool -l') 
negotiated. There should be some validation in handle_ctrl_mq to 
disallow user from configuring more than 2 data VQs for that case. You 
don't need to worry too much for that at the moment, I'll post a patch  
following your series to fix it. Let's keep it simple for just checking 
both BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ) together.

Thanks,
-Siwei

>
>> otherwise it looks good to me.
>>
>> Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>
>>> +	else
>>> +		ndev->cur_num_vqs = 2;
>>> +
>>>    	update_cvq_info(mvdev);
>>>    	return err;
>>>    }
>>> @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>>>    	clear_vqs_ready(ndev);
>>>    	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>>    	ndev->mvdev.status = 0;
>>> +	ndev->cur_num_vqs = 0;
>>>    	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>>>    	ndev->mvdev.actual_features = 0;
>>>    	++mvdev->generation;
>>> @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>    	ndev->nb.notifier_call = event_handler;
>>>    	mlx5_notifier_register(mdev, &ndev->nb);
>>> -	ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs);
>>>    	mvdev->vdev.mdev = &mgtdev->mgtdev;
>>> -	err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1);
>>> +	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>>>    	if (err)
>>>    		goto err_reg;

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
  2022-01-11 22:14   ` [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs Si-Wei Liu
  2022-01-12  2:29     ` Jason Wang
       [not found]     ` <20220112063732.GA196570@mtl-vdi-166.wap.labs.mlnx>
@ 2022-01-14 20:25     ` Michael S. Tsirkin
  2 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 20:25 UTC (permalink / raw)
  To: Si-Wei Liu; +Cc: lvivier, virtualization, eperezma, Eli Cohen

On Tue, Jan 11, 2022 at 02:14:47PM -0800, Si-Wei Liu wrote:
> Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>

Note: the correct format is:

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>

(with a space before <>).

Pls take care in the future.
Thanks!

-- 
MST

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-01-14 20:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220111183400.38418-1-elic@nvidia.com>
     [not found] ` <20220111183400.38418-2-elic@nvidia.com>
2022-01-11 21:46   ` [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status Si-Wei Liu
2022-01-12  2:32   ` Jason Wang
     [not found] ` <20220111183400.38418-3-elic@nvidia.com>
2022-01-11 21:47   ` [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex Si-Wei Liu
2022-01-12  2:34   ` Jason Wang
     [not found] ` <20220111183400.38418-4-elic@nvidia.com>
2022-01-11 21:47   ` [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features Si-Wei Liu
2022-01-12  2:35   ` Jason Wang
     [not found] ` <20220111183400.38418-5-elic@nvidia.com>
2022-01-11 22:14   ` [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs Si-Wei Liu
2022-01-12  2:29     ` Jason Wang
2022-01-12  3:12       ` Si-Wei Liu
2022-01-12  3:57         ` Jason Wang
     [not found]     ` <20220112063732.GA196570@mtl-vdi-166.wap.labs.mlnx>
2022-01-12  7:46       ` Si-Wei Liu
2022-01-14 20:25     ` Michael S. Tsirkin
2022-01-12  2:35   ` Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.