Michael S. Tsirkin
2023-Jun-08 19:44 UTC
[PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from add_config
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 at 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
Jason Wang
2023-Jun-09 02:30 UTC
[PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from add_config
On Fri, Jun 9, 2023 at 3:45?AM Michael S. Tsirkin <mst at redhat.com> wrote:> > 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.I think I kind of agree with Michael, I don't see any obvious advantages to allow usersapce to configure max_vqp if it can't be provisioned dynamically. What's wrong if we just stick the current approach that doesn't accept max_vqp? A better approach is to tweak the vdpa tool to display the legal attributes that can be provisioned.> > > > Signed-off-by: Angus Chen <angus.chen at jaguarmicro.com> > > I am not sure about this one. How does userspace know > which values are legal?vdpa mgmtdev show can gives hints like: max_supported_vqs 3> > If there's no way then maybe we should just cap the value > to what device can support but otherwise keep the device > working.This seems conflict to how other drivers (like mlx5) did: if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) { if (add_config->net.max_vq_pairs > max_vqs / 2) return -EINVAL; max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs); } else { max_vqs = 2; } Thanks> > > --- > > 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 >