| Message ID | 1713363659-30156-3-git-send-email-kotaranov@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/mana_ib: Enable DMA-mapped memory regions | expand |
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
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
> 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
> 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
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
> 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
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
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