Re: [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status

Re: [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status

[Si-Wei Liu]

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

Re: [PATCH 1/4] vdpa: Avoid taking cf_mutex lock on get status

[Jason Wang]

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

Re: [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex

[Si-Wei Liu]

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

Re: [PATCH 2/4] vdpa: Protect vdpa reset with cf_mutex

[Jason Wang]

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

Re: [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features

[Si-Wei Liu]

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

Re: [PATCH 3/4] vdpa/mlx5: Fix is_index_valid() to refer to features

[Jason Wang]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Si-Wei Liu]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Jason Wang]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Si-Wei Liu]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Jason Wang]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Si-Wei Liu]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Michael S. Tsirkin]

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

Re: [PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs

[Jason Wang]

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