Sorry, the reply is incomplete, please ignore this email.
On 2021/10/22 16:56, Wenpeng Liang wrote:
> On 2021/10/21 7:15, Jason Gunthorpe wrote:
>> On Tue, Oct 12, 2021 at 08:41:55PM +0800, Wenpeng Liang wrote:
>>> From: Chengchang Tang <tangchengchang@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Signed-off-by: Wenpeng Liang <liangwenpeng@xxxxxxxxxx>
>>> 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
>>
>
> I will fix it in v3, thanks!
>
>>> +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);
>> }
>>
>
> ditto.
>
>>> -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
>>
>
> There will be new features added to this switch branch in a later patch.
>
>>> 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
>> .
>>
>
> The related userspace series is named "libhns: Add a new mmap implementation".
>
> Thanks,
> Wenpeng
> .
>
