> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> Sent: Tuesday, June 25, 2019 11:04 PM
>
> External Email
>
> ----------------------------------------------------------------------
> On Mon, Jun 24, 2019 at 01:28:08PM +0300, Michal Kalderon wrote:
>
> > +/* Map the kernel doorbell recovery memory entry */ int
> > +qedr_mmap_db_rec(struct vm_area_struct *vma) {
> > + unsigned long len = vma->vm_end - vma->vm_start;
> > +
> > + return remap_pfn_range(vma, vma->vm_start,
> > + vma->vm_pgoff,
> > + len, vma->vm_page_prot);
> > +}
> > +
> > int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct
> > *vma) {
> > struct qedr_ucontext *ucontext = get_qedr_ucontext(context); @@
> > -390,6 +446,8 @@ int qedr_mmap(struct ib_ucontext *context, struct
> vm_area_struct *vma)
> > unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
> > unsigned long len = (vma->vm_end - vma->vm_start);
> > unsigned long dpi_start;
> > + struct qedr_mm *mm;
> > + int rc;
> >
> > dpi_start = dev->db_phys_addr + (ucontext->dpi *
> > ucontext->dpi_size);
> >
> > @@ -405,29 +463,28 @@ int qedr_mmap(struct ib_ucontext *context,
> struct vm_area_struct *vma)
> > return -EINVAL;
> > }
> >
> > - if (!qedr_search_mmap(ucontext, phys_addr, len)) {
> > - DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not
> authorized\n",
> > + mm = qedr_remove_mmap(ucontext, phys_addr, len);
> > + if (!mm) {
> > + DP_ERR(dev, "failed to remove mmap, vm_pgoff=0x%lx\n",
> > vma->vm_pgoff);
> > return -EINVAL;
> >
>
> This is so gross, please follow the pattern other drivers use for managing the
> mmap cookie
>
> In fact I am sick of seeing drivers wrongly re-implement this, so you now get
> the job to make some proper core helpers to manage mmap cookies for
> drivers.
>
> The EFA driver is probably the best example, I suggest you move that code to
> a common file in ib-core and use it here instead of redoing yet again another
> broken version.
>
> siw has another copy of basically the same thing.
Hi Jason,
Thanks for the feedback. I looked at the efa driver and it seems perhaps we can make
the entire mmap code common and not just some core helpers.
I will send out an RFC with modifying the EFA + QEDR driver accordingly to get your input.
Thanks,
Michal
>
> > +static int qedr_init_user_db_rec(struct ib_udata *udata,
> > + struct qedr_dev *dev, struct qedr_userq *q,
> > + bool requires_db_rec)
> > +{
> > + struct qedr_ucontext *uctx =
> > + rdma_udata_to_drv_context(udata, struct qedr_ucontext,
> > + ibucontext);
> > +
> > + /* Aborting for non doorbell userqueue (SRQ) or non-supporting lib
> */
> > + if (requires_db_rec == 0 || !uctx->db_rec)
> > + return 0;
> > +
> > + /* Allocate a page for doorbell recovery, add to mmap ) */
> > + q->db_rec_data = (void *)get_zeroed_page(GFP_KERNEL);
>
> Pages obtained by get_zeroed_page shuld not be inserted by
> remap_pfn_range, those cases need to use vm_insert_page instead.
Thanks.
>
> > struct qedr_alloc_ucontext_resp {
> > __aligned_u64 db_pa;
> > @@ -74,6 +83,7 @@ struct qedr_create_cq_uresp {
> > __u32 db_offset;
> > __u16 icid;
> > __u16 reserved;
> > + __u64 db_rec_addr;
> > };
>
> All uapi u64s need to be __aligned_u64 in this file.
>
> > +/* doorbell recovery entry allocated and populated by userspace
> > +doorbelling
> > + * entities and mapped to kernel. Kernel uses this to register
> > +doorbell
> > + * information with doorbell drop recovery mechanism.
> > + */
> > +struct qedr_user_db_rec {
> > + __aligned_u64 db_data; /* doorbell data */ };
>
> like this one :\
>
> Jason
