Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191010140615.26460-1-oshpigelman@habana.ai> <20191010140950.GA27176@infradead.org>
 <AM6PR0202MB338206146804E2E2BC18C67FB8940@AM6PR0202MB3382.eurprd02.prod.outlook.com>
 <20191011081055.GA9052@infradead.org>
In-Reply-To: <20191011081055.GA9052@infradead.org>
From:   Oded Gabbay <oded.gabbay@gmail.com>
Date:   Fri, 11 Oct 2019 12:19:36 +0300
Message-ID: <CAFCwf11hYWYNeROT8zpW+fcALijcTUuGVk-NoWvxzCORvd+dew@mail.gmail.com>
Subject: Re: [PATCH 1/2] habanalabs: support vmalloc memory mapping
To:     Christoph Hellwig <hch@infradead.org>
Cc:     Omer Shpigelman <oshpigelman@habana.ai>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 11, 2019 at 11:10 AM Christoph Hellwig <hch@infradead.org> wrot=
e:
>
> On Thu, Oct 10, 2019 at 07:54:07PM +0000, Omer Shpigelman wrote:
> > The is_vmalloc_addr checks are for user pointers and for memory which w=
as allocated by the driver with vmalloc_user.
>
> This does not make any sense whatsoever.  vmalloc_user returns a kernel
> address, it just does a GFP_USER instead of GFP_KERNEL allocation, which
> is just accounting differences.
>
> > > > Mapping vmalloc memory is needed for Gaudi ASIC.
> > >
> > > How does that ASIC pass in the vmalloc memory?  I don't fully underst=
and
> > > the code, but it seems like the addresses are fed from ioctl, which m=
eans
> > > they only come from userspace.
> >
> > The user pointers are indeed fed from ioctl for DMA purpose, but as I w=
rote above the vmalloc memory is allocated by the driver with vmalloc_user =
which will be mmapped later on in order to create a shared buffer between t=
he driver and the userspace process.
>
> Again, you can't pass pointers obtained from vmalloc* to userspace.  You
> can map the underlying pages into user pagetables, but is_vmalloc_addr
> won't know that.  I think you guys need to read up on virtual memory 101
> first and then come back and actually explain what you are trying to do.

Hi Christoph,
I think the confusion here is because this is only a pre-requisite
patch, which doesn't show the full code of how we use vmalloc_user.
So I want to describe the full flow here and please tell me if and
what we are doing wrong.

We first allocate, using vmalloc_user, a certain memory block that
will be used by the ASIC and the user (ASIC is producer, user is
consumer).
After we use vmalloc_user, we map the *kernel* pointer we got from the
vmalloc_user() to the ASIC MMU. We reuse our driver's generic code
path to map host memory to ASIC MMU and that's why we need the patch
above. The user does NOT send us the pointer. He doesn't have this
pointer. It is internal to the kernel driver. To do this reuse, we
added a call to the is_vmalloc_addr(), so the function will know if it
is called to work on user pointers, or on vmalloc *kernel* pointers.

What the user does get is an opaque handle. Later he calls mmap() with
our FD and this handle. In the driver, we do remap_vmalloc_range() on
the *kernel* pointer we retrieved using the opaque handle we got from
the user, and return a valid user-space process address that points to
this memory block, so the user can access it.

AFAIK from my GPU days, this is totally valid. The user does NOT see
kernel pointers. There is total separation between kernel and user
pointers. User get pointers only by calling mmap(). We NEVER return a
pointer to a user in an IOCTL. Only through mmap.
Please tell me if you see a problem here. If you need more details,
I'll happy to provide them.

Side note:
The reason the driver allocates the memory block and not the user
itself (like we did so far in all our code), is because this block is
mapped to our ASIC internal MMU using a priviliged ASID. I don't want
to give the user a uapi that will enable him to request to map memory
to the ASIC using a priviliged ASID. Therefore, the kernel driver does
it internally.

Thanks,
Oded