Date:   Mon, 6 Aug 2018 11:12:52 +0800
From:   Kenneth Lee <liguozhu@...ilicon.com>
To:     Jerome Glisse <jglisse@...hat.com>
CC:     "Tian, Kevin" <kevin.tian@...el.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Zaibo Xu <xuzaibo@...wei.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "Kumar, Sanjay K" <sanjay.k.kumar@...el.com>,
        Hao Fang <fanghao11@...wei.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxarm@...wei.com" <linuxarm@...wei.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        Philippe Ombredanne <pombredanne@...b.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kenneth Lee <nek.in.cn@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        "linux-accelerators@...ts.ozlabs.org" 
        <linux-accelerators@...ts.ozlabs.org>
Subject: Re: [RFC PATCH 0/7] A General Accelerator Framework, WarpDrive

On Fri, Aug 03, 2018 at 10:39:44AM -0400, Jerome Glisse wrote:
> Date: Fri, 3 Aug 2018 10:39:44 -0400
> From: Jerome Glisse <jglisse@...hat.com>
> To: Kenneth Lee <liguozhu@...ilicon.com>
> CC: "Tian, Kevin" <kevin.tian@...el.com>, Herbert Xu
>  <herbert@...dor.apana.org.au>, "kvm@...r.kernel.org"
>  <kvm@...r.kernel.org>, Jonathan Corbet <corbet@....net>, Greg
>  Kroah-Hartman <gregkh@...uxfoundation.org>, Zaibo Xu <xuzaibo@...wei.com>,
>  "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "Kumar, Sanjay K"
>  <sanjay.k.kumar@...el.com>, Hao Fang <fanghao11@...wei.com>,
>  "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
>  "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
>  "linuxarm@...wei.com" <linuxarm@...wei.com>, Alex Williamson
>  <alex.williamson@...hat.com>, "linux-crypto@...r.kernel.org"
>  <linux-crypto@...r.kernel.org>, Philippe Ombredanne
>  <pombredanne@...b.com>, Thomas Gleixner <tglx@...utronix.de>, Kenneth Lee
>  <nek.in.cn@...il.com>, "David S . Miller" <davem@...emloft.net>,
>  "linux-accelerators@...ts.ozlabs.org"
>  <linux-accelerators@...ts.ozlabs.org>
> Subject: Re: [RFC PATCH 0/7] A General Accelerator Framework, WarpDrive
> User-Agent: Mutt/1.10.0 (2018-05-17)
> Message-ID: <20180803143944.GA4079@...hat.com>
> 
> On Fri, Aug 03, 2018 at 11:47:21AM +0800, Kenneth Lee wrote:
> > On Thu, Aug 02, 2018 at 10:22:43AM -0400, Jerome Glisse wrote:
> > > Date: Thu, 2 Aug 2018 10:22:43 -0400
> > > From: Jerome Glisse <jglisse@...hat.com>
> > > To: Kenneth Lee <liguozhu@...ilicon.com>
> > > CC: "Tian, Kevin" <kevin.tian@...el.com>, Hao Fang <fanghao11@...wei.com>,
> > >  Alex Williamson <alex.williamson@...hat.com>, Herbert Xu
> > >  <herbert@...dor.apana.org.au>, "kvm@...r.kernel.org"
> > >  <kvm@...r.kernel.org>, Jonathan Corbet <corbet@....net>, Greg
> > >  Kroah-Hartman <gregkh@...uxfoundation.org>, Zaibo Xu <xuzaibo@...wei.com>,
> > >  "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "Kumar, Sanjay K"
> > >  <sanjay.k.kumar@...el.com>, Kenneth Lee <nek.in.cn@...il.com>,
> > >  "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
> > >  "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
> > >  "linuxarm@...wei.com" <linuxarm@...wei.com>,
> > >  "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>, Philippe
> > >  Ombredanne <pombredanne@...b.com>, Thomas Gleixner <tglx@...utronix.de>,
> > >  "David S . Miller" <davem@...emloft.net>,
> > >  "linux-accelerators@...ts.ozlabs.org"
> > >  <linux-accelerators@...ts.ozlabs.org>
> > > Subject: Re: [RFC PATCH 0/7] A General Accelerator Framework, WarpDrive
> > > User-Agent: Mutt/1.10.0 (2018-05-17)
> > > Message-ID: <20180802142243.GA3481@...hat.com>
> > > 
> > > On Thu, Aug 02, 2018 at 12:05:57PM +0800, Kenneth Lee wrote:
> > > > On Thu, Aug 02, 2018 at 02:33:12AM +0000, Tian, Kevin wrote:
> > > > > Date: Thu, 2 Aug 2018 02:33:12 +0000
> > > > > > From: Jerome Glisse
> > > > > > On Wed, Aug 01, 2018 at 06:22:14PM +0800, Kenneth Lee wrote:
> > > > > > > From: Kenneth Lee <liguozhu@...ilicon.com>
> > > > > > >
> > > > > > > WarpDrive is an accelerator framework to expose the hardware
> > > > > > capabilities
> > > > > > > directly to the user space. It makes use of the exist vfio and vfio-mdev
> > > > > > > facilities. So the user application can send request and DMA to the
> > > > > > > hardware without interaction with the kernel. This remove the latency
> > > > > > > of syscall and context switch.
> > > > > > >
> > > > > > > The patchset contains documents for the detail. Please refer to it for
> > > > > > more
> > > > > > > information.
> > > > > > >
> > > > > > > This patchset is intended to be used with Jean Philippe Brucker's SVA
> > > > > > > patch [1] (Which is also in RFC stage). But it is not mandatory. This
> > > > > > > patchset is tested in the latest mainline kernel without the SVA patches.
> > > > > > > So it support only one process for each accelerator.
> > > > > > >
> > > > > > > With SVA support, WarpDrive can support multi-process in the same
> > > > > > > accelerator device.  We tested it in our SoC integrated Accelerator (board
> > > > > > > ID: D06, Chip ID: HIP08). A reference work tree can be found here: [2].
> > > > > > 
> > > > > > I have not fully inspected things nor do i know enough about
> > > > > > this Hisilicon ZIP accelerator to ascertain, but from glimpsing
> > > > > > at the code it seems that it is unsafe to use even with SVA due
> > > > > > to the doorbell. There is a comment talking about safetyness
> > > > > > in patch 7.
> > > > > > 
> > > > > > Exposing thing to userspace is always enticing, but if it is
> > > > > > a security risk then it should clearly say so and maybe a
> > > > > > kernel boot flag should be necessary to allow such device to
> > > > > > be use.
> > > > > > 
> > > > 
> > > > But doorbell is just a notification. Except for DOS (to make hardware busy) it
> > > > cannot actually take or change anything from the kernel space. And the DOS
> > > > problem can be always taken as the problem that a group of processes share the
> > > > same kernel entity.
> > > > 
> > > > In the coming HIP09 hardware, the doorbell will come with a random number so
> > > > only the process who allocated the queue can knock it correctly.
> > > 
> > > When doorbell is ring the hardware start fetching commands from
> > > the queue and execute them ? If so than a rogue process B might
> > > ring the doorbell of process A which would starts execution of
> > > random commands (ie whatever random memory value there is left
> > > inside the command buffer memory, could be old commands i guess).
> > > 
> > > If this is not how this doorbell works then, yes it can only do
> > > a denial of service i guess. Issue i have with doorbell is that
> > > i have seen 10 differents implementations in 10 differents hw
> > > and each are different as to what ringing or value written to the
> > > doorbell does. It is painfull to track what is what for each hw.
> > > 
> > 
> > In our implementation, doorbell is simply a notification, just like an interrupt
> > to the accelerator. The command is all about what's in the queue.
> > 
> > I agree that there is no simple and standard way to track the shared IO space.
> > But I think we have to trust the driver in some way. If the driver is malicious,
> > even a simple ioctl can become an attack.
> 
> Trusting kernel space driver is fine, trusting user space driver is
> not in my view. AFAICT every driver developer so far always made
> sure that someone could not abuse its device to do harmfull thing to
> other process.
> 

