On 03/07/2019 11:53, Michal Kalderon wrote:
>> From: Gal Pressman <galpress@xxxxxxxxxx>
>> Sent: Wednesday, July 3, 2019 11:20 AM
>>
>> On 03/07/2019 1:31, Jason Gunthorpe wrote:
>>>> Seems except Mellanox + hns the mmap flags aren't ABI.
>>>> Also, current Mellanox code seems like it won't benefit from mmap
>>>> cookie helper functions in any case as the mmap function is very
>>>> specific and the flags used indicate the address and not just how to map
>> it.
>>>
>>> IMHO, mlx5 has a goofy implementaiton here as it codes all of the
>>> object type, handle and cachability flags in one thing.
>>
>> Do we need object type flags as well in the generic mmap code?
>>
>>>
>>>> For most drivers (efa, qedr, siw, cxgb3/4, ocrdma) mmap is called on
>>>> address received by kernel in some response. Meaning driver can write
>>>> anything in the response that will serve as the key / flag.
>>>> Other drivers ( i40iw ) have a simple mmap function that doesn't
>>>> require a mmap database at all.
>>>
>>> Are you sure? I thought the reason to have to flags at all was so that
>>> userspace could specify different cachability..
>>>
>>> Otherwise the offset should just be an opaque cookie and internal xa
>>> should specify the cachability mode..
>>
>> That was my intention as well. The driver returns a "hint" flag to the user, but
>> the userspace library can override it with its own cachability flags.
>> However, I now notice EFA lost that functionality when it was merged as the
>> mmap function looks at 'entry->mmap_flag' (hint) instead of the given offset
>> flag:
>>
>> static int __efa_mmap(struct efa_dev *dev, struct efa_ucontext *ucontext,
>> struct vm_area_struct *vma, u64 key, u64 length) {
>> struct efa_mmap_entry *entry;
>> unsigned long va;
>> u64 pfn;
>> int err;
>>
>> entry = mmap_entry_get(dev, ucontext, key, length);
>> if (!entry) {
>> ibdev_dbg(&dev->ibdev, "key[%#llx] does not have valid
>> entry\n",
>> key);
>> return -EINVAL;
>> }
>>
>> ibdev_dbg(&dev->ibdev,
>> "Mapping address[%#llx], length[%#llx],
>> mmap_flag[%d]\n",
>> entry->address, length, entry->mmap_flag);
>>
>> pfn = entry->address >> PAGE_SHIFT;
>> switch (entry->mmap_flag) {
>> ^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> case EFA_MMAP_IO_NC:
>> err = rdma_user_mmap_io(&ucontext->ibucontext, vma,
>> pfn, length,
>> pgprot_noncached(vma-
>>> vm_page_prot));
>> break;
>> case EFA_MMAP_IO_WC:
>> err = rdma_user_mmap_io(&ucontext->ibucontext, vma,
>> pfn, length,
>> pgprot_writecombine(vma-
>>> vm_page_prot));
>> break;
>> case EFA_MMAP_DMA_PAGE:
>> for (va = vma->vm_start; va < vma->vm_end;
>> va += PAGE_SIZE, pfn++) {
>> err = vm_insert_page(vma, va, pfn_to_page(pfn));
>> if (err)
>> break;
>> }
>> break;
>> default:
>> err = -EINVAL;
>> }
>>
>> if (err) {
>> ibdev_dbg(
>> &dev->ibdev,
>> "Couldn't mmap address[%#llx] length[%#llx]
>> mmap_flag[%d] err[%d]\n",
>> entry->address, length, entry->mmap_flag, err);
>> return err;
>> }
>>
>> return 0;
>> }
>>
>> Another issue is that these flags aren't exposed in an ABI file, so a userspace
>> library can't really make use of it in current state.
>
> Can you give an example of a use-case where the user would want to override the kernel hint ?
> Do you plan on changing this and exposing the flags to ABI?
I don't have an example of such use case currently, I thought it's good to have
such capability.
Regarding the ABI, I guess it depends on how this RFC is going to end up.
