> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Gal Pressman
> Sent: Wednesday, July 3, 2019 1:58 PM
> To: Michal Kalderon <mkalderon@xxxxxxxxxxx>; Jason Gunthorpe
> <jgg@xxxxxxxx>
> Cc: dledford@xxxxxxxxxx; leon@xxxxxxxxxx; sleybo@xxxxxxxxxx; Ariel Elior
> <aelior@xxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
> Subject: Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function
>
> 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.
OK, thanks. I think as a start, I'll send a patch series with just the helper functions
for handling the mmap_xa.
I'll send the patch series as a base for the qedr doorbell recovery patch.
And I'll be happy to continue discussion on whether we want the entire mmap
To be generic or not.
Thanks,
Michal