Fully agree. That is why this driver shares only the doorbell space. There is
only the doorbell is shared in the whole page, nothing else.

Maybe you are concerning the user driver will give malicious command to the
hardware? But these commands cannot influence the other process. If we can trust
the hardware design, the process cannot do any harm.

> 
> > > > > > My more general question is do we want to grow VFIO to become
> > > > > > a more generic device driver API. This patchset adds a command
> > > > > > queue concept to it (i don't think it exist today but i have
> > > > > > not follow VFIO closely).
> > > > > > 
> > > > 
> > > > The thing is, VFIO is the only place to support DMA from user land. If we don't
> > > > put it here, we have to create another similar facility to support the same.
> > > 
> > > No it is not, network device, GPU, block device, ... they all do
> > > support DMA. The point i am trying to make here is that even in
> > 
> > Sorry, wait a minute, are we talking the same thing? I meant "DMA from user
> > land", not "DMA from kernel driver". To do that we have to manipulate the
> > IOMMU(Unit). I think it can only be done by default_domain or vfio domain. Or
> > the user space have to directly access the IOMMU.
> 
> GPU do DMA in the sense that you pass to the kernel a valid
> virtual address (kernel driver do all the proper check) and
> then you can use the GPU to copy from or to that range of
> virtual address. Exactly how you want to use this compression
> engine. It does not rely on SVM but SVM going forward would
> still be the prefered option.
> 

