Mailing List Archive

[PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr
From: Konstantin Taranov <kotaranov@microsoft.com>

Implement allocation of DMA-mapped memory regions.

Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
---
drivers/infiniband/hw/mana/device.c | 1 +
drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
include/net/mana/gdma.h | 5 ++++
3 files changed, 42 insertions(+)

diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
index 6fa902ee80a6..043cef09f1c2 100644
--- a/drivers/infiniband/hw/mana/device.c
+++ b/drivers/infiniband/hw/mana/device.c
@@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops = {
.destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
.destroy_wq = mana_ib_destroy_wq,
.disassociate_ucontext = mana_ib_disassociate_ucontext,
+ .get_dma_mr = mana_ib_get_dma_mr,
.get_port_immutable = mana_ib_get_port_immutable,
.mmap = mana_ib_mmap,
.modify_qp = mana_ib_modify_qp,
diff --git a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
index 4f13423ecdbd..7c9394926a18 100644
--- a/drivers/infiniband/hw/mana/mr.c
+++ b/drivers/infiniband/hw/mana/mr.c
@@ -8,6 +8,8 @@
#define VALID_MR_FLAGS \
(IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ)

+#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
+
static enum gdma_mr_access_flags
mana_ib_verbs_to_gdma_access_flags(int access_flags)
{
@@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev *dev, struct mana_ib_mr *mr,
req.mr_type = mr_params->mr_type;

switch (mr_params->mr_type) {
+ case GDMA_MR_TYPE_GPA:
+ break;
case GDMA_MR_TYPE_GVA:
req.gva.dma_region_handle = mr_params->gva.dma_region_handle;
req.gva.virtual_address = mr_params->gva.virtual_address;
@@ -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd *ibpd, u64 start, u64 length,
return ERR_PTR(err);
}

+struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int access_flags)
+{
+ struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd, ibpd);
+ struct gdma_create_mr_params mr_params = {};
+ struct ib_device *ibdev = ibpd->device;
+ struct mana_ib_dev *dev;
+ struct mana_ib_mr *mr;
+ int err;
+
+ dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
+
+ if (access_flags & ~VALID_DMA_MR_FLAGS)
+ return ERR_PTR(-EINVAL);
+
+ mr = kzalloc(sizeof(*mr), GFP_KERNEL);
+ if (!mr)
+ return ERR_PTR(-ENOMEM);
+
+ mr_params.pd_handle = pd->pd_handle;
+ mr_params.mr_type = GDMA_MR_TYPE_GPA;
+
+ err = mana_ib_gd_create_mr(dev, mr, &mr_params);
+ if (err)
+ goto err_free;
+
+ return &mr->ibmr;
+
+err_free:
+ kfree(mr);
+ return ERR_PTR(err);
+}
+
int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
{
struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr, ibmr);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 8d796a30ddde..dc19b5cb33a6 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
};/* HW DATA */

