Re: [PATCH 2/2] uacce: add uacce module
On Tue, Aug 27, 2019 at 07:42:08PM +0800, Kenneth Lee wrote:
> On Mon, Aug 26, 2019 at 06:29:10AM +0200, Greg Kroah-Hartman wrote:
> > Date: Mon, 26 Aug 2019 06:29:10 +0200
> > From: Greg Kroah-Hartman
> > To: Kenneth Lee
> > CC: zhangfei , Arnd Bergmann ,
> > linux-accelerat...@lists.ozlabs.org, linux-kernel@vger.kernel.org, Zaibo
> > Xu , Zhou Wang
> > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > User-Agent: Mutt/1.12.1 (2019-06-15)
> > Message-ID: <20190826042910.ga26...@kroah.com>
> >
> > On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> > > On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > > > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > > > From: Greg Kroah-Hartman
> > > > To: zhangfei
> > > > CC: Arnd Bergmann , linux-accelerat...@lists.ozlabs.org,
> > > > linux-kernel@vger.kernel.org, Kenneth Lee ,
> > > > Zaibo
> > > > Xu , Zhou Wang
> > > > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > > > User-Agent: Mutt/1.12.1 (2019-06-15)
> > > > Message-ID: <20190821160542.ga14...@kroah.com>
> > > >
> > > > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > > > >
> > > > >
> > > > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com
> > > > > > wrote:
> > > > > > > Hi, Greg
> > > > > > >
> > > > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao
> > > > > > > > > > wrote:
> > > > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > > > +{
> > > > > > > > > > > + int ret;
> > > > > > > > > > > +
> > > > > > > > > > > + if (!uacce->pdev) {
> > > > > > > > > > > + pr_debug("uacce parent device not set\n");
> > > > > > > > > > > + return -ENODEV;
> > > > > > > > > > > + }
> > > > > > > > > > > +
> > > > > > > > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > > > + dev_warn(uacce->pdev,
> > > > > > > > > > > + "Register to noiommu mode, which
> > > > > > > > > > > export kernel data to user space and may vulnerable to
> > > > > > > > > > > attack");
> > > > > > > > > > > + }
> > > > > > > > > > THat is odd, why even offer this feature then if it is a
> > > > > > > > > > major issue?
> > > > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > > > >
> > > > > > > > > In this mode, app use ioctl to get dma_handle from
> > > > > > > > > dma_alloc_coherent.
> > > > > > > > That's odd, why not use the other default apis to do that?
> > > > > > > >
> > > > > > > > > It does not matter iommu is enabled or not.
> > > > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so
> > > > > > > > > we added warning here, is it required?
> > > > > > > > You should use the other documentated apis for this, don't
> > > > > > > > create your
> > > > > > > > own.
> > > > > > > I am sorry, not understand here.
> > > > > > > Do you mean there is a standard ioctl or standard api in user
> > > > > > > space, it can
> > > > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > > > There should be a standard way to get such a handle from userspace
> > > > > > t
Re: [PATCH 2/2] uacce: add uacce module
On Mon, Aug 26, 2019 at 06:29:10AM +0200, Greg Kroah-Hartman wrote:
> Date: Mon, 26 Aug 2019 06:29:10 +0200
> From: Greg Kroah-Hartman
> To: Kenneth Lee
> CC: zhangfei , Arnd Bergmann ,
> linux-accelerat...@lists.ozlabs.org, linux-kernel@vger.kernel.org, Zaibo
> Xu , Zhou Wang
> Subject: Re: [PATCH 2/2] uacce: add uacce module
> User-Agent: Mutt/1.12.1 (2019-06-15)
> Message-ID: <20190826042910.ga26...@kroah.com>
>
> On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> > On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > > From: Greg Kroah-Hartman
> > > To: zhangfei
> > > CC: Arnd Bergmann , linux-accelerat...@lists.ozlabs.org,
> > > linux-kernel@vger.kernel.org, Kenneth Lee , Zaibo
> > > Xu , Zhou Wang
> > > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > > User-Agent: Mutt/1.12.1 (2019-06-15)
> > > Message-ID: <20190821160542.ga14...@kroah.com>
> > >
> > > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > > >
> > > >
> > > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com
> > > > > wrote:
> > > > > > Hi, Greg
> > > > > >
> > > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > > +{
> > > > > > > > > > + int ret;
> > > > > > > > > > +
> > > > > > > > > > + if (!uacce->pdev) {
> > > > > > > > > > + pr_debug("uacce parent device not set\n");
> > > > > > > > > > + return -ENODEV;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > > + dev_warn(uacce->pdev,
> > > > > > > > > > +"Register to noiommu mode, which
> > > > > > > > > > export kernel data to user space and may vulnerable to
> > > > > > > > > > attack");
> > > > > > > > > > + }
> > > > > > > > > THat is odd, why even offer this feature then if it is a
> > > > > > > > > major issue?
> > > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > > >
> > > > > > > > In this mode, app use ioctl to get dma_handle from
> > > > > > > > dma_alloc_coherent.
> > > > > > > That's odd, why not use the other default apis to do that?
> > > > > > >
> > > > > > > > It does not matter iommu is enabled or not.
> > > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we
> > > > > > > > added warning here, is it required?
> > > > > > > You should use the other documentated apis for this, don't create
> > > > > > > your
> > > > > > > own.
> > > > > > I am sorry, not understand here.
> > > > > > Do you mean there is a standard ioctl or standard api in user
> > > > > > space, it can
> > > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > > There should be a standard way to get such a handle from userspace
> > > > > today. Isn't that what the ion interface does? DRM also does this,
> > > > > as
> > > > > does UIO I think.
> > > > Thanks Greg,
> > > > Still not find it, will do more search.
> > > > But this may introduce dependency in our lib, like depend on ion?
> > > > > Do you have a spec somewhere that shows exactly what you are trying to
> > > > > do here, along with exa
Re: [PATCH 2/2] uacce: add uacce module
On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > From: Greg Kroah-Hartman
> > To: zhangfei
> > CC: Arnd Bergmann , linux-accelerat...@lists.ozlabs.org,
> > linux-kernel@vger.kernel.org, Kenneth Lee , Zaibo
> > Xu , Zhou Wang
> > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > User-Agent: Mutt/1.12.1 (2019-06-15)
> > Message-ID: <20190821160542.ga14...@kroah.com>
> >
> > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > >
> > >
> > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com
> > > > wrote:
> > > > > Hi, Greg
> > > > >
> > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > +{
> > > > > > > > > + int ret;
> > > > > > > > > +
> > > > > > > > > + if (!uacce->pdev) {
> > > > > > > > > + pr_debug("uacce parent device not set\n");
> > > > > > > > > + return -ENODEV;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > + dev_warn(uacce->pdev,
> > > > > > > > > + "Register to noiommu mode, which
> > > > > > > > > export kernel data to user space and may vulnerable to
> > > > > > > > > attack");
> > > > > > > > > + }
> > > > > > > > THat is odd, why even offer this feature then if it is a major
> > > > > > > > issue?
> > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > >
> > > > > > > In this mode, app use ioctl to get dma_handle from
> > > > > > > dma_alloc_coherent.
> > > > > > That's odd, why not use the other default apis to do that?
> > > > > >
> > > > > > > It does not matter iommu is enabled or not.
> > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we
> > > > > > > added warning here, is it required?
> > > > > > You should use the other documentated apis for this, don't create
> > > > > > your
> > > > > > own.
> > > > > I am sorry, not understand here.
> > > > > Do you mean there is a standard ioctl or standard api in user space,
> > > > > it can
> > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > There should be a standard way to get such a handle from userspace
> > > > today. Isn't that what the ion interface does? DRM also does this, as
> > > > does UIO I think.
> > > Thanks Greg,
> > > Still not find it, will do more search.
> > > But this may introduce dependency in our lib, like depend on ion?
> > > > Do you have a spec somewhere that shows exactly what you are trying to
> > > > do here, along with example userspace code? It's hard to determine it
> > > > given you only have one "half" of the code here and no users of the apis
> > > > you are creating.
> > > >
> > > The purpose is doing dma in user space.
> >
> > Oh no, please no. Are you _SURE_ you want to do this?
> >
> > Again, look at how ION does this and how the DMAbuff stuff is replacing
> > it. Use that api please instead, otherwise you will get it wrong and we
> > don't want to duplicate efforts.
> >
> > thanks,
> >
> > greg k-h
>
> Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
> https://zhuanlan.zhihu.com/p/79680889.
Putting that information into the changelog and kernel documentation is
a much better idea than putting it there.
> Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
> temporary solution to let some hardware which does not care the security issue
> to try WarpDrive for the first step. Some user do not care this much in
> embedded
> scenario. We saw VFIO use the same model so we also want to make a try. If you
> insist this is risky, we can remove it.
Why not just use vfio then?
And yes, for now, please remove it, if you are not requiring it.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> Date: Wed, 21 Aug 2019 09:05:42 -0700
> From: Greg Kroah-Hartman
> To: zhangfei
> CC: Arnd Bergmann , linux-accelerat...@lists.ozlabs.org,
> linux-kernel@vger.kernel.org, Kenneth Lee , Zaibo
> Xu , Zhou Wang
> Subject: Re: [PATCH 2/2] uacce: add uacce module
> User-Agent: Mutt/1.12.1 (2019-06-15)
> Message-ID: <20190821160542.ga14...@kroah.com>
>
> On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> >
> >
> > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com wrote:
> > > > Hi, Greg
> > > >
> > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > +{
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + if (!uacce->pdev) {
> > > > > > > > + pr_debug("uacce parent device not set\n");
> > > > > > > > + return -ENODEV;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > + dev_warn(uacce->pdev,
> > > > > > > > +"Register to noiommu mode, which
> > > > > > > > export kernel data to user space and may vulnerable to attack");
> > > > > > > > + }
> > > > > > > THat is odd, why even offer this feature then if it is a major
> > > > > > > issue?
> > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > >
> > > > > > In this mode, app use ioctl to get dma_handle from
> > > > > > dma_alloc_coherent.
> > > > > That's odd, why not use the other default apis to do that?
> > > > >
> > > > > > It does not matter iommu is enabled or not.
> > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we
> > > > > > added warning here, is it required?
> > > > > You should use the other documentated apis for this, don't create your
> > > > > own.
> > > > I am sorry, not understand here.
> > > > Do you mean there is a standard ioctl or standard api in user space, it
> > > > can
> > > > get dma_handle from dma_alloc_coherent from kernel?
> > > There should be a standard way to get such a handle from userspace
> > > today. Isn't that what the ion interface does? DRM also does this, as
> > > does UIO I think.
> > Thanks Greg,
> > Still not find it, will do more search.
> > But this may introduce dependency in our lib, like depend on ion?
> > > Do you have a spec somewhere that shows exactly what you are trying to
> > > do here, along with example userspace code? It's hard to determine it
> > > given you only have one "half" of the code here and no users of the apis
> > > you are creating.
> > >
> > The purpose is doing dma in user space.
>
> Oh no, please no. Are you _SURE_ you want to do this?
>
> Again, look at how ION does this and how the DMAbuff stuff is replacing
> it. Use that api please instead, otherwise you will get it wrong and we
> don't want to duplicate efforts.
>
> thanks,
>
> greg k-h
Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
https://zhuanlan.zhihu.com/p/79680889.
Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
temporary solution to let some hardware which does not care the security issue
to try WarpDrive for the first step. Some user do not care this much in embedded
scenario. We saw VFIO use the same model so we also want to make a try. If you
insist this is risky, we can remove it.
Thanks.
--
-Kenneth(Hisilicon)
Re: [PATCH 2/2] uacce: add uacce module
On Sat, Aug 24, 2019 at 08:53:01PM +0800, zhangfei wrote:
>
>
> On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
> > On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
> > > Hi, Greg
> > >
> > > On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
> > > > > > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > Shouldn't this function create the memory needed for this structure?
> > > > > > You are relying ont he caller to do it for you, why?
> > > > > I think you mean uacce structure here.
> > > > > Yes, currently we count on caller to prepare uacce structure and call
> > > > > uacce_register(uacce).
> > > > > We still think this method is simpler, prepare uacce, register uacce.
> > > > > And there are other system using the same method, like crypto
> > > > > (crypto_register_acomp), nand, etc.
> > > > crypto is not a subsystem to ever try to emulate :)
> > > >
> > > > You are creating a structure with a lifetime that you control, don't
> > > > have someone else create your memory, that's almost never what you want
> > > > to do. Most all driver subsystems create their own memory chunks for
> > > > what they need to do, it's a much better pattern.
> > > >
> > > > Especially when you get into pointer lifetime issues...
> > > OK, understand now, thanks for your patience.
> > > will use this instead.
> > > struct uacce_interface {
> > > char name[32];
> > > unsigned int flags;
> > > struct uacce_ops *ops;
> > > };
> > > struct uacce *uacce_register(struct device *dev, struct uacce_interface
> > > *interface);
> > What? Why do you need a structure? A pointer to the name and the ops
> > should be all that is needed, right?
> We are thinking transfer structure will be more flexible.
> And modify api later would be difficult, requiring many drivers modify
> together.
> Currently parameters need a flag, a pointer to the name, and ops, but in
> case more requirement from future drivers usage.
> Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure para
> can be set as static.
Ok, I can live with that, but it is a bit more complex. However, if you
are creating a new device structure, your "core" has to do it, do not
rely on the driver to do it for you as that is just lots of duplicated
logic everywhere. Not to mention a reference counting nightmare.
> > And 'dev' here is a pointer to the parent, right? Might want to make
> > that explicit in the name of the variable :)
> Yes, 'dev' is parent, will change to 'pdev', thanks.
Use "struct device *parent" it's much more obvious that way and does not
look like crazy hungarian notation :)
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
Hi, Greg
On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
+static int uacce_create_chrdev(struct uacce *uacce)
+{
+ int ret;
+
+ ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?
I think you mean uacce structure here.
Yes, currently we count on caller to prepare uacce structure and call
uacce_register(uacce).
We still think this method is simpler, prepare uacce, register uacce.
And there are other system using the same method, like crypto
(crypto_register_acomp), nand, etc.
crypto is not a subsystem to ever try to emulate :)
You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do. Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.
Especially when you get into pointer lifetime issues...
OK, understand now, thanks for your patience.
will use this instead.
struct uacce_interface {
char name[32];
unsigned int flags;
struct uacce_ops *ops;
};
struct uacce *uacce_register(struct device *dev, struct uacce_interface
*interface);
What? Why do you need a structure? A pointer to the name and the ops
should be all that is needed, right?
We are thinking transfer structure will be more flexible.
And modify api later would be difficult, requiring many drivers modify
together.
Currently parameters need a flag, a pointer to the name, and ops, but in
case more requirement from future drivers usage.
Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure
para can be set as static.
And 'dev' here is a pointer to the parent, right? Might want to make
that explicit in the name of the variable :)
Yes, 'dev' is parent, will change to 'pdev', thanks.
+
+static int uacce_dev_match(struct device *dev, void *data)
+{
+ if (dev->parent == data)
+ return -EBUSY;
There should be in-kernel functions for this now, no need for you to
roll your own.
Sorry, do not find this function.
Only find class_find_device, which still require match.
It is in linux-next, look there...
Suppose you mean the funcs: device_match_name,
device_match_of_node,device_match_devt etc.
Here we need dev->parent, there still no such func.
You should NEVER be matching on a parent. If so, your use of the driver
model is wrong :)
Remind me to really review the use of the driver core code in your next
submission of this series please, I think it needs it.
OK, thanks Greg.
Re: [PATCH 2/2] uacce: add uacce module
On Fri, 23 Aug 2019 17:21:33 +0800 zhangfei wrote: > Hi, Jonathan > > Thanks for your careful review and good suggestion. > Sorry for late response, I am checking one detail. > > On 2019/8/16 上午12:54, Jonathan Cameron wrote: > > On Wed, 14 Aug 2019 17:34:25 +0800 > > Zhangfei Gao wrote: > > > >> From: Kenneth Lee > >> > >> Uacce is the kernel component to support WarpDrive accelerator > >> framework. It provides register/unregister interface for device drivers > >> to expose their hardware resource to the user space. The resource is > >> taken as "queue" in WarpDrive. > > It's a bit confusing to have both the term UACCE and WarpDrive in here. > > I'd just use the uacce name in all comments etc. > Yes, make sense > > > >> Uacce create a chrdev for every registration, the queue is allocated to > >> the process when the chrdev is opened. Then the process can access the > >> hardware resource by interact with the queue file. By mmap the queue > >> file space to user space, the process can directly put requests to the > >> hardware without syscall to the kernel space. > >> > >> Uacce also manages unify addresses between the hardware and user space > >> of the process. So they can share the same virtual address in the > >> communication. > >> > >> Signed-off-by: Kenneth Lee > >> Signed-off-by: Zaibo Xu > >> Signed-off-by: Zhou Wang > >> Signed-off-by: Zhangfei Gao > > I would strip this back to which ever case is of most interest (SVA I > > guess?) > > and only think about adding support for the others if necessary at a later > > date. > > (or in later patches). > Do you mean split the patch and send sva part first? Yes. Either send them as two series with SVA only in the first one, or a single series with SVA only in the early patches. I want to be able to review one case first then only consider what needs to be added for the others. Thanks, Jonathan
Re: [PATCH 2/2] uacce: add uacce module
On Fri, 23 Aug 2019 17:21:33 +0800
zhangfei wrote:
> Hi, Jonathan
Hi zhangfei,
>
> Thanks for your careful review and good suggestion.
> Sorry for late response, I am checking one detail.
I have reviews on patches from years ago that I still haven't replied to ;)
>
> On 2019/8/16 上午12:54, Jonathan Cameron wrote:
> > On Wed, 14 Aug 2019 17:34:25 +0800
> > Zhangfei Gao wrote:
> >
> >> From: Kenneth Lee
> >>
> >> Uacce is the kernel component to support WarpDrive accelerator
> >> framework. It provides register/unregister interface for device drivers
> >> to expose their hardware resource to the user space. The resource is
> >> taken as "queue" in WarpDrive.
> > It's a bit confusing to have both the term UACCE and WarpDrive in here.
> > I'd just use the uacce name in all comments etc.
> Yes, make sense
> >
> >> Uacce create a chrdev for every registration, the queue is allocated to
> >> the process when the chrdev is opened. Then the process can access the
> >> hardware resource by interact with the queue file. By mmap the queue
> >> file space to user space, the process can directly put requests to the
> >> hardware without syscall to the kernel space.
> >>
> >> Uacce also manages unify addresses between the hardware and user space
> >> of the process. So they can share the same virtual address in the
> >> communication.
> >>
> >> Signed-off-by: Kenneth Lee
> >> Signed-off-by: Zaibo Xu
> >> Signed-off-by: Zhou Wang
> >> Signed-off-by: Zhangfei Gao
> > I would strip this back to which ever case is of most interest (SVA I
> > guess?)
> > and only think about adding support for the others if necessary at a later
> > date.
> > (or in later patches).
> Do you mean split the patch and send sva part first?
Either:
1) SVA only in the first series, second series can do other options.
2) Patch N for SVA only, N+1... for other features.
I don't mind which, but I want to be able to see just one case and
review that before taking into account the affect of the more complex cases.
> >> +
> >> +static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
> >> +{
> >> + int gfp_mask = GFP_ATOMIC | __GFP_ZERO;
> > More readable to just have this inline.
> Yes, all right.
> >
> >> + int i, j;
> >> +
...
> >> +static int uacce_set_iommu_domain(struct uacce *uacce)
> >> +{
> >> + struct iommu_domain *domain;
> >> + struct iommu_group *group;
> >> + struct device *dev = uacce->pdev;
> >> + bool resv_msi;
> >> + phys_addr_t resv_msi_base = 0;
> >> + int ret;
> >> +
> >> + if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
> >> + (uacce->flags & UACCE_DEV_PASID))
> >> + return 0;
> >> +
> >> + /*
> >> + * We don't support multiple register for the same dev in RFC version ,
> >> + * will add it in formal version
> > So this effectively multiple complete uacce interfaces for one device.
> > Is there a known usecase for that?
> Here is preventing one device with multiple algorithm and register
> multi-times,
> and without sva, they can not be distinguished.
Isn't that a bug in the device driver?
> >> + */
> >> + ret = class_for_each_device(uacce_class, NULL, uacce->pdev,
> >> + uacce_dev_match);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* allocate and attach a unmanged domain */
...
Re: [PATCH 2/2] uacce: add uacce module
Hi, Jonathan
Thanks for your careful review and good suggestion.
Sorry for late response, I am checking one detail.
On 2019/8/16 上午12:54, Jonathan Cameron wrote:
On Wed, 14 Aug 2019 17:34:25 +0800
Zhangfei Gao wrote:
From: Kenneth Lee
Uacce is the kernel component to support WarpDrive accelerator
framework. It provides register/unregister interface for device drivers
to expose their hardware resource to the user space. The resource is
taken as "queue" in WarpDrive.
It's a bit confusing to have both the term UACCE and WarpDrive in here.
I'd just use the uacce name in all comments etc.
Yes, make sense
Uacce create a chrdev for every registration, the queue is allocated to
the process when the chrdev is opened. Then the process can access the
hardware resource by interact with the queue file. By mmap the queue
file space to user space, the process can directly put requests to the
hardware without syscall to the kernel space.
Uacce also manages unify addresses between the hardware and user space
of the process. So they can share the same virtual address in the
communication.
Signed-off-by: Kenneth Lee
Signed-off-by: Zaibo Xu
Signed-off-by: Zhou Wang
Signed-off-by: Zhangfei Gao
I would strip this back to which ever case is of most interest (SVA I guess?)
and only think about adding support for the others if necessary at a later date.
(or in later patches).
Do you mean split the patch and send sva part first?
+
+static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
+{
+ int gfp_mask = GFP_ATOMIC | __GFP_ZERO;
More readable to just have this inline.
Yes, all right.
+ int i, j;
+
+ qfr->pages = kcalloc(qfr->nr_pages, sizeof(*qfr->pages), gfp_mask);
kcalloc is always set to zero anyway.
OK
+
+static struct uacce_qfile_region *
+uacce_create_region(struct uacce_queue *q, struct vm_area_struct *vma,
+ enum uacce_qfrt type, unsigned int flags)
+{
+ struct uacce_qfile_region *qfr;
+ struct uacce *uacce = q->uacce;
+ unsigned long vm_pgoff;
+ int ret = -ENOMEM;
+
+ dev_dbg(uacce->pdev, "create qfr (type=%x, flags=%x)\n", type, flags);
+ qfr = kzalloc(sizeof(*qfr), GFP_ATOMIC);
+ if (!qfr)
+ return ERR_PTR(-ENOMEM);
+
+ qfr->type = type;
+ qfr->flags = flags;
+ qfr->iova = vma->vm_start;
+ qfr->nr_pages = vma_pages(vma);
+
+ if (vma->vm_flags & VM_READ)
+ qfr->prot |= IOMMU_READ;
+
+ if (vma->vm_flags & VM_WRITE)
+ qfr->prot |= IOMMU_WRITE;
+
+ if (flags & UACCE_QFRF_SELFMT) {
+ ret = uacce->ops->mmap(q, vma, qfr);
+ if (ret)
+ goto err_with_qfr;
+ return qfr;
+ }
+
+ /* allocate memory */
+ if (flags & UACCE_QFRF_DMA) {
+ dev_dbg(uacce->pdev, "allocate dma %d pages\n", qfr->nr_pages);
+ qfr->kaddr = dma_alloc_coherent(uacce->pdev, qfr->nr_pages <<
+ PAGE_SHIFT, &qfr->dma,
+ GFP_KERNEL);
+ if (!qfr->kaddr) {
+ ret = -ENOMEM;
+ goto err_with_qfr;
+ }
+ } else {
+ dev_dbg(uacce->pdev, "allocate %d pages\n", qfr->nr_pages);
+ ret = uacce_qfr_alloc_pages(qfr);
+ if (ret)
+ goto err_with_qfr;
+ }
+
+ /* map to device */
+ ret = uacce_queue_map_qfr(q, qfr);
+ if (ret)
+ goto err_with_pages;
+
+ /* mmap to user space */
+ if (flags & UACCE_QFRF_MMAP) {
+ if (flags & UACCE_QFRF_DMA) {
+
+ /* dma_mmap_coherent() requires vm_pgoff as 0
+* restore vm_pfoff to initial value for mmap()
+*/
+ dev_dbg(uacce->pdev, "mmap dma qfr\n");
+ vm_pgoff = vma->vm_pgoff;
+ vma->vm_pgoff = 0;
+ ret = dma_mmap_coherent(uacce->pdev, vma, qfr->kaddr,
+ qfr->dma,
+ qfr->nr_pages << PAGE_SHIFT);
Does setting vm_pgoff if (ret) make sense?
Since we need restore the environment, so restore vm_pgoff no matter
succeed or not.
+ vma->vm_pgoff = vm_pgoff;
+ } else {
+ ret = uacce_queue_mmap_qfr(q, qfr, vma);
+ }
+
+ if (ret)
+ goto err_with_mapped_qfr;
+ }
+
+ return qfr;
+
+err_with_mapped_qfr:
+ uacce_queue_unmap_qfr(q, qfr);
+err_with_pages:
+ if (flags & UACCE_QFRF_DMA)
+ dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
+ qfr->kaddr, qfr->dma);
+ else
+ uacce_qfr_free_pages(qfr);
+err_with_qfr:
+
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
>
>
> On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com wrote:
> > > Hi, Greg
> > >
> > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (!uacce->pdev) {
> > > > > > > + pr_debug("uacce parent device not set\n");
> > > > > > > + return -ENODEV;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > + dev_warn(uacce->pdev,
> > > > > > > + "Register to noiommu mode, which export kernel
> > > > > > > data to user space and may vulnerable to attack");
> > > > > > > + }
> > > > > > THat is odd, why even offer this feature then if it is a major
> > > > > > issue?
> > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > >
> > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > That's odd, why not use the other default apis to do that?
> > > >
> > > > > It does not matter iommu is enabled or not.
> > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added
> > > > > warning here, is it required?
> > > > You should use the other documentated apis for this, don't create your
> > > > own.
> > > I am sorry, not understand here.
> > > Do you mean there is a standard ioctl or standard api in user space, it
> > > can
> > > get dma_handle from dma_alloc_coherent from kernel?
> > There should be a standard way to get such a handle from userspace
> > today. Isn't that what the ion interface does? DRM also does this, as
> > does UIO I think.
> Thanks Greg,
> Still not find it, will do more search.
> But this may introduce dependency in our lib, like depend on ion?
> > Do you have a spec somewhere that shows exactly what you are trying to
> > do here, along with example userspace code? It's hard to determine it
> > given you only have one "half" of the code here and no users of the apis
> > you are creating.
> >
> The purpose is doing dma in user space.
Oh no, please no. Are you _SURE_ you want to do this?
Again, look at how ION does this and how the DMAbuff stuff is replacing
it. Use that api please instead, otherwise you will get it wrong and we
don't want to duplicate efforts.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com wrote:
Hi, Greg
On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
+int uacce_register(struct uacce *uacce)
+{
+ int ret;
+
+ if (!uacce->pdev) {
+ pr_debug("uacce parent device not set\n");
+ return -ENODEV;
+ }
+
+ if (uacce->flags & UACCE_DEV_NOIOMMU) {
+ add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+ dev_warn(uacce->pdev,
+"Register to noiommu mode, which export kernel data to user
space and may vulnerable to attack");
+ }
THat is odd, why even offer this feature then if it is a major issue?
UACCE_DEV_NOIOMMU maybe confusing here.
In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
That's odd, why not use the other default apis to do that?
It does not matter iommu is enabled or not.
In case iommu is disabled, it maybe dangerous to kernel, so we added warning
here, is it required?
You should use the other documentated apis for this, don't create your
own.
I am sorry, not understand here.
Do you mean there is a standard ioctl or standard api in user space, it can
get dma_handle from dma_alloc_coherent from kernel?
There should be a standard way to get such a handle from userspace
today. Isn't that what the ion interface does? DRM also does this, as
does UIO I think.
Thanks Greg,
Still not find it, will do more search.
But this may introduce dependency in our lib, like depend on ion?
Do you have a spec somewhere that shows exactly what you are trying to
do here, along with example userspace code? It's hard to determine it
given you only have one "half" of the code here and no users of the apis
you are creating.
The purpose is doing dma in user space.
If sva is supported, we can directly use malloc memory.
If sva is not supported, device can not recognize va, so we get
dma_handle via ioctl.
Sample user code is in
https://github.com/Linaro/warpdrive/blob/wdprd-v1-current/test/test_hisi_zip.c
hizip_wd_sched_init_cache:
if (sched->qs[0].dev_flags & UACCE_DEV_NOIOMMU) {
data_in = wd_get_pa_from_va(&sched->qs[0], wd_msg->data_in);
data_out = wd_get_pa_from_va(&sched->qs[0], wd_msg->data_out);
} else {
data_in = wd_msg->data_in;
data_out = wd_msg->data_out;
}
msg->source_addr_l = (__u64)data_in & 0x;
msg->source_addr_h = (__u64)data_in >> 32;
msg->dest_addr_l = (__u64)data_out & 0x;
msg->dest_addr_h = (__u64)data_out >> 32;
Have added some explanation in warpdrive.rst, in the first patch.
The device can also be declared as UACCE_DEV_NOIOMMU. It can be used
when the
device has no iommu support or the iommu is set in pass through mode.
In this
case, the driver should map address to device by itself with DMA API. The
ioctl(UACCE_CMD_GET_SS_DMA) can be used to get the physical address,
though it
is an untrusted and kernel-tainted behavior.
Thanks
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei@foxmail.com wrote:
> Hi, Greg
>
> On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > >
> > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > +int uacce_register(struct uacce *uacce)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + if (!uacce->pdev) {
> > > > > + pr_debug("uacce parent device not set\n");
> > > > > + return -ENODEV;
> > > > > + }
> > > > > +
> > > > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > + dev_warn(uacce->pdev,
> > > > > + "Register to noiommu mode, which export kernel
> > > > > data to user space and may vulnerable to attack");
> > > > > + }
> > > > THat is odd, why even offer this feature then if it is a major issue?
> > > UACCE_DEV_NOIOMMU maybe confusing here.
> > >
> > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > That's odd, why not use the other default apis to do that?
> >
> > > It does not matter iommu is enabled or not.
> > > In case iommu is disabled, it maybe dangerous to kernel, so we added
> > > warning here, is it required?
> > You should use the other documentated apis for this, don't create your
> > own.
> I am sorry, not understand here.
> Do you mean there is a standard ioctl or standard api in user space, it can
> get dma_handle from dma_alloc_coherent from kernel?
There should be a standard way to get such a handle from userspace
today. Isn't that what the ion interface does? DRM also does this, as
does UIO I think.
Do you have a spec somewhere that shows exactly what you are trying to
do here, along with example userspace code? It's hard to determine it
given you only have one "half" of the code here and no users of the apis
you are creating.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
>
>
> On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > +int uacce_register(struct uacce *uacce)
> > > +{
> > > + int ret;
> > > +
> > > + if (!uacce->pdev) {
> > > + pr_debug("uacce parent device not set\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > + dev_warn(uacce->pdev,
> > > + "Register to noiommu mode, which export kernel data to
> > > user space and may vulnerable to attack");
> > > + }
> > THat is odd, why even offer this feature then if it is a major issue?
> UACCE_DEV_NOIOMMU maybe confusing here.
>
> In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
That's odd, why not use the other default apis to do that?
> It does not matter iommu is enabled or not.
> In case iommu is disabled, it maybe dangerous to kernel, so we added warning
> here, is it required?
You should use the other documentated apis for this, don't create your
own.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
> Hi, Greg
>
> On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
> > > > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > Shouldn't this function create the memory needed for this structure?
> > > > You are relying ont he caller to do it for you, why?
> > > I think you mean uacce structure here.
> > > Yes, currently we count on caller to prepare uacce structure and call
> > > uacce_register(uacce).
> > > We still think this method is simpler, prepare uacce, register uacce.
> > > And there are other system using the same method, like crypto
> > > (crypto_register_acomp), nand, etc.
> > crypto is not a subsystem to ever try to emulate :)
> >
> > You are creating a structure with a lifetime that you control, don't
> > have someone else create your memory, that's almost never what you want
> > to do. Most all driver subsystems create their own memory chunks for
> > what they need to do, it's a much better pattern.
> >
> > Especially when you get into pointer lifetime issues...
> OK, understand now, thanks for your patience.
> will use this instead.
> struct uacce_interface {
> char name[32];
> unsigned int flags;
> struct uacce_ops *ops;
> };
> struct uacce *uacce_register(struct device *dev, struct uacce_interface
> *interface);
What? Why do you need a structure? A pointer to the name and the ops
should be all that is needed, right?
And 'dev' here is a pointer to the parent, right? Might want to make
that explicit in the name of the variable :)
> > > > > +
> > > > > +static int uacce_dev_match(struct device *dev, void *data)
> > > > > +{
> > > > > + if (dev->parent == data)
> > > > > + return -EBUSY;
> > > > There should be in-kernel functions for this now, no need for you to
> > > > roll your own.
> > > Sorry, do not find this function.
> > > Only find class_find_device, which still require match.
> > It is in linux-next, look there...
> >
> Suppose you mean the funcs: device_match_name,
> device_match_of_node,device_match_devt etc.
> Here we need dev->parent, there still no such func.
You should NEVER be matching on a parent. If so, your use of the driver
model is wrong :)
Remind me to really review the use of the driver core code in your next
submission of this series please, I think it needs it.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Tue, Aug 20, 2019 at 08:38:46PM +0800, zhangfei wrote: > > > On 2019/8/19 下午6:22, Greg Kroah-Hartman wrote: > > On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei@foxmail.com wrote: > > > Hi, Greg > > > > > > Thanks for your kind suggestion. > > > > > > On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote: > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote: > > > > > diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h > > > > > new file mode 100644 > > > > > index 000..44a0a5d > > > > > --- /dev/null > > > > > +++ b/include/uapi/misc/uacce.h > > > > > @@ -0,0 +1,44 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > +#ifndef _UAPIUUACCE_H > > > > > +#define _UAPIUUACCE_H > > > > > + > > > > > +#include > > > > > +#include > > > > > + > > > > > +#define UACCE_CLASS_NAME "uacce" > > > > Why is this in a uapi file? > > > User app need get attribute from /sys/class, > > > For example: /sys/class/uacce/hisi_zip-0/xxx > > > UACCE_CLASS_NAME here tells app which subsystem to open, > > > /sys/class/subsystem/ > > But that never comes from a uapi file. No other subsystem does this. > OK, got it > Maybe the entry info can come from Documentation/ABI/entries Yes it can, we now have a tool in the tree that automatically parses those entries and outputs it in a variety of different formats. thanks, greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
+int uacce_register(struct uacce *uacce)
+{
+ int ret;
+
+ if (!uacce->pdev) {
+ pr_debug("uacce parent device not set\n");
+ return -ENODEV;
+ }
+
+ if (uacce->flags & UACCE_DEV_NOIOMMU) {
+ add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+ dev_warn(uacce->pdev,
+"Register to noiommu mode, which export kernel data to user
space and may vulnerable to attack");
+ }
THat is odd, why even offer this feature then if it is a major issue?
UACCE_DEV_NOIOMMU maybe confusing here.
In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
It does not matter iommu is enabled or not.
In case iommu is disabled, it maybe dangerous to kernel, so we added warning
here, is it required?
We support two modes.
1. support sva, (va = iova)
a. If pasid is supported, multi-process can be supported.
b. If pasid is not supported, multi-process can NOT be supported.
We borrowed va from user space to achieve a single virtual address system.
2. Can not support sva, iova != va,
Here user app get dma_handle from dma_alloc_coherent via ioctl.
We need this mode for two reasons:
1. This mode can support multi-process, to support openssl etc.
2. Some accelerators (crypto like zip, etc) can work without sva, just prepare
data and kick start.
Thanks
Re: [PATCH 2/2] uacce: add uacce module
On 2019/8/19 下午6:22, Greg Kroah-Hartman wrote: On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei@foxmail.com wrote: Hi, Greg Thanks for your kind suggestion. On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote: On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote: diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h new file mode 100644 index 000..44a0a5d --- /dev/null +++ b/include/uapi/misc/uacce.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _UAPIUUACCE_H +#define _UAPIUUACCE_H + +#include +#include + +#define UACCE_CLASS_NAME "uacce" Why is this in a uapi file? User app need get attribute from /sys/class, For example: /sys/class/uacce/hisi_zip-0/xxx UACCE_CLASS_NAME here tells app which subsystem to open, /sys/class/subsystem/ But that never comes from a uapi file. No other subsystem does this. OK, got it Maybe the entry info can come from Documentation/ABI/entries Thanks
Re: [PATCH 2/2] uacce: add uacce module
Hi, Greg
On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
+static int uacce_create_chrdev(struct uacce *uacce)
+{
+ int ret;
+
+ ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?
I think you mean uacce structure here.
Yes, currently we count on caller to prepare uacce structure and call
uacce_register(uacce).
We still think this method is simpler, prepare uacce, register uacce.
And there are other system using the same method, like crypto
(crypto_register_acomp), nand, etc.
crypto is not a subsystem to ever try to emulate :)
You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do. Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.
Especially when you get into pointer lifetime issues...
OK, understand now, thanks for your patience.
will use this instead.
struct uacce_interface {
char name[32];
unsigned int flags;
struct uacce_ops *ops;
};
struct uacce *uacce_register(struct device *dev, struct uacce_interface
*interface);
+}
+
+static int uacce_dev_match(struct device *dev, void *data)
+{
+ if (dev->parent == data)
+ return -EBUSY;
There should be in-kernel functions for this now, no need for you to
roll your own.
Sorry, do not find this function.
Only find class_find_device, which still require match.
It is in linux-next, look there...
Suppose you mean the funcs: device_match_name,
device_match_of_node,device_match_devt etc.
Here we need dev->parent, there still no such func.
Thanks
Re: [PATCH 2/2] uacce: add uacce module
On Mon, Aug 19, 2019 at 05:43:40PM +0800, zhangfei@foxmail.com wrote:
> Hi, Greg
>
> On 2019/8/15 下午10:20, Greg Kroah-Hartman wrote:
> > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > +/* lock to protect all queues management */
> > > +static DECLARE_RWSEM(uacce_qs_lock);
> > > +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> > > +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> > > +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> > > +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
> > Do not define your own locking macros. That makes the code impossible
> > to review.
> >
> > And are you _sure_ you need a rw lock? You have benchmarked where it
> > has a performance impact?
> OK, will use uacce_mutex for this first version, and put performance tunning
> later.
> > > +/**
> > > + * uacce_wake_up - Wake up the process who is waiting this queue
> > > + * @q the accelerator queue to wake up
> > > + */
> > > +void uacce_wake_up(struct uacce_queue *q)
> > > +{
> > > + dev_dbg(&q->uacce->dev, "wake up\n");
> > ftrace is your friend, no need for any such logging lines anywhere in
> > these files.
> OK, will remove the dev_dbg.
> > > + wake_up_interruptible(&q->wait);
> > > +}
> > > +EXPORT_SYMBOL_GPL(uacce_wake_up);
> > ...
> >
> > > +static struct attribute *uacce_dev_attrs[] = {
> > > + &dev_attr_id.attr,
> > > + &dev_attr_api.attr,
> > > + &dev_attr_node_id.attr,
> > > + &dev_attr_numa_distance.attr,
> > > + &dev_attr_flags.attr,
> > > + &dev_attr_available_instances.attr,
> > > + &dev_attr_algorithms.attr,
> > > + &dev_attr_qfrs_offset.attr,
> > > + NULL,
> > > +};
> > > +
> > > +static const struct attribute_group uacce_dev_attr_group = {
> > > + .name = UACCE_DEV_ATTRS,
> > > + .attrs = uacce_dev_attrs,
> > > +};
> > Why is your attribute group in a subdirectory? Why not in the "normal"
> > class directory?
> Yes, .name = UACCE_DEV_ATTRS can be removed to make it simple.
> Then attrs are in /sys/class/uacce/hisi_zip-x/xxx
> >
> > You are adding sysfs files to the kernel without any Documentation/ABI/
> > entries, which is a requirement. Please fix that up for the next time
> > you send these.
> Will add Documentation/ABI/entries in next version.
> > > +static const struct attribute_group *uacce_dev_attr_groups[] = {
> > > + &uacce_dev_attr_group,
> > > + NULL
> > > +};
> > > +
> > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > +{
> > > + int ret;
> > > +
> > > + ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > Shouldn't this function create the memory needed for this structure?
> > You are relying ont he caller to do it for you, why?
> I think you mean uacce structure here.
> Yes, currently we count on caller to prepare uacce structure and call
> uacce_register(uacce).
> We still think this method is simpler, prepare uacce, register uacce.
> And there are other system using the same method, like crypto
> (crypto_register_acomp), nand, etc.
crypto is not a subsystem to ever try to emulate :)
You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do. Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.
Especially when you get into pointer lifetime issues...
> > > + cdev_init(&uacce->cdev, &uacce_fops);
> > > + uacce->dev_id = ret;
> > > + uacce->cdev.owner = THIS_MODULE;
> > > + device_initialize(&uacce->dev);
> > > + uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
> > > + uacce->dev.class = uacce_class;
> > > + uacce->dev.groups = uacce_dev_attr_groups;
> > > + uacce->dev.parent = uacce->pdev;
> > > + dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
> > > + ret = cdev_device_add(&uacce->cdev, &uacce->dev);
> > > + if (ret)
> > > + goto err_with_idr;
> > > +
> > > + dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
> > > + return 0;
> > > +
> > > +err_with_idr:
> > > + idr_remove(&uacce_idr, uacce->dev_id);
> > > + return ret;
> > > +}
> > > +
> > > +static void uacce_destroy_chrdev(struct uacce *uacce)
> > > +{
> > > + cdev_device_del(&uacce->cdev, &uacce->dev);
> > > + idr_remove(&uacce_idr, uacce->dev_id);
> > > +}
> > > +
> > > +static int uacce_default_get_available_instances(struct uacce *uacce)
> > > +{
> > > + return -1;
> > Do not make up error values, use the proper -E value instead.
> >
> > > +}
> > > +
> > > +static int uacce_default_start_queue(struct uacce_queue *q)
> > > +{
> > > + dev_dbg(&q->uacce->dev, "fake start queue");
> > > + return 0;
> > Why even have this function if you do not do anything in it?
> OK, will remove these two funcs.
> > > +}
> > > +
> > > +static int uacce_dev_match(struct device *dev, void *data)
> > > +{
> > > + if (dev->parent == data)
> > > + return -EBUSY;
> > There should be in-kernel functions for this now, no nee
Re: [PATCH 2/2] uacce: add uacce module
On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei@foxmail.com wrote: > Hi, Greg > > Thanks for your kind suggestion. > > On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote: > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote: > > > diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h > > > new file mode 100644 > > > index 000..44a0a5d > > > --- /dev/null > > > +++ b/include/uapi/misc/uacce.h > > > @@ -0,0 +1,44 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +#ifndef _UAPIUUACCE_H > > > +#define _UAPIUUACCE_H > > > + > > > +#include > > > +#include > > > + > > > +#define UACCE_CLASS_NAME "uacce" > > Why is this in a uapi file? > User app need get attribute from /sys/class, > For example: /sys/class/uacce/hisi_zip-0/xxx > UACCE_CLASS_NAME here tells app which subsystem to open, > /sys/class/subsystem/ But that never comes from a uapi file. No other subsystem does this. thanks, greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Wed, 14 Aug 2019 17:34:25 +0800
Zhangfei Gao wrote:
> From: Kenneth Lee
>
> Uacce is the kernel component to support WarpDrive accelerator
> framework. It provides register/unregister interface for device drivers
> to expose their hardware resource to the user space. The resource is
> taken as "queue" in WarpDrive.
It's a bit confusing to have both the term UACCE and WarpDrive in here.
I'd just use the uacce name in all comments etc.
>
> Uacce create a chrdev for every registration, the queue is allocated to
> the process when the chrdev is opened. Then the process can access the
> hardware resource by interact with the queue file. By mmap the queue
> file space to user space, the process can directly put requests to the
> hardware without syscall to the kernel space.
>
> Uacce also manages unify addresses between the hardware and user space
> of the process. So they can share the same virtual address in the
> communication.
>
> Signed-off-by: Kenneth Lee
> Signed-off-by: Zaibo Xu
> Signed-off-by: Zhou Wang
> Signed-off-by: Zhangfei Gao
I would strip this back to which ever case is of most interest (SVA I guess?)
and only think about adding support for the others if necessary at a later date.
(or in later patches).
This is only a fairly superficial review, so I'll have another look when time
allows.
Thanks,
Jonathan
> ---
> drivers/misc/Kconfig|1 +
> drivers/misc/Makefile |1 +
> drivers/misc/uacce/Kconfig | 13 +
> drivers/misc/uacce/Makefile |2 +
> drivers/misc/uacce/uacce.c | 1186
> +++
> include/linux/uacce.h | 109
> include/uapi/misc/uacce.h | 44 ++
> 7 files changed, 1356 insertions(+)
> create mode 100644 drivers/misc/uacce/Kconfig
> create mode 100644 drivers/misc/uacce/Makefile
> create mode 100644 drivers/misc/uacce/uacce.c
> create mode 100644 include/linux/uacce.h
> create mode 100644 include/uapi/misc/uacce.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6abfc8e..8073eb8 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -502,4 +502,5 @@ source "drivers/misc/cxl/Kconfig"
> source "drivers/misc/ocxl/Kconfig"
> source "drivers/misc/cardreader/Kconfig"
> source "drivers/misc/habanalabs/Kconfig"
> +source "drivers/misc/uacce/Kconfig"
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index abd8ae2..93a131b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,4 +58,5 @@ obj-$(CONFIG_OCXL) += ocxl/
> obj-y+= cardreader/
> obj-$(CONFIG_PVPANIC)+= pvpanic.o
> obj-$(CONFIG_HABANA_AI) += habanalabs/
> +obj-$(CONFIG_UACCE) += uacce/
> obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> diff --git a/drivers/misc/uacce/Kconfig b/drivers/misc/uacce/Kconfig
> new file mode 100644
> index 000..569669c
> --- /dev/null
> +++ b/drivers/misc/uacce/Kconfig
> @@ -0,0 +1,13 @@
> +config UACCE
> + tristate "Accelerator Framework for User Land"
> + depends on IOMMU_API
> + help
> + UACCE provides interface for the user process to access the hardware
> + without interaction with the kernel space in data path.
> +
> + The user-space interface is described in
> + include/uapi/misc/uacce.h
> +
> + See Documentation/misc-devices/warpdrive.rst for more details.
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/misc/uacce/Makefile b/drivers/misc/uacce/Makefile
> new file mode 100644
> index 000..5b4374e
> --- /dev/null
> +++ b/drivers/misc/uacce/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +obj-$(CONFIG_UACCE) += uacce.o
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> new file mode 100644
> index 000..43e0c9b
> --- /dev/null
> +++ b/drivers/misc/uacce/uacce.c
> @@ -0,0 +1,1186 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static struct class *uacce_class;
> +static DEFINE_IDR(uacce_idr);
> +static dev_t uacce_devt;
> +static DEFINE_MUTEX(uacce_mutex); /* mutex to protect uacce */
> +
> +/* lock to protect all queues management */
> +static DECLARE_RWSEM(uacce_qs_lock);
> +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
> +
> +static const struct file_operations uacce_fops;
> +
> +/* match with enum uacce_qfrt */
> +static const char *const qfrt_str[] = {
> + "mmio",
> + "dko",
> + "dus",
> + "ss",
> + "invalid"
> +};
> +
> +static const char *uacce_qfrt_str(struct uacce_qfile_region *qfr)
> +{
> + enum uacce_qfrt type = qfr->type;
> +
> + if (type > UACCE_QFRT_INVALID)
> +
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> +/* lock to protect all queues management */
> +static DECLARE_RWSEM(uacce_qs_lock);
> +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
Do not define your own locking macros. That makes the code impossible
to review.
And are you _sure_ you need a rw lock? You have benchmarked where it
has a performance impact?
> +/**
> + * uacce_wake_up - Wake up the process who is waiting this queue
> + * @q the accelerator queue to wake up
> + */
> +void uacce_wake_up(struct uacce_queue *q)
> +{
> + dev_dbg(&q->uacce->dev, "wake up\n");
ftrace is your friend, no need for any such logging lines anywhere in
these files.
> + wake_up_interruptible(&q->wait);
> +}
> +EXPORT_SYMBOL_GPL(uacce_wake_up);
...
> +static struct attribute *uacce_dev_attrs[] = {
> + &dev_attr_id.attr,
> + &dev_attr_api.attr,
> + &dev_attr_node_id.attr,
> + &dev_attr_numa_distance.attr,
> + &dev_attr_flags.attr,
> + &dev_attr_available_instances.attr,
> + &dev_attr_algorithms.attr,
> + &dev_attr_qfrs_offset.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group uacce_dev_attr_group = {
> + .name = UACCE_DEV_ATTRS,
> + .attrs = uacce_dev_attrs,
> +};
Why is your attribute group in a subdirectory? Why not in the "normal"
class directory?
You are adding sysfs files to the kernel without any Documentation/ABI/
entries, which is a requirement. Please fix that up for the next time
you send these.
> +static const struct attribute_group *uacce_dev_attr_groups[] = {
> + &uacce_dev_attr_group,
> + NULL
> +};
> +
> +static int uacce_create_chrdev(struct uacce *uacce)
> +{
> + int ret;
> +
> + ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> +
Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?
> + cdev_init(&uacce->cdev, &uacce_fops);
> + uacce->dev_id = ret;
> + uacce->cdev.owner = THIS_MODULE;
> + device_initialize(&uacce->dev);
> + uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
> + uacce->dev.class = uacce_class;
> + uacce->dev.groups = uacce_dev_attr_groups;
> + uacce->dev.parent = uacce->pdev;
> + dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
> + ret = cdev_device_add(&uacce->cdev, &uacce->dev);
> + if (ret)
> + goto err_with_idr;
> +
> + dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
> + return 0;
> +
> +err_with_idr:
> + idr_remove(&uacce_idr, uacce->dev_id);
> + return ret;
> +}
> +
> +static void uacce_destroy_chrdev(struct uacce *uacce)
> +{
> + cdev_device_del(&uacce->cdev, &uacce->dev);
> + idr_remove(&uacce_idr, uacce->dev_id);
> +}
> +
> +static int uacce_default_get_available_instances(struct uacce *uacce)
> +{
> + return -1;
Do not make up error values, use the proper -E value instead.
> +}
> +
> +static int uacce_default_start_queue(struct uacce_queue *q)
> +{
> + dev_dbg(&q->uacce->dev, "fake start queue");
> + return 0;
Why even have this function if you do not do anything in it?
> +}
> +
> +static int uacce_dev_match(struct device *dev, void *data)
> +{
> + if (dev->parent == data)
> + return -EBUSY;
There should be in-kernel functions for this now, no need for you to
roll your own.
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> +int uacce_register(struct uacce *uacce)
> +{
> + int ret;
> +
> + if (!uacce->pdev) {
> + pr_debug("uacce parent device not set\n");
> + return -ENODEV;
> + }
> +
> + if (uacce->flags & UACCE_DEV_NOIOMMU) {
> + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> + dev_warn(uacce->pdev,
> + "Register to noiommu mode, which export kernel data to
> user space and may vulnerable to attack");
> + }
THat is odd, why even offer this feature then if it is a major issue?
thanks,
greg k-h
Re: [PATCH 2/2] uacce: add uacce module
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
> new file mode 100644
> index 000..44a0a5d
> --- /dev/null
> +++ b/include/uapi/misc/uacce.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _UAPIUUACCE_H
> +#define _UAPIUUACCE_H
> +
> +#include
> +#include
> +
> +#define UACCE_CLASS_NAME "uacce"
Why is this in a uapi file?
> +#define UACCE_DEV_ATTRS "attrs"
Same here.
> +#define UACCE_CMD_SHARE_SVAS _IO('W', 0)
> +#define UACCE_CMD_START _IO('W', 1)
> +#define UACCE_CMD_GET_SS_DMA _IOR('W', 2, unsigned long)
> +
> +/**
> + * UACCE Device Attributes:
> + *
> + * NOIOMMU: the device has no IOMMU support
> + * can do share sva, but no map to the dev
> + * PASID: the device has IOMMU which support PASID setting
> + * can do share sva, mapped to dev per process
> + * FAULT_FROM_DEV: the device has IOMMU which can do page fault request
> + * no need for share sva, should be used with PASID
> + * SVA: full function device
> + * SHARE_DOMAIN: no PASID, can do share sva only for one process and the
> kernel
> + */
> +#define UACCE_DEV_NOIOMMU(1 << 0)
> +#define UACCE_DEV_PASID (1 << 1)
> +#define UACCE_DEV_FAULT_FROM_DEV (1 << 2)
> +#define UACCE_DEV_SVA(UACCE_DEV_PASID |
> UACCE_DEV_FAULT_FROM_DEV)
> +#define UACCE_DEV_SHARE_DOMAIN (0)
> +
> +#define UACCE_API_VER_NOIOMMU_SUBFIX "_noiommu"
> +
> +#define UACCE_QFR_NA ((unsigned long)-1)
> +enum uacce_qfrt {
> + UACCE_QFRT_MMIO = 0,/* device mmio region */
> + UACCE_QFRT_DKO, /* device kernel-only */
> + UACCE_QFRT_DUS, /* device user share */
> + UACCE_QFRT_SS, /* static share memory */
> + UACCE_QFRT_MAX,
These enums need to be explicitly set, as per the documentation,
otherwise they could be messed up when dealing with odd compilers.
> +};
> +#define UACCE_QFRT_INVALID UACCE_QFRT_MAX
Why not just use INVALID instead of MAX?
thanks,
greg k-h
