| Message ID | 20200907075136.GA114876@mtl-vdi-166.wap.labs.mlnx |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | vdpa/mlx5: Setup driver only if VIRTIO_CONFIG_S_DRIVER_OK | expand |
----- Original Message ----- > If the memory map changes before the driver status is > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > may fail. For example, if the VQ is not ready there is no point in > creating resources. > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > Signed-off-by: Eli Cohen <elic@nvidia.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 9df69d5efe8c..c89cd48a0aab 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net > *ndev, struct vhost_iotlb * > if (err) > goto err_mr; > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > + return 0; > + I'm not sure I get this. It looks to me if set_map() is called before DRIVER_OK, we won't build any mapping? Thanks > restore_channels_info(ndev); > err = setup_driver(ndev); > if (err) > -- > 2.26.0 > >
On Mon, Sep 07, 2020 at 06:53:23AM -0400, Jason Wang wrote: > > > ----- Original Message ----- > > If the memory map changes before the driver status is > > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > > may fail. For example, if the VQ is not ready there is no point in > > creating resources. > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > > Signed-off-by: Eli Cohen <elic@nvidia.com> > > --- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index 9df69d5efe8c..c89cd48a0aab 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net > > *ndev, struct vhost_iotlb * > > if (err) > > goto err_mr; > > > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > > + return 0; > > + > > I'm not sure I get this. > > It looks to me if set_map() is called before DRIVER_OK, we won't build > any mapping? > What would prevent that? Is it some qemu logic you're relying upon? With current qemu 5.1 with lack of batching support, I get plenty calls to set_map which result in calls to mlx5_vdpa_change_map(). If that happens before VIRTIO_CONFIG_S_DRIVER_OK then Imay fail (in case I was not called to set VQs ready). > > > restore_channels_info(ndev); > > err = setup_driver(ndev); > > if (err) > > -- > > 2.26.0 > > > > >
On Mon, Sep 07, 2020 at 10:51:36AM +0300, Eli Cohen wrote: > If the memory map changes before the driver status is > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > may fail. For example, if the VQ is not ready there is no point in > creating resources. > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > Signed-off-by: Eli Cohen <elic@nvidia.com> Could you add a bit more data about the problem to the log? To be more exact, what exactly happens right now? > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 9df69d5efe8c..c89cd48a0aab 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb * > if (err) > goto err_mr; > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > + return 0; > + > restore_channels_info(ndev); > err = setup_driver(ndev); > if (err) > -- > 2.26.0
On Mon, Sep 07, 2020 at 07:34:00AM -0400, Michael S. Tsirkin wrote: > On Mon, Sep 07, 2020 at 10:51:36AM +0300, Eli Cohen wrote: > > If the memory map changes before the driver status is > > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > > may fail. For example, if the VQ is not ready there is no point in > > creating resources. > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > > Signed-off-by: Eli Cohen <elic@nvidia.com> > > > Could you add a bit more data about the problem to the log? > To be more exact, what exactly happens right now? > Sure I can. set_map() is used by mlx5 vdpa to create a memory region based on the address map passed by the iotlb argument. If I get successive calls, I will destroy the current memory region and build another one based on the new address mapping. I also need to setup the hardware resources since they depend on the memory region. If these calls happen before DRIVER_OK, It means it that driver VQs may also not been setup and I may not create them yet. In this case I want to avoid setting up the other resources and defer this till I get DRIVER OK. Let me know if that answers your question so I can post another patch. > > --- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index 9df69d5efe8c..c89cd48a0aab 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb * > > if (err) > > goto err_mr; > > > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > > + return 0; > > + > > restore_channels_info(ndev); > > err = setup_driver(ndev); > > if (err) > > -- > > 2.26.0 >
On Mon, Sep 07, 2020 at 02:43:51PM +0300, Eli Cohen wrote: > On Mon, Sep 07, 2020 at 07:34:00AM -0400, Michael S. Tsirkin wrote: > > On Mon, Sep 07, 2020 at 10:51:36AM +0300, Eli Cohen wrote: > > > If the memory map changes before the driver status is > > > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > > > may fail. For example, if the VQ is not ready there is no point in > > > creating resources. > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") > > > Signed-off-by: Eli Cohen <elic@nvidia.com> > > > > > > Could you add a bit more data about the problem to the log? > > To be more exact, what exactly happens right now? > > > > Sure I can. > > set_map() is used by mlx5 vdpa to create a memory region based on the > address map passed by the iotlb argument. If I get successive calls, I > will destroy the current memory region and build another one based on > the new address mapping. I also need to setup the hardware resources > since they depend on the memory region. > > If these calls happen before DRIVER_OK, It means it that driver VQs may > also not been setup and I may not create them yet. In this case I want > to avoid setting up the other resources and defer this till I get DRIVER > OK. > > Let me know if that answers your question so I can post another patch. it does, pls do. > > > --- > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > index 9df69d5efe8c..c89cd48a0aab 100644 > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb * > > > if (err) > > > goto err_mr; > > > > > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > > > + return 0; > > > + > > > restore_channels_info(ndev); > > > err = setup_driver(ndev); > > > if (err) > > > -- > > > 2.26.0 > >
----- Original Message ----- > On Mon, Sep 07, 2020 at 06:53:23AM -0400, Jason Wang wrote: > > > > > > ----- Original Message ----- > > > If the memory map changes before the driver status is > > > VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it > > > may fail. For example, if the VQ is not ready there is no point in > > > creating resources. > > > > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 > > > devices") > > > Signed-off-by: Eli Cohen <elic@nvidia.com> > > > --- > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > index 9df69d5efe8c..c89cd48a0aab 100644 > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct > > > mlx5_vdpa_net > > > *ndev, struct vhost_iotlb * > > > if (err) > > > goto err_mr; > > > > > > + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) > > > + return 0; > > > + > > > > I'm not sure I get this. > > > > It looks to me if set_map() is called before DRIVER_OK, we won't build > > any mapping? > > > What would prevent that? Is it some qemu logic you're relying upon? Ok, I think the map is still there, we just avoid to create some resources. > With current qemu 5.1 with lack of batching support, I get plenty calls > to set_map which result in calls to mlx5_vdpa_change_map(). > If that happens before VIRTIO_CONFIG_S_DRIVER_OK then Imay fail (in case > I was not called to set VQs ready). Right, this could be solved by adding the batched IOTLB updating. Thanks > > > > > > restore_channels_info(ndev); > > > err = setup_driver(ndev); > > > if (err) > > > -- > > > 2.26.0 > > > > > > > > > >
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 9df69d5efe8c..c89cd48a0aab 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1645,6 +1645,9 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_net *ndev, struct vhost_iotlb * if (err) goto err_mr; + if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) + return 0; + restore_channels_info(ndev); err = setup_driver(ndev); if (err)
If the memory map changes before the driver status is VIRTIO_CONFIG_S_DRIVER_OK, don't attempt to create resources because it may fail. For example, if the VQ is not ready there is no point in creating resources. Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") Signed-off-by: Eli Cohen <elic@nvidia.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +++ 1 file changed, 3 insertions(+)