No, SVM is not the reason why we rely on Jean's SVM(SVA) series. We rely on
Jean's series because of multi-process (PASID or substream ID) support.

But of couse, WarpDrive can still benefit from the SVM feature.

> 
> > > your mechanisms the userspace must have a specific userspace
> > > drivers for each hardware and thus there are virtually no
> > > differences between having this userspace driver open a device
> > > file in vfio or somewhere else in the device filesystem. This is
> > > just a different path.
> > > 
> > 
> > The basic problem WarpDrive want to solve it to avoid syscall. This is important
> > to accelerators. We have some data here:
> > https://www.slideshare.net/linaroorg/progress-and-demonstration-of-wrapdrive-a-accelerator-framework-sfo17317
> > 
> > (see page 3)
> > 
> > The performance is different on using kernel and user drivers.
> 
> Yes and example i point to is exactly that. You have a one time setup
> cost (creating command buffer binding PASID with command buffer and
> couple other setup steps). Then userspace no longer have to do any
> ioctl to schedule work on the GPU. It is all down from userspace and
> it use a doorbell to notify hardware when it should go look at command
> buffer for new thing to execute.
> 
> My point stands on that. You have existing driver already doing so
> with no new framework and in your scheme you need a userspace driver.
> So i do not see the value add, using one path or the other in the
> userspace driver is litteraly one line to change.
> 

Sorry, I'd got confuse here. I partially agree that the user driver is
redundance of kernel driver. (But for WarpDrive, the kernel driver is a full
driver include all preparation and setup stuff for the hardware, the user driver
is simply to send request and receive answer). Yes, it is just a choice of path.
But the user path is faster if the request come from use space. And to do that,
we need user land DMA support. Then why is it invaluable to let VFIO involved?

> 
> > And we also believe the hardware interface can become standard after sometime.
> > Some companies have started to do this (such ARM's Revere). But before that, we
> > should have a software channel for it.
> 
> I hope it does, but right now for every single piece of hardware you
> will need a specific driver (i am ignoring backward compatible hardware
> evolution as this is a thing that do exist).
> 
> Even if down the road for every class of hardware you can use the same
> driver, i am not sure what the value add is to do it inside VFIO versus
> a class of device driver (like USB, PCIE, DRM aka GPU, ...) ie you would
> have a compression class (/dev/compress/*) a encryption one, ...
> 
>
> > > So this is why i do not see any benefit to having all drivers with
> > > SVM (can we please use SVM and not SVA as SVM is what have been use
> > > in more places so far).
> > > 
> > 
> > Personally, we don't care what name to be used. I used SVM when I start this
> > work. And then Jean said SVM had been used by AMD as Secure Virtual Machine. So
> > he called it SVA. And now... who should I follow? :)
> 
> I think Intel call it SVM too, i do not have any strong preference
> beside have only one to remember :)

OK. Then let's call it SVM for now.

> 
> Cheers,
> Jérôme

-- 
			-Kenneth(Hisilicon)

================================================================================
本邮件及其附件含有华为公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁
止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中
的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
This e-mail and its attachments contain confidential information from HUAWEI,
which is intended only for the person or entity whose address is listed above.
Any use of the 
information contained herein in any way (including, but not limited to, total or
partial disclosure, reproduction, or dissemination) by persons other than the
intended 
recipient(s) is prohibited. If you receive this e-mail in error, please notify
the sender by phone or email immediately and delete it!

Powered by blists - more mailing lists