Re: [PATCH v2 for-next] RDMA/hns: Add a new mmap implementation

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Oct 12, 2021 at 08:41:55PM +0800, Wenpeng Liang wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Add a new implementation for mmap by using the new mmap entry API.
> 
> The new implementation prepares for subsequent features and is compatible
> with the old implementation. And the old implementation using hard-coded
> offset will not be extended in the future.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com>
>  drivers/infiniband/hw/hns/hns_roce_device.h |  23 +++
>  drivers/infiniband/hw/hns/hns_roce_main.c   | 208 +++++++++++++++++---
>  include/uapi/rdma/hns-abi.h                 |  21 +-
>  3 files changed, 225 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index 9467c39e3d28..1d4cf3f083c2 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -225,11 +225,25 @@ struct hns_roce_uar {
>  	unsigned long	logic_idx;
>  };
>  
> +struct hns_user_mmap_entry {
> +	struct rdma_user_mmap_entry rdma_entry;
> +	u64 address;
> +	u8 mmap_flag;

Call this mmap_type and use the enum:

 enum hns_roce_mmap_type mmap_type

> +struct hns_user_mmap_entry *hns_roce_user_mmap_entry_insert(
> +				struct ib_ucontext *ucontext, u64 address,
> +				size_t length, u8 mmap_flag)
> +{
> +#define HNS_ROCE_PGOFFSET_TPTR 1
> +#define HNS_ROCE_PGOFFSET_DB 0
> +	struct hns_roce_ucontext *context = to_hr_ucontext(ucontext);
> +	struct hns_user_mmap_entry *entry;
> +	int ret;
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return NULL;
> +
> +	entry->address = address;
> +	entry->mmap_flag = mmap_flag;
> +
> +	if (context->mmap_key_support) {
> +		ret = rdma_user_mmap_entry_insert(ucontext, &entry->rdma_entry,
> +						  length);
> +	} else {
> +		switch (mmap_flag) {
> +		case HNS_ROCE_MMAP_TYPE_DB:
> +			ret = rdma_user_mmap_entry_insert_range(ucontext,
> +						&entry->rdma_entry, length,
> +						HNS_ROCE_PGOFFSET_DB,
> +						HNS_ROCE_PGOFFSET_DB);

Please add this to avoid the odd #defines:

static inline int
rdma_user_mmap_entry_insert_exact(struct ib_ucontext *ucontext,
                                  struct rdma_user_mmap_entry *entry,
                                  size_t length, u32 pgoff)
{
        return rdma_user_mmap_entry_insert_range(ucontext, entry, length, pgoff,
                                                 pgoff);
}

> -static int hns_roce_mmap(struct ib_ucontext *context,
> -			 struct vm_area_struct *vma)
> +static int hns_roce_mmap(struct ib_ucontext *uctx, struct vm_area_struct *vma)
>  {
> -	struct hns_roce_dev *hr_dev = to_hr_dev(context->device);
> -
> -	switch (vma->vm_pgoff) {
> -	case 0:
> -		return rdma_user_mmap_io(context, vma,
> -					 to_hr_ucontext(context)->uar.pfn,
> -					 PAGE_SIZE,
> -					 pgprot_noncached(vma->vm_page_prot),
> -					 NULL);
> -
> -	/* vm_pgoff: 1 -- TPTR */
> -	case 1:
> -		if (!hr_dev->tptr_dma_addr || !hr_dev->tptr_size)
> -			return -EINVAL;
> +	struct hns_roce_dev *hr_dev = to_hr_dev(uctx->device);
> +	struct ib_device *ibdev = &hr_dev->ib_dev;
> +	struct rdma_user_mmap_entry *rdma_entry;
> +	struct hns_user_mmap_entry *entry;
> +	phys_addr_t pfn;
> +	pgprot_t prot;
> +	int ret;
> +
> +	rdma_entry = rdma_user_mmap_entry_get_pgoff(uctx, vma->vm_pgoff);
> +	if (!rdma_entry) {
> +		ibdev_err(ibdev, "Invalid entry vm_pgoff %lu.\n",
> +			  vma->vm_pgoff);
> +		return -EINVAL;
> +	}
> +
> +	entry = to_hns_mmap(rdma_entry);
> +	pfn = entry->address >> PAGE_SHIFT;
> +	prot = vma->vm_page_prot;

Just write

 if (entry->mmap_type != HNS_ROCE_MMAP_TYPE_TPTR)
    prot = pgprot_noncached(prot);
 ret = rdma_user_mmap_io(uctx, vma, pfn,
			rdma_entry->npages * PAGE_SIZE,
			pgprot_noncached(prot), rdma_entry);

No need for the big case statement

> diff --git a/include/uapi/rdma/hns-abi.h b/include/uapi/rdma/hns-abi.h
> index 42b177655560..ce1e39f21d73 100644
> +++ b/include/uapi/rdma/hns-abi.h
> @@ -83,11 +83,30 @@ struct hns_roce_ib_create_qp_resp {
>  	__aligned_u64 cap_flags;
>  };
>  
> +enum hns_roce_alloc_uctx_comp_flag {
> +	HNS_ROCE_ALLOC_UCTX_COMP_CONFIG = 1 << 0,
> +};
> +
> +enum hns_roce_alloc_uctx_resp_config {
> +	HNS_ROCE_UCTX_RESP_MMAP_KEY_EN = 1 << 0,
> +};
> +
> +enum hns_roce_alloc_uctx_req_config {
> +	HNS_ROCE_UCTX_REQ_MMAP_KEY_EN = 1 << 0,
> +};
> +
> +struct hns_roce_ib_alloc_ucontext {
> +	__u32 comp;
> +	__u32 config;
> +};
> +
>  struct hns_roce_ib_alloc_ucontext_resp {
>  	__u32	qp_tab_size;
>  	__u32	cqe_size;
>  	__u32	srq_tab_size;
> -	__u32	reserved;
> +	__u8    config;
> +	__u8    rsv[3];
> +	__aligned_u64 db_mmap_key;

I'm confused, this doesn't change the uAPI, so why add this stuff?
This should go in a later patch?

Jason