* [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
@ 2021-10-28 5:55 Leon Romanovsky
2021-10-28 7:56 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Leon Romanovsky @ 2021-10-28 5:55 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Aharon Landau, linux-kernel, linux-rdma
From: Aharon Landau <aharonl@nvidia.com>
The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
flag is provided.
Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
Signed-off-by: Aharon Landau <aharonl@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/infiniband/core/uverbs_cmd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 740e6b2efe0e..d1345d76d9b1 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
new_mr->device = new_pd->device;
new_mr->pd = new_pd;
new_mr->type = IB_MR_TYPE_USER;
- new_mr->dm = NULL;
- new_mr->sig_attrs = NULL;
new_mr->uobject = uobj;
atomic_inc(&new_pd->usecnt);
- new_mr->iova = cmd.hca_va;
new_uobj->object = new_mr;
rdma_restrack_new(&new_mr->res, RDMA_RESTRACK_MR);
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-28 5:55 [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA Leon Romanovsky
@ 2021-10-28 7:56 ` Christoph Hellwig
2021-10-28 9:58 ` Leon Romanovsky
2021-10-29 16:27 ` Jason Gunthorpe
2021-11-03 12:51 ` Jason Gunthorpe
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-10-28 7:56 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Jason Gunthorpe, Aharon Landau, linux-kernel, linux-rdma
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> From: Aharon Landau <aharonl@nvidia.com>
>
> The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> flag is provided.
How is a "vendor" involved with this? This should all be upstream code.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-28 7:56 ` Christoph Hellwig
@ 2021-10-28 9:58 ` Leon Romanovsky
2021-10-28 14:02 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-10-28 9:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Doug Ledford, Jason Gunthorpe, Aharon Landau, linux-kernel, linux-rdma
On Thu, Oct 28, 2021 at 12:56:46AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> > From: Aharon Landau <aharonl@nvidia.com>
> >
> > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> > flag is provided.
>
> How is a "vendor" involved with this? This should all be upstream code.
"vendor" is wrong word here.
I wanted to say that all drivers which support ".rereg_user_mr()"
callback and return new_mr should set everything. In case of IB_MR_REREG_TRANS
flow, it is IOVA which is not cmd.hca_va, but mr->iova.
Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-28 9:58 ` Leon Romanovsky
@ 2021-10-28 14:02 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2021-10-28 14:02 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Christoph Hellwig, Doug Ledford, Jason Gunthorpe, Aharon Landau,
linux-kernel, linux-rdma
On Thu, Oct 28, 2021 at 12:58:53PM +0300, Leon Romanovsky wrote:
> "vendor" is wrong word here.
>
> I wanted to say that all drivers which support ".rereg_user_mr()"
> callback and return new_mr should set everything. In case of IB_MR_REREG_TRANS
> flow, it is IOVA which is not cmd.hca_va, but mr->iova.
Thanks, that makes a lot more sense.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-28 5:55 [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA Leon Romanovsky
2021-10-28 7:56 ` Christoph Hellwig
@ 2021-10-29 16:27 ` Jason Gunthorpe
2021-10-29 16:50 ` Leon Romanovsky
2021-11-03 12:51 ` Jason Gunthorpe
2 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2021-10-29 16:27 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Aharon Landau, linux-kernel, linux-rdma
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> From: Aharon Landau <aharonl@nvidia.com>
>
> The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> flag is provided.
>
> Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
This isn't really a fixes type patch..
> Signed-off-by: Aharon Landau <aharonl@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> drivers/infiniband/core/uverbs_cmd.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 740e6b2efe0e..d1345d76d9b1 100644
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
> new_mr->device = new_pd->device;
> new_mr->pd = new_pd;
> new_mr->type = IB_MR_TYPE_USER;
> - new_mr->dm = NULL;
> - new_mr->sig_attrs = NULL;
> new_mr->uobject = uobj;
> atomic_inc(&new_pd->usecnt);
> - new_mr->iova = cmd.hca_va;
> new_uobj->object = new_mr;
It is like this because the reg_mr path does it this way, if you want
to change it here then change reg_mr as well, but that needs auditing
all the drivers..
And I'd also suggest removing the other set a few lines below
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-29 16:27 ` Jason Gunthorpe
@ 2021-10-29 16:50 ` Leon Romanovsky
2021-10-29 16:51 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-10-29 16:50 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, Aharon Landau, linux-kernel, linux-rdma
On Fri, Oct 29, 2021 at 01:27:02PM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> > From: Aharon Landau <aharonl@nvidia.com>
> >
> > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> > flag is provided.
> >
> > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
>
> This isn't really a fixes type patch..
Why? We see that without this patch MR IOVA is not as expected.
>
> > Signed-off-by: Aharon Landau <aharonl@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > drivers/infiniband/core/uverbs_cmd.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index 740e6b2efe0e..d1345d76d9b1 100644
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
> > new_mr->device = new_pd->device;
> > new_mr->pd = new_pd;
> > new_mr->type = IB_MR_TYPE_USER;
> > - new_mr->dm = NULL;
> > - new_mr->sig_attrs = NULL;
> > new_mr->uobject = uobj;
> > atomic_inc(&new_pd->usecnt);
> > - new_mr->iova = cmd.hca_va;
> > new_uobj->object = new_mr;
>
> It is like this because the reg_mr path does it this way, if you want
> to change it here then change reg_mr as well, but that needs auditing
> all the drivers..
>
> And I'd also suggest removing the other set a few lines below
I decided to be safe than sorry and removed only 100% correct
attributes that does require minimal audit.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-29 16:50 ` Leon Romanovsky
@ 2021-10-29 16:51 ` Jason Gunthorpe
0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2021-10-29 16:51 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Aharon Landau, linux-kernel, linux-rdma
On Fri, Oct 29, 2021 at 07:50:37PM +0300, Leon Romanovsky wrote:
> On Fri, Oct 29, 2021 at 01:27:02PM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> > > From: Aharon Landau <aharonl@nvidia.com>
> > >
> > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> > > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> > > flag is provided.
> > >
> > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
> >
> > This isn't really a fixes type patch..
>
> Why? We see that without this patch MR IOVA is not as expected.
How so? Aharon should explain that in the commit message
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-10-28 5:55 [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA Leon Romanovsky
2021-10-28 7:56 ` Christoph Hellwig
2021-10-29 16:27 ` Jason Gunthorpe
@ 2021-11-03 12:51 ` Jason Gunthorpe
2021-11-03 13:24 ` Leon Romanovsky
2 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2021-11-03 12:51 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Aharon Landau, linux-kernel, linux-rdma
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> From: Aharon Landau <aharonl@nvidia.com>
>
> The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> flag is provided.
>
> Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
> Signed-off-by: Aharon Landau <aharonl@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
> drivers/infiniband/core/uverbs_cmd.c | 3 ---
> 1 file changed, 3 deletions(-)
I rewrote the commit message:
RDMA/core: Require the driver to set the IOVA correctly during rereg_mr
If the driver returns a new MR during rereg it has to fill it with the
IOVA from the proper source. If IB_MR_REREG_TRANS is set then the IOVA is
cmd.hca_va, otherwise the IOVA comes from the old MR. mlx5 for example has
two calls inside rereg_mr:
return create_real_mr(new_pd, umem, mr->ibmr.iova,
new_access_flags);
and
return create_real_mr(new_pd, new_umem, iova, new_access_flags);
Unconditionally overwriting the iova in the newly allocated MR will
corrupt the iova if the first path is used.
Remove the redundant initializations from ib_uverbs_rereg_mr().
Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
Link: https://lore.kernel.org/r/4b0a31bbc372842613286a10d7a8cbb0ee6069c7.1635400472.git.leonro@nvidia.com
Signed-off-by: Aharon Landau <aharonl@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA
2021-11-03 12:51 ` Jason Gunthorpe
@ 2021-11-03 13:24 ` Leon Romanovsky
0 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2021-11-03 13:24 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, Aharon Landau, linux-kernel, linux-rdma
On Wed, Nov 03, 2021 at 09:51:44AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote:
> > From: Aharon Landau <aharonl@nvidia.com>
> >
> > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't
> > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS
> > flag is provided.
> >
> > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
> > Signed-off-by: Aharon Landau <aharonl@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > drivers/infiniband/core/uverbs_cmd.c | 3 ---
> > 1 file changed, 3 deletions(-)
>
> I rewrote the commit message:
Thanks a lot.
>
> RDMA/core: Require the driver to set the IOVA correctly during rereg_mr
>
> If the driver returns a new MR during rereg it has to fill it with the
> IOVA from the proper source. If IB_MR_REREG_TRANS is set then the IOVA is
> cmd.hca_va, otherwise the IOVA comes from the old MR. mlx5 for example has
> two calls inside rereg_mr:
>
> return create_real_mr(new_pd, umem, mr->ibmr.iova,
> new_access_flags);
> and
> return create_real_mr(new_pd, new_umem, iova, new_access_flags);
>
> Unconditionally overwriting the iova in the newly allocated MR will
> corrupt the iova if the first path is used.
>
> Remove the redundant initializations from ib_uverbs_rereg_mr().
>
> Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr")
> Link: https://lore.kernel.org/r/4b0a31bbc372842613286a10d7a8cbb0ee6069c7.1635400472.git.leonro@nvidia.com
> Signed-off-by: Aharon Landau <aharonl@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>
> Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-03 13:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-28 5:55 [PATCH rdma-next] RDMA/core: Rely on vendors to set right IOVA Leon Romanovsky
2021-10-28 7:56 ` Christoph Hellwig
2021-10-28 9:58 ` Leon Romanovsky
2021-10-28 14:02 ` Christoph Hellwig
2021-10-29 16:27 ` Jason Gunthorpe
2021-10-29 16:50 ` Leon Romanovsky
2021-10-29 16:51 ` Jason Gunthorpe
2021-11-03 12:51 ` Jason Gunthorpe
2021-11-03 13:24 ` Leon Romanovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).