enum gdma_mr_type {
+ /*
+ * Guest Physical Address - MRs of this type allow access
+ * to any DMA-mapped memory using bus-logical address
+ */
+ GDMA_MR_TYPE_GPA = 1,
/* Guest Virtual Address - MRs of this type allow access
* to memory mapped by PTEs associated with this MR using a virtual
* address that is set up in the MST
--
2.43.0
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> From: Konstantin Taranov <kotaranov@microsoft.com>
>
> Implement allocation of DMA-mapped memory regions.
>
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> ---
> drivers/infiniband/hw/mana/device.c | 1 +
> drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
> include/net/mana/gdma.h | 5 ++++
> 3 files changed, 42 insertions(+)

What is the point of doing this without supporting enough verbs to
allow a kernel ULP?

Jason
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
On 17.04.24 16:20, Konstantin Taranov wrote:
> From: Konstantin Taranov <kotaranov@microsoft.com>
>
> Implement allocation of DMA-mapped memory regions.
>
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> ---
> drivers/infiniband/hw/mana/device.c | 1 +
> drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
> include/net/mana/gdma.h | 5 ++++
> 3 files changed, 42 insertions(+)
>
> diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
> index 6fa902ee80a6..043cef09f1c2 100644
> --- a/drivers/infiniband/hw/mana/device.c
> +++ b/drivers/infiniband/hw/mana/device.c
> @@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops = {
> .destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
> .destroy_wq = mana_ib_destroy_wq,
> .disassociate_ucontext = mana_ib_disassociate_ucontext,
> + .get_dma_mr = mana_ib_get_dma_mr,
> .get_port_immutable = mana_ib_get_port_immutable,
> .mmap = mana_ib_mmap,
> .modify_qp = mana_ib_modify_qp,
> diff --git a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
> index 4f13423ecdbd..7c9394926a18 100644
> --- a/drivers/infiniband/hw/mana/mr.c
> +++ b/drivers/infiniband/hw/mana/mr.c
> @@ -8,6 +8,8 @@
> #define VALID_MR_FLAGS \
> (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ)
>
> +#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
> +
> static enum gdma_mr_access_flags
> mana_ib_verbs_to_gdma_access_flags(int access_flags)
> {
> @@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev *dev, struct mana_ib_mr *mr,
> req.mr_type = mr_params->mr_type;
>
> switch (mr_params->mr_type) {
> + case GDMA_MR_TYPE_GPA:
> + break;
> case GDMA_MR_TYPE_GVA:
> req.gva.dma_region_handle = mr_params->gva.dma_region_handle;
> req.gva.virtual_address = mr_params->gva.virtual_address;
> @@ -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd *ibpd, u64 start, u64 length,
> return ERR_PTR(err);
> }
>

Not sure if the following function needs comments or not.
If yes, the kernel doc
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst?h=v6.9-rc4#n67
can provide a good example.

Best Regards,
Zhu Yanjun

> +struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int access_flags)
> +{
> + struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd, ibpd);
> + struct gdma_create_mr_params mr_params = {};
> + struct ib_device *ibdev = ibpd->device;
> + struct mana_ib_dev *dev;
> + struct mana_ib_mr *mr;
> + int err;
> +
> + dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> +
> + if (access_flags & ~VALID_DMA_MR_FLAGS)
> + return ERR_PTR(-EINVAL);
> +
> + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> + if (!mr)
> + return ERR_PTR(-ENOMEM);
> +
> + mr_params.pd_handle = pd->pd_handle;
> + mr_params.mr_type = GDMA_MR_TYPE_GPA;
> +
> + err = mana_ib_gd_create_mr(dev, mr, &mr_params);
> + if (err)
> + goto err_free;
> +
> + return &mr->ibmr;
> +
> +err_free:
> + kfree(mr);
> + return ERR_PTR(err);
> +}
> +
> int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> {
> struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr, ibmr);
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 8d796a30ddde..dc19b5cb33a6 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
> };/* HW DATA */
>
> enum gdma_mr_type {
> + /*
> + * Guest Physical Address - MRs of this type allow access
> + * to any DMA-mapped memory using bus-logical address
> + */
> + GDMA_MR_TYPE_GPA = 1,
> /* Guest Virtual Address - MRs of this type allow access
> * to memory mapped by PTEs associated with this MR using a virtual
> * address that is set up in the MST
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
> From: Zhu Yanjun <zyjzyj2000@gmail.com>
> On 17.04.24 16:20, Konstantin Taranov wrote:
> > From: Konstantin Taranov <kotaranov@microsoft.com>
> >
> > Implement allocation of DMA-mapped memory regions.
> >
> > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > ---
> > drivers/infiniband/hw/mana/device.c | 1 +
> > drivers/infiniband/hw/mana/mr.c | 36
> +++++++++++++++++++++++++++++
> > include/net/mana/gdma.h | 5 ++++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/mana/device.c
> > b/drivers/infiniband/hw/mana/device.c
> > index 6fa902ee80a6..043cef09f1c2 100644
> > --- a/drivers/infiniband/hw/mana/device.c
> > +++ b/drivers/infiniband/hw/mana/device.c
> > @@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops =
> {
> > .destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
> > .destroy_wq = mana_ib_destroy_wq,
> > .disassociate_ucontext = mana_ib_disassociate_ucontext,
> > + .get_dma_mr = mana_ib_get_dma_mr,
> > .get_port_immutable = mana_ib_get_port_immutable,
> > .mmap = mana_ib_mmap,
> > .modify_qp = mana_ib_modify_qp,
> > diff --git a/drivers/infiniband/hw/mana/mr.c
> > b/drivers/infiniband/hw/mana/mr.c index 4f13423ecdbd..7c9394926a18
> > 100644
> > --- a/drivers/infiniband/hw/mana/mr.c
> > +++ b/drivers/infiniband/hw/mana/mr.c
> > @@ -8,6 +8,8 @@
> > #define VALID_MR_FLAGS \
> > (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE |
> > IB_ACCESS_REMOTE_READ)
> >
> > +#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
> > +
> > static enum gdma_mr_access_flags
> > mana_ib_verbs_to_gdma_access_flags(int access_flags)
> > {
> > @@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev
> *dev, struct mana_ib_mr *mr,
> > req.mr_type = mr_params->mr_type;
> >
> > switch (mr_params->mr_type) {
> > + case GDMA_MR_TYPE_GPA:
> > + break;
> > case GDMA_MR_TYPE_GVA:
> > req.gva.dma_region_handle = mr_params-
> >gva.dma_region_handle;
> > req.gva.virtual_address = mr_params->gva.virtual_address;
> @@
> > -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd
> *ibpd, u64 start, u64 length,
> > return ERR_PTR(err);
> > }
> >
>
> Not sure if the following function needs comments or not.
> If yes, the kernel doc
> https://git.ke/
> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2
> Ftree%2FDocumentation%2Fdoc-guide%2Fkernel-doc.rst%3Fh%3Dv6.9-
> rc4%23n67&data=05%7C02%7Ckotaranov%40microsoft.com%7C2816715935
> 85405f280e08dc5f925007%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> 0%7C638490329257001758%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C
> %7C&sdata=1Mzt0DKzty2jMJYm52gP%2FaloYnFGUTzN7gzAP05RdoQ%3D&res
> erved=0
> can provide a good example.
>

Thanks! I will have a look and see how I can improve comments.

> Best Regards,
> Zhu Yanjun
>
> > +struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int
> > +access_flags) {
> > + struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd,
> ibpd);
> > + struct gdma_create_mr_params mr_params = {};
> > + struct ib_device *ibdev = ibpd->device;
> > + struct mana_ib_dev *dev;
> > + struct mana_ib_mr *mr;
> > + int err;
> > +
> > + dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> > +
> > + if (access_flags & ~VALID_DMA_MR_FLAGS)
> > + return ERR_PTR(-EINVAL);
> > +
> > + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> > + if (!mr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + mr_params.pd_handle = pd->pd_handle;
> > + mr_params.mr_type = GDMA_MR_TYPE_GPA;
> > +
> > + err = mana_ib_gd_create_mr(dev, mr, &mr_params);
> > + if (err)
> > + goto err_free;
> > +
> > + return &mr->ibmr;
> > +
> > +err_free:
> > + kfree(mr);
> > + return ERR_PTR(err);
> > +}
> > +
> > int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> > {
> > struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr,
> > ibmr); diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> > index 8d796a30ddde..dc19b5cb33a6 100644
> > --- a/include/net/mana/gdma.h
> > +++ b/include/net/mana/gdma.h
> > @@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
> > };/* HW DATA */
> >
> > enum gdma_mr_type {
> > + /*
> > + * Guest Physical Address - MRs of this type allow access
> > + * to any DMA-mapped memory using bus-logical address
> > + */
> > + GDMA_MR_TYPE_GPA = 1,
> > /* Guest Virtual Address - MRs of this type allow access
> > * to memory mapped by PTEs associated with this MR using a virtual
> > * address that is set up in the MST
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
> From: Jason Gunthorpe <jgg@ziepe.ca>
> On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> > From: Konstantin Taranov <kotaranov@microsoft.com>
> >
> > Implement allocation of DMA-mapped memory regions.
> >
> > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > ---
> > drivers/infiniband/hw/mana/device.c | 1 +
> > drivers/infiniband/hw/mana/mr.c | 36
> +++++++++++++++++++++++++++++
> > include/net/mana/gdma.h | 5 ++++
> > 3 files changed, 42 insertions(+)
>
> What is the point of doing this without supporting enough verbs to allow a
> kernel ULP?
>

True, the proposed code is useless at this state.
Nevertheless, mana_ib team aims to send kernel ULP patches after we are done
with uverbs pathes (i.e., udata is not null). As this change does not conflict with the
current effort, I decided to send this patch now. I can extend the series to make
it more useful.

Jason, could you suggest a minimal list of ib_device_ops methods, that includes
get_dma_mr, which can be approved?

Thanks,
Konstantin

> Jason
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> > > From: Konstantin Taranov <kotaranov@microsoft.com>
> > >
> > > Implement allocation of DMA-mapped memory regions.
> > >
> > > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > > ---
> > > drivers/infiniband/hw/mana/device.c | 1 +
> > > drivers/infiniband/hw/mana/mr.c | 36
> > +++++++++++++++++++++++++++++
> > > include/net/mana/gdma.h | 5 ++++
> > > 3 files changed, 42 insertions(+)
> >
> > What is the point of doing this without supporting enough verbs to allow a
> > kernel ULP?
> >
>
> True, the proposed code is useless at this state.
> Nevertheless, mana_ib team aims to send kernel ULP patches after we are done
> with uverbs pathes (i.e., udata is not null). As this change does not conflict with the
> current effort, I decided to send this patch now. I can extend the series to make
> it more useful.
>
> Jason, could you suggest a minimal list of ib_device_ops methods, that includes
> get_dma_mr, which can be approved?

Is there any chance you can send a single series to support a
ULP. NVMe or something like?

Jason
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
> From: Jason Gunthorpe <jgg@ziepe.ca>
> On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca> On Wed, Apr 17, 2024 at
> > > 07:20:59AM -0700, Konstantin Taranov wrote:
> > > > From: Konstantin Taranov <kotaranov@microsoft.com>
> > > >
> > > > Implement allocation of DMA-mapped memory regions.
> > > >
> > > > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > > > ---
> > > > drivers/infiniband/hw/mana/device.c | 1 +
> > > > drivers/infiniband/hw/mana/mr.c | 36
> > > +++++++++++++++++++++++++++++
> > > > include/net/mana/gdma.h | 5 ++++
> > > > 3 files changed, 42 insertions(+)
> > >
> > > What is the point of doing this without supporting enough verbs to
> > > allow a kernel ULP?
> > >
> >
> > True, the proposed code is useless at this state.
> > Nevertheless, mana_ib team aims to send kernel ULP patches after we
> > are done with uverbs pathes (i.e., udata is not null). As this change
> > does not conflict with the current effort, I decided to send this
> > patch now. I can extend the series to make it more useful.
> >
> > Jason, could you suggest a minimal list of ib_device_ops methods,
> > that includes get_dma_mr, which can be approved?
>
> Is there any chance you can send a single series to support a ULP. NVMe or
> something like?

Sure, I can. I will investigate the way to make get_dma_mr used with fewer changes.

Generally, I am wondering what would be easier for reviewers.
Should I try to send short patch series enabling one feature, or should I actually try
to produce long patch series that enable a use-case consisting of several features?

Konstantin
Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr [ In reply to ]
On Mon, Apr 22, 2024 at 09:12:46AM +0000, Konstantin Taranov wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > > > From: Jason Gunthorpe <jgg@ziepe.ca> On Wed, Apr 17, 2024 at
> > > > 07:20:59AM -0700, Konstantin Taranov wrote:
> > > > > From: Konstantin Taranov <kotaranov@microsoft.com>
> > > > >
> > > > > Implement allocation of DMA-mapped memory regions.
> > > > >
> > > > > Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> > > > > ---
> > > > > drivers/infiniband/hw/mana/device.c | 1 +
> > > > > drivers/infiniband/hw/mana/mr.c | 36
> > > > +++++++++++++++++++++++++++++
> > > > > include/net/mana/gdma.h | 5 ++++
> > > > > 3 files changed, 42 insertions(+)
> > > >
> > > > What is the point of doing this without supporting enough verbs to
> > > > allow a kernel ULP?
> > > >
> > >
> > > True, the proposed code is useless at this state.
> > > Nevertheless, mana_ib team aims to send kernel ULP patches after we
> > > are done with uverbs pathes (i.e., udata is not null). As this change
> > > does not conflict with the current effort, I decided to send this
> > > patch now. I can extend the series to make it more useful.
> > >
> > > Jason, could you suggest a minimal list of ib_device_ops methods,
> > > that includes get_dma_mr, which can be approved?
> >
> > Is there any chance you can send a single series to support a ULP. NVMe or
> > something like?
>
> Sure, I can. I will investigate the way to make get_dma_mr used with fewer changes.
>
> Generally, I am wondering what would be easier for reviewers.
> Should I try to send short patch series enabling one feature, or should I actually try
> to produce long patch series that enable a use-case consisting of several features?

Generally the guideline is to avoid adding dead code, some exceptions
may be possible, but that should be the gold standard to try for,
IMHO.

If you want to support kernel ULPs then say witch kernel ULP you want
and send a series to make it work.

If that series is way too big then split it into two halfs and post
both at the start.

Jason