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 <[email protected]>
---
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
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 <[email protected]>
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
> -----Original Message-----
> From: Michael S. Tsirkin <[email protected]>
> Sent: Friday, June 9, 2023 3:45 AM
> To: Angus Chen <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [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 <[email protected]>
>
> I am not sure about this one. How does userspace know
> which values are legal?
Maybe we can print device cap of device in dev_err?
>
> If there's no way then maybe we should just cap the value
> to what device can support but otherwise keep the device
> working.
We I use max_vqs pair to test vp_vdpa,it doesn't work as expect.
And there is no any hint of this.
>
> > ---
> > 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
On Fri, Jun 9, 2023 at 3:45 AM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>
> 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
>
On Fri, Jun 09, 2023 at 12:42:22AM +0000, Angus Chen wrote:
>
>
> > -----Original Message-----
> > From: Michael S. Tsirkin <[email protected]>
> > Sent: Friday, June 9, 2023 3:45 AM
> > To: Angus Chen <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [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 <[email protected]>
> >
> > I am not sure about this one. How does userspace know
> > which values are legal?
> Maybe we can print device cap of device in dev_err?
No one reads these except kernel devs. Surely not userspace.
> >
> > If there's no way then maybe we should just cap the value
> > to what device can support but otherwise keep the device
> > working.
> We I use max_vqs pair to test vp_vdpa,it doesn't work as expect.
> And there is no any hint of this.
So things don't work either way just differently.
Let's come up with a way for userspace to know what's legal
so things can start 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
>
On Thu, Jun 8, 2023 at 5:02 PM Angus Chen <[email protected]> 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 <[email protected]>
> ---
> 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;
Why bother with those changes?
mgtdev->max_supported_vqs = vp_modern_get_num_queues(mdev);
Thanks
> + 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
>
On Mon, Jun 26, 2023 at 10:42 AM Angus Chen <[email protected]> wrote:
>
>
> Hi，jason.
> > -----Original Message-----
> > From: Jason Wang <[email protected]>
> > Sent: Monday, June 26, 2023 10:30 AM
> > To: Angus Chen <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from
> > add_config
> >
> > On Thu, Jun 8, 2023 at 5:02 PM Angus Chen <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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;
> >
> > Why bother with those changes?
> >
> > mgtdev->max_supported_vqs = vp_modern_get_num_queues(mdev);
> max_supported_vqs will not be changed, so we can get max_supported_vqs from mgtdev->max_supported_vqs.
> If we use vp_modern_get_num_queues(mdev),it will use tlp to communicate with device.
> It just reduce some tlp .
Ok, but
1) I think we don't care the performance here
2) If we did, let's use a separate patch to do that as an optimization
Thanks
> >
> > Thanks
> >
> >
> > > + 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
> > >
>
Hi，jason.
> -----Original Message-----
> From: Jason Wang <[email protected]>
> Sent: Monday, June 26, 2023 10:30 AM
> To: Angus Chen <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from
> add_config
>
> On Thu, Jun 8, 2023 at 5:02 PM Angus Chen <[email protected]>
> 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 <[email protected]>
> > ---
> > 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;
>
> Why bother with those changes?
>
> mgtdev->max_supported_vqs = vp_modern_get_num_queues(mdev);
max_supported_vqs will not be changed, so we can get max_supported_vqs from mgtdev->max_supported_vqs.
If we use vp_modern_get_num_queues(mdev),it will use tlp to communicate with device.
It just reduce some tlp .
>
> Thanks
>
>
> > + 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
> >
On Mon, Jun 26, 2023 at 11:02 AM Angus Chen <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <[email protected]>
> > Sent: Monday, June 26, 2023 10:51 AM
> > To: Angus Chen <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from
> > add_config
> >
> > On Mon, Jun 26, 2023 at 10:42 AM Angus Chen <[email protected]>
> > wrote:
> > >
> > >
> > > Hi，jason.
> > > > -----Original Message-----
> > > > From: Jason Wang <[email protected]>
> > > > Sent: Monday, June 26, 2023 10:30 AM
> > > > To: Angus Chen <[email protected]>
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device
> > from
> > > > add_config
> > > >
> > > > On Thu, Jun 8, 2023 at 5:02 PM Angus Chen
> > <[email protected]>
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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;
> > > >
> > > > Why bother with those changes?
> > > >
> > > > mgtdev->max_supported_vqs =
> > vp_modern_get_num_queues(mdev);
> > > max_supported_vqs will not be changed, so we can get max_supported_vqs
> > from mgtdev->max_supported_vqs.
> > > If we use vp_modern_get_num_queues(mdev),it will use tlp to communicate
> > with device.
> > > It just reduce some tlp .
> >
> > Ok, but
> >
> > 1) I think we don't care the performance here
> > 2) If we did, let's use a separate patch to do that as an optimization
> >
> Thank you.As mst did not support this patch some days ago,so this patch will be dropped.
> I plan to push a dependent driver of our product rather than reuse vp_vdpa.
That would be fine. But please try best to reuse modern virtio-pci library.
> By the way ,if I want to add sriov support in our vdpa pci driver,would it be accepted or not?
I think the answer is yes.
Thanks
> > Thanks
> >
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > + 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
> > > > >
> > >
>
> -----Original Message-----
> From: Jason Wang <[email protected]>
> Sent: Monday, June 26, 2023 10:51 AM
> To: Angus Chen <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device from
> add_config
>
> On Mon, Jun 26, 2023 at 10:42 AM Angus Chen <[email protected]>
> wrote:
> >
> >
> > Hi，jason.
> > > -----Original Message-----
> > > From: Jason Wang <[email protected]>
> > > Sent: Monday, June 26, 2023 10:30 AM
> > > To: Angus Chen <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH v2] vdpa/vp_vdpa: Check queue number of vdpa device
> from
> > > add_config
> > >
> > > On Thu, Jun 8, 2023 at 5:02 PM Angus Chen
> <[email protected]>
> > > 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 <[email protected]>
> > > > ---
> > > > 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;
> > >
> > > Why bother with those changes?
> > >
> > > mgtdev->max_supported_vqs =
> vp_modern_get_num_queues(mdev);
> > max_supported_vqs will not be changed, so we can get max_supported_vqs
> from mgtdev->max_supported_vqs.
> > If we use vp_modern_get_num_queues(mdev),it will use tlp to communicate
> with device.
> > It just reduce some tlp .
>
> Ok, but
>
> 1) I think we don't care the performance here
> 2) If we did, let's use a separate patch to do that as an optimization
>
Thank you.As mst did not support this patch some days ago,so this patch will be dropped.
I plan to push a dependent driver of our product rather than reuse vp_vdpa.
By the way ,if I want to add sriov support in our vdpa pci driver,would it be accepted or not?
> Thanks
>
> > >
> > > Thanks
> > >
> > >
> > > > + 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
> > > >
> >