Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 15:44:56 -0400
From:   "Michael S. Tsirkin" <mst@redhat.com>
To:     Angus Chen <angus.chen@jaguarmicro.com>
Cc:     jasowang@redhat.com, virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from
 add_config
Message-ID: <20230608154400-mutt-send-email-mst@kernel.org>
References: <20230608090124.1807-1-angus.chen@jaguarmicro.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230608090124.1807-1-angus.chen@jaguarmicro.com>
Precedence: bulk

On Thu, Jun 08, 2023 at 05:01:24PM +0800, Angus Chen wrote:
> When add virtio_pci vdpa device,check the vqs number of device cap
> and max_vq_pairs from add_config.
> Simply starting from failing if the provisioned #qp is not
> equal to the one that hardware has.
> 
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>

I am not sure about this one. How does userspace know
which values are legal?

If there's no way then maybe we should just cap the value
to what device can support but otherwise keep the device
working.

> ---
> v1: Use max_vqs from add_config
> v2: Just return fail if max_vqs from add_config is not same as device
> 	cap. Suggested by jason.
> 
>  drivers/vdpa/virtio_pci/vp_vdpa.c | 35 ++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index 281287fae89f..c1fb6963da12 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -480,32 +480,39 @@ static int vp_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	u64 device_features;
>  	int ret, i;
>  
> -	vp_vdpa = vdpa_alloc_device(struct vp_vdpa, vdpa,
> -				    dev, &vp_vdpa_ops, 1, 1, name, false);
> -
> -	if (IS_ERR(vp_vdpa)) {
> -		dev_err(dev, "vp_vdpa: Failed to allocate vDPA structure\n");
> -		return PTR_ERR(vp_vdpa);
> +	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
> +		if (add_config->net.max_vq_pairs != (v_mdev->max_supported_vqs / 2)) {
> +			dev_err(&pdev->dev, "max vqs 0x%x should be equal to 0x%x which device has\n",
> +				add_config->net.max_vq_pairs*2, v_mdev->max_supported_vqs);
> +			return -EINVAL;
> +		}
>  	}
>  
> -	vp_vdpa_mgtdev->vp_vdpa = vp_vdpa;
> -
> -	vp_vdpa->vdpa.dma_dev = &pdev->dev;
> -	vp_vdpa->queues = vp_modern_get_num_queues(mdev);
> -	vp_vdpa->mdev = mdev;
> -
>  	device_features = vp_modern_get_features(mdev);
>  	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) {
>  		if (add_config->device_features & ~device_features) {
> -			ret = -EINVAL;
>  			dev_err(&pdev->dev, "Try to provision features "
>  				"that are not supported by the device: "
>  				"device_features 0x%llx provisioned 0x%llx\n",
>  				device_features, add_config->device_features);
> -			goto err;
> +			return -EINVAL;
>  		}
>  		device_features = add_config->device_features;
>  	}
> +
> +	vp_vdpa = vdpa_alloc_device(struct vp_vdpa, vdpa,
> +				    dev, &vp_vdpa_ops, 1, 1, name, false);
> +
> +	if (IS_ERR(vp_vdpa)) {
> +		dev_err(dev, "vp_vdpa: Failed to allocate vDPA structure\n");
> +		return PTR_ERR(vp_vdpa);
> +	}
> +
> +	vp_vdpa_mgtdev->vp_vdpa = vp_vdpa;
> +
> +	vp_vdpa->vdpa.dma_dev = &pdev->dev;
> +	vp_vdpa->queues = v_mdev->max_supported_vqs;
> +	vp_vdpa->mdev = mdev;
>  	vp_vdpa->device_features = device_features;
>  
>  	ret = devm_add_action_or_reset(dev, vp_vdpa_free_irq_vectors, pdev);
> -- 
> 2.25.1