| Message ID | 20180601114132.22596-7-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Add UAPI and IOCTLs for dma-buf grant device driver extension: > the extension allows userspace processes and kernel modules to > use Xen backed dma-buf implementation. With this extension grant > references to the pages of an imported dma-buf can be exported > for other domain use and grant references coming from a foreign > domain can be converted into a local dma-buf for local export. > Implement basic initialization and stubs for Xen DMA buffers' > support. It would be very helpful if people advocating for this interface reviewed it as well. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > drivers/xen/Kconfig | 10 +++ > drivers/xen/Makefile | 1 + > drivers/xen/gntdev-dmabuf.c | 75 +++++++++++++++++++ > drivers/xen/gntdev-dmabuf.h | 41 +++++++++++ > drivers/xen/gntdev.c | 142 ++++++++++++++++++++++++++++++++++++ > include/uapi/xen/gntdev.h | 91 +++++++++++++++++++++++ > 6 files changed, 360 insertions(+) > create mode 100644 drivers/xen/gntdev-dmabuf.c > create mode 100644 drivers/xen/gntdev-dmabuf.h > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig > index 39536ddfbce4..52d64e4b6b81 100644 > --- a/drivers/xen/Kconfig > +++ b/drivers/xen/Kconfig > @@ -152,6 +152,16 @@ config XEN_GNTDEV > help > Allows userspace processes to use grants. > > +config XEN_GNTDEV_DMABUF > + bool "Add support for dma-buf grant access device driver extension" > + depends on XEN_GNTDEV && XEN_GRANT_DMA_ALLOC && DMA_SHARED_BUFFER Is there a reason to have XEN_GRANT_DMA_ALLOC without XEN_GNTDEV_DMABUF? > + help > + Allows userspace processes and kernel modules to use Xen backed > + dma-buf implementation. With this extension grant references to > + the pages of an imported dma-buf can be exported for other domain > + use and grant references coming from a foreign domain can be > + converted into a local dma-buf for local export. > + > config XEN_GRANT_DEV_ALLOC > tristate "User-space grant reference allocator driver" > depends on XEN > diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile > index 3c87b0c3aca6..33afb7b2b227 100644 > --- a/drivers/xen/Makefile > +++ b/drivers/xen/Makefile > @@ -41,5 +41,6 @@ obj-$(CONFIG_XEN_PVCALLS_BACKEND) += pvcalls-back.o > obj-$(CONFIG_XEN_PVCALLS_FRONTEND) += pvcalls-front.o > xen-evtchn-y := evtchn.o > xen-gntdev-y := gntdev.o > +xen-gntdev-$(CONFIG_XEN_GNTDEV_DMABUF) += gntdev-dmabuf.o > xen-gntalloc-y := gntalloc.o > xen-privcmd-y := privcmd.o > diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c > new file mode 100644 > index 000000000000..6bedd1387bd9 > --- /dev/null > +++ b/drivers/xen/gntdev-dmabuf.c > @@ -0,0 +1,75 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Xen dma-buf functionality for gntdev. > + * > + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. > + */ > + > +#include <linux/slab.h> > + > +#include "gntdev-dmabuf.h" > + > +struct gntdev_dmabuf_priv { > + int dummy; > +}; > + > +/* ------------------------------------------------------------------ */ > +/* DMA buffer export support. */ > +/* ------------------------------------------------------------------ */ > + > +/* ------------------------------------------------------------------ */ > +/* Implementation of wait for exported DMA buffer to be released. */ > +/* ------------------------------------------------------------------ */ Why this comment style? > + > +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, > + int wait_to_ms) > +{ > + return -EINVAL; > +} > + > +/* ------------------------------------------------------------------ */ > +/* DMA buffer export support. */ > +/* ------------------------------------------------------------------ */ > + > +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args) > +{ > + return -EINVAL; > +} > + > +/* ------------------------------------------------------------------ */ > +/* DMA buffer import support. */ > +/* ------------------------------------------------------------------ */ > + > +struct gntdev_dmabuf * > +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, > + int fd, int count, int domid) > +{ > + return ERR_PTR(-ENOMEM); > +} > + > +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf) > +{ > + return NULL; > +} > + > +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd) > +{ > + return -EINVAL; > +} > + > +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void) > +{ > + struct gntdev_dmabuf_priv *priv; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return ERR_PTR(-ENOMEM); > + > + return priv; > +} > + > +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv) > +{ > + kfree(priv); > +} > diff --git a/drivers/xen/gntdev-dmabuf.h b/drivers/xen/gntdev-dmabuf.h > new file mode 100644 > index 000000000000..040b2de904ac > --- /dev/null > +++ b/drivers/xen/gntdev-dmabuf.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* > + * Xen dma-buf functionality for gntdev. > + * > + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. > + */ > + > +#ifndef _GNTDEV_DMABUF_H > +#define _GNTDEV_DMABUF_H > + > +#include <linux/kernel.h> > +#include <linux/errno.h> > +#include <linux/types.h> > + > +struct gntdev_dmabuf_priv; > +struct gntdev_dmabuf; > +struct device; > + > +struct gntdev_dmabuf_export_args { > + int dummy; > +}; Please define the full structure (at least what you have in the next patch) here. > + > +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void); > + > +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv); > + > +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args); > + > +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, > + int wait_to_ms); > + > +struct gntdev_dmabuf * > +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, > + int fd, int count, int domid); > + > +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf); > + > +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd); > + > +#endif > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 9813fc440c70..7d58dfb3e5e8 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c ... > > +#ifdef CONFIG_XEN_GNTDEV_DMABUF This code belongs in gntdev-dmabuf.c. > +/* ------------------------------------------------------------------ */ > +/* DMA buffer export support. */ > +/* ------------------------------------------------------------------ */ > + > +int gntdev_dmabuf_exp_from_refs(struct gntdev_priv *priv, int flags, > + int count, u32 domid, u32 *refs, u32 *fd) > +{ > + /* XXX: this will need to work with gntdev's map, so leave it here. */ This doesn't help understanding what's going on (at least to me) and is removed in the next patch. So no need for this comment. -boris > + *fd = -1; > + return -EINVAL; > +}
On 06/04/2018 11:49 PM, Boris Ostrovsky wrote: > On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Add UAPI and IOCTLs for dma-buf grant device driver extension: >> the extension allows userspace processes and kernel modules to >> use Xen backed dma-buf implementation. With this extension grant >> references to the pages of an imported dma-buf can be exported >> for other domain use and grant references coming from a foreign >> domain can be converted into a local dma-buf for local export. >> Implement basic initialization and stubs for Xen DMA buffers' >> support. > > It would be very helpful if people advocating for this interface > reviewed it as well. I would also love to see their comments here ;) > >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> drivers/xen/Kconfig | 10 +++ >> drivers/xen/Makefile | 1 + >> drivers/xen/gntdev-dmabuf.c | 75 +++++++++++++++++++ >> drivers/xen/gntdev-dmabuf.h | 41 +++++++++++ >> drivers/xen/gntdev.c | 142 ++++++++++++++++++++++++++++++++++++ >> include/uapi/xen/gntdev.h | 91 +++++++++++++++++++++++ >> 6 files changed, 360 insertions(+) >> create mode 100644 drivers/xen/gntdev-dmabuf.c >> create mode 100644 drivers/xen/gntdev-dmabuf.h >> >> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig >> index 39536ddfbce4..52d64e4b6b81 100644 >> --- a/drivers/xen/Kconfig >> +++ b/drivers/xen/Kconfig >> @@ -152,6 +152,16 @@ config XEN_GNTDEV >> help >> Allows userspace processes to use grants. >> >> +config XEN_GNTDEV_DMABUF >> + bool "Add support for dma-buf grant access device driver extension" >> + depends on XEN_GNTDEV && XEN_GRANT_DMA_ALLOC && DMA_SHARED_BUFFER > > Is there a reason to have XEN_GRANT_DMA_ALLOC without XEN_GNTDEV_DMABUF? One can use grant-table's DMA API without using dma-buf at all, e.g. dma-buf is sort of functionality on top of DMA allocated memory. We have a use-case for a driver domain (guest domain in fact) backed with IOMMU and still requiring allocations created as contiguous/DMA memory, so those buffers can be passed around to drivers expecting DMA-only buffers. So, IMO this is a valid use-case "to have XEN_GRANT_DMA_ALLOC without XEN_GNTDEV_DMABUF" > >> + help >> + Allows userspace processes and kernel modules to use Xen backed >> + dma-buf implementation. With this extension grant references to >> + the pages of an imported dma-buf can be exported for other domain >> + use and grant references coming from a foreign domain can be >> + converted into a local dma-buf for local export. >> + >> config XEN_GRANT_DEV_ALLOC >> tristate "User-space grant reference allocator driver" >> depends on XEN >> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile >> index 3c87b0c3aca6..33afb7b2b227 100644 >> --- a/drivers/xen/Makefile >> +++ b/drivers/xen/Makefile >> @@ -41,5 +41,6 @@ obj-$(CONFIG_XEN_PVCALLS_BACKEND) += pvcalls-back.o >> obj-$(CONFIG_XEN_PVCALLS_FRONTEND) += pvcalls-front.o >> xen-evtchn-y := evtchn.o >> xen-gntdev-y := gntdev.o >> +xen-gntdev-$(CONFIG_XEN_GNTDEV_DMABUF) += gntdev-dmabuf.o >> xen-gntalloc-y := gntalloc.o >> xen-privcmd-y := privcmd.o >> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c >> new file mode 100644 >> index 000000000000..6bedd1387bd9 >> --- /dev/null >> +++ b/drivers/xen/gntdev-dmabuf.c >> @@ -0,0 +1,75 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +/* >> + * Xen dma-buf functionality for gntdev. >> + * >> + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. >> + */ >> + >> +#include <linux/slab.h> >> + >> +#include "gntdev-dmabuf.h" >> + >> +struct gntdev_dmabuf_priv { >> + int dummy; >> +}; >> + >> +/* ------------------------------------------------------------------ */ >> +/* DMA buffer export support. */ >> +/* ------------------------------------------------------------------ */ >> + >> +/* ------------------------------------------------------------------ */ >> +/* Implementation of wait for exported DMA buffer to be released. */ >> +/* ------------------------------------------------------------------ */ > Why this comment style? Just a copy-paste from gntdev, will change to usual /*..*/ > >> + >> +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, >> + int wait_to_ms) >> +{ >> + return -EINVAL; >> +} >> + >> +/* ------------------------------------------------------------------ */ >> +/* DMA buffer export support. */ >> +/* ------------------------------------------------------------------ */ >> + >> +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args) >> +{ >> + return -EINVAL; >> +} >> + >> +/* ------------------------------------------------------------------ */ >> +/* DMA buffer import support. */ >> +/* ------------------------------------------------------------------ */ >> + >> +struct gntdev_dmabuf * >> +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, >> + int fd, int count, int domid) >> +{ >> + return ERR_PTR(-ENOMEM); >> +} >> + >> +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf) >> +{ >> + return NULL; >> +} >> + >> +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd) >> +{ >> + return -EINVAL; >> +} >> + >> +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void) >> +{ >> + struct gntdev_dmabuf_priv *priv; >> + >> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return ERR_PTR(-ENOMEM); >> + >> + return priv; >> +} >> + >> +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv) >> +{ >> + kfree(priv); >> +} >> diff --git a/drivers/xen/gntdev-dmabuf.h b/drivers/xen/gntdev-dmabuf.h >> new file mode 100644 >> index 000000000000..040b2de904ac >> --- /dev/null >> +++ b/drivers/xen/gntdev-dmabuf.h >> @@ -0,0 +1,41 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +/* >> + * Xen dma-buf functionality for gntdev. >> + * >> + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. >> + */ >> + >> +#ifndef _GNTDEV_DMABUF_H >> +#define _GNTDEV_DMABUF_H >> + >> +#include <linux/kernel.h> >> +#include <linux/errno.h> >> +#include <linux/types.h> >> + >> +struct gntdev_dmabuf_priv; >> +struct gntdev_dmabuf; >> +struct device; >> + >> +struct gntdev_dmabuf_export_args { >> + int dummy; >> +}; > > Please define the full structure (at least what you have in the next > patch) here. Ok, will define what I have in the next patch, but won't initialize anything until the next patch. Will this work for you? > >> + >> +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void); >> + >> +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv); >> + >> +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args); >> + >> +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, >> + int wait_to_ms); >> + >> +struct gntdev_dmabuf * >> +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, >> + int fd, int count, int domid); >> + >> +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf); >> + >> +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd); >> + >> +#endif >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index 9813fc440c70..7d58dfb3e5e8 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c > ... > >> >> +#ifdef CONFIG_XEN_GNTDEV_DMABUF > This code belongs in gntdev-dmabuf.c. The reason I have this code here is that it is heavily tied to gntdev's internal functionality, e.g. map/unmap. I do not want to extend gntdev's API, so gntdev-dmabuf can access these. What is more dma-buf doesn't need to know about maps done by gntdev as there is no use of that information in gntdev-dmabuf. So, it seems more naturally to have dma-buf's related map/unmap code where it is: in gntdev. > >> +/* ------------------------------------------------------------------ */ >> +/* DMA buffer export support. */ >> +/* ------------------------------------------------------------------ */ >> + >> +int gntdev_dmabuf_exp_from_refs(struct gntdev_priv *priv, int flags, >> + int count, u32 domid, u32 *refs, u32 *fd) >> +{ >> + /* XXX: this will need to work with gntdev's map, so leave it here. */ > This doesn't help understanding what's going on (at least to me) and is > removed in the next patch. So no need for this comment. Will remove the comment > -boris > >> + *fd = -1; >> + return -EINVAL; >> +} >
On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote: > On 06/04/2018 11:49 PM, Boris Ostrovsky wrote: >> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >> +struct gntdev_dmabuf_export_args { >> + int dummy; >> +}; >> >> Please define the full structure (at least what you have in the next >> patch) here. > Ok, will define what I have in the next patch, but won't > initialize anything until the next patch. Will this work for you? Sure, I just didn't see the need for the dummy argument that you remove later. >> >>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >>> index 9813fc440c70..7d58dfb3e5e8 100644 >>> --- a/drivers/xen/gntdev.c >>> +++ b/drivers/xen/gntdev.c >> ... >> >>> +#ifdef CONFIG_XEN_GNTDEV_DMABUF >> This code belongs in gntdev-dmabuf.c. > The reason I have this code here is that it is heavily > tied to gntdev's internal functionality, e.g. map/unmap. > I do not want to extend gntdev's API, so gntdev-dmabuf can > access these. What is more dma-buf doesn't need to know about > maps done by gntdev as there is no use of that information > in gntdev-dmabuf. So, it seems more naturally to have > dma-buf's related map/unmap code where it is: in gntdev. Sorry, I don't follow. Why would this require extending the API? It's just moving routines to a different file that is linked to the same module. Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in gntdev-dmabuf.c? In my view that's the file where all dma-related "stuff" lives. -boris -boris
On 06/07/2018 12:32 AM, Boris Ostrovsky wrote: > On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote: >> On 06/04/2018 11:49 PM, Boris Ostrovsky wrote: >>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >>> +struct gntdev_dmabuf_export_args { >>> + int dummy; >>> +}; >>> >>> Please define the full structure (at least what you have in the next >>> patch) here. >> Ok, will define what I have in the next patch, but won't >> initialize anything until the next patch. Will this work for you? > Sure, I just didn't see the need for the dummy argument that you remove > later. Ok >>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >>>> index 9813fc440c70..7d58dfb3e5e8 100644 >>>> --- a/drivers/xen/gntdev.c >>>> +++ b/drivers/xen/gntdev.c >>> ... >>> >>>> +#ifdef CONFIG_XEN_GNTDEV_DMABUF >>> This code belongs in gntdev-dmabuf.c. >> The reason I have this code here is that it is heavily >> tied to gntdev's internal functionality, e.g. map/unmap. >> I do not want to extend gntdev's API, so gntdev-dmabuf can >> access these. What is more dma-buf doesn't need to know about >> maps done by gntdev as there is no use of that information >> in gntdev-dmabuf. So, it seems more naturally to have >> dma-buf's related map/unmap code where it is: in gntdev. > Sorry, I don't follow. Why would this require extending the API? It's > just moving routines to a different file that is linked to the same module. I do understand your intention here and tried to avoid dma-buf related code in gntdev.c as much as possible. So, let me explain my decision in more detail. There are 2 use-cases we have: dma-buf import and export. While importing a dma-buf all the dma-buf related functionality can easily be kept inside gntdev-dmabuf.c w/o any issue as all we need from gntdev.c is dev, dma_buf_fd, count and domid for that. But in case of dma-buf export we need to: 1. struct grant_map *map = gntdev_alloc_map(priv, count, dmabuf_flags); 2. gntdev_add_map(priv, map); 3. Set map->flags 4. ret = map_grant_pages(map); 5. And only now we are all set to export the new dma-buf from *map->pages* So, until 5) we use private gtndev.c's API not exported to outside world: a. struct grant_map b. static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count, int dma_flags) c. static void gntdev_add_map(struct gntdev_priv *priv, struct grant_map *add) d. static int map_grant_pages(struct grant_map *map) Thus, all the above cannot be accessed from gntdev-dmabuf.c This is why I say that gntdev.c's API will need to be extended to provide the above a-d if we want all dma-buf export code to leave in gntdev-dmabuf.c. But that doesn't seem good to me and what is more a-d are really gntdev.c's functionality, not dma-buf's which only needs pages and doesn't really care from where those come. That was the reason I partitioned export into 2 chunks: gntdev + gntdev-dmabuf. You might also ask why importing side does Xen related things (granting references+) in gntdev-dmabuf, not gntdev so it is consistent with the dma-buf exporter? This is because importer uses grant-table's API which seems to be not natural for gntdev.c, so it can leave in gntdev-dmabuf.c which has a use-case for that, while gntdev remains the same. > Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in > gntdev-dmabuf.c? In my view that's the file where all dma-related > "stuff" lives. Agree, but IMO grant_map stuff for dma-buf importer is right in its place in gntdev.c and all the rest of dma-buf specifics live in gntdev-dmabuf.c as they should > > -boris > > > -boris > Thank you, Oleksandr
On 06/07/2018 03:17 AM, Oleksandr Andrushchenko wrote: > On 06/07/2018 12:32 AM, Boris Ostrovsky wrote: >> On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote: >>> On 06/04/2018 11:49 PM, Boris Ostrovsky wrote: >>>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >>>>> index 9813fc440c70..7d58dfb3e5e8 100644 >>>>> --- a/drivers/xen/gntdev.c >>>>> +++ b/drivers/xen/gntdev.c >>>> ... >>>> >>>>> +#ifdef CONFIG_XEN_GNTDEV_DMABUF >>>> This code belongs in gntdev-dmabuf.c. >>> The reason I have this code here is that it is heavily >>> tied to gntdev's internal functionality, e.g. map/unmap. >>> I do not want to extend gntdev's API, so gntdev-dmabuf can >>> access these. What is more dma-buf doesn't need to know about >>> maps done by gntdev as there is no use of that information >>> in gntdev-dmabuf. So, it seems more naturally to have >>> dma-buf's related map/unmap code where it is: in gntdev. >> Sorry, I don't follow. Why would this require extending the API? It's >> just moving routines to a different file that is linked to the same >> module. > I do understand your intention here and tried to avoid dma-buf > related code in gntdev.c as much as possible. So, let me explain > my decision in more detail. > > There are 2 use-cases we have: dma-buf import and export. > > While importing a dma-buf all the dma-buf related functionality can > easily be kept inside gntdev-dmabuf.c w/o any issue as all we need > from gntdev.c is dev, dma_buf_fd, count and domid for that. > > But in case of dma-buf export we need to: > 1. struct grant_map *map = gntdev_alloc_map(priv, count, dmabuf_flags); > 2. gntdev_add_map(priv, map); > 3. Set map->flags > 4. ret = map_grant_pages(map); > 5. And only now we are all set to export the new dma-buf from > *map->pages* > > So, until 5) we use private gtndev.c's API not exported to outside world: > a. struct grant_map > b. static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, > int count, > int dma_flags) > c. static void gntdev_add_map(struct gntdev_priv *priv, struct > grant_map *add) > d. static int map_grant_pages(struct grant_map *map) > > Thus, all the above cannot be accessed from gntdev-dmabuf.c > This is why I say that gntdev.c's API will need to be extended to > provide the above > a-d if we want all dma-buf export code to leave in gntdev-dmabuf.c. I still don't understand why you feel this would be extending the API. These routines and the struct can be declared in local header file and this header file will not be visible to anyone but gntdev.c and gntdev-dmabuf.c. You can, for example, put this into gntdev-dmabuf.h (and then rename it to something else, like gntdev-common.h). > But that doesn't seem good to me and what is more a-d are really > gntdev.c's > functionality, not dma-buf's which only needs pages and doesn't really > care from > where those come. > That was the reason I partitioned export into 2 chunks: gntdev + > gntdev-dmabuf. > > You might also ask why importing side does Xen related things > (granting references+) > in gntdev-dmabuf, not gntdev so it is consistent with the dma-buf > exporter? > This is because importer uses grant-table's API which seems to be not > natural for gntdev.c, > so it can leave in gntdev-dmabuf.c which has a use-case for that, > while gntdev > remains the same. Yet another reason why this code should be moved: importing and exporting functionalities logically belong together. The fat that they are implemented using different methods is not relevant IMO. If you have code which is under ifdef CONFIG_GNTDEV_DMABUF and you have file called gntdev-dmabuf.c it sort of implies that this code should live in that file (unless that code is intertwined with other code, which is not the case here). -boris >> Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in >> gntdev-dmabuf.c? In my view that's the file where all dma-related >> "stuff" lives. > Agree, but IMO grant_map stuff for dma-buf importer is right in its > place in gntdev.c > and all the rest of dma-buf specifics live in gntdev-dmabuf.c as they > should >> >> -boris >> >> >> -boris >> > Thank you, > Oleksandr
On 06/08/2018 01:26 AM, Boris Ostrovsky wrote: > On 06/07/2018 03:17 AM, Oleksandr Andrushchenko wrote: >> On 06/07/2018 12:32 AM, Boris Ostrovsky wrote: >>> On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote: >>>> On 06/04/2018 11:49 PM, Boris Ostrovsky wrote: >>>>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >>>>>> index 9813fc440c70..7d58dfb3e5e8 100644 >>>>>> --- a/drivers/xen/gntdev.c >>>>>> +++ b/drivers/xen/gntdev.c >>>>> ... >>>>> >>>>>> +#ifdef CONFIG_XEN_GNTDEV_DMABUF >>>>> This code belongs in gntdev-dmabuf.c. >>>> The reason I have this code here is that it is heavily >>>> tied to gntdev's internal functionality, e.g. map/unmap. >>>> I do not want to extend gntdev's API, so gntdev-dmabuf can >>>> access these. What is more dma-buf doesn't need to know about >>>> maps done by gntdev as there is no use of that information >>>> in gntdev-dmabuf. So, it seems more naturally to have >>>> dma-buf's related map/unmap code where it is: in gntdev. >>> Sorry, I don't follow. Why would this require extending the API? It's >>> just moving routines to a different file that is linked to the same >>> module. >> I do understand your intention here and tried to avoid dma-buf >> related code in gntdev.c as much as possible. So, let me explain >> my decision in more detail. >> >> There are 2 use-cases we have: dma-buf import and export. >> >> While importing a dma-buf all the dma-buf related functionality can >> easily be kept inside gntdev-dmabuf.c w/o any issue as all we need >> from gntdev.c is dev, dma_buf_fd, count and domid for that. >> >> But in case of dma-buf export we need to: >> 1. struct grant_map *map = gntdev_alloc_map(priv, count, dmabuf_flags); >> 2. gntdev_add_map(priv, map); >> 3. Set map->flags >> 4. ret = map_grant_pages(map); >> 5. And only now we are all set to export the new dma-buf from >> *map->pages* >> >> So, until 5) we use private gtndev.c's API not exported to outside world: >> a. struct grant_map >> b. static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, >> int count, >> int dma_flags) >> c. static void gntdev_add_map(struct gntdev_priv *priv, struct >> grant_map *add) >> d. static int map_grant_pages(struct grant_map *map) >> >> Thus, all the above cannot be accessed from gntdev-dmabuf.c >> This is why I say that gntdev.c's API will need to be extended to >> provide the above >> a-d if we want all dma-buf export code to leave in gntdev-dmabuf.c. > > > I still don't understand why you feel this would be extending the API. > These routines and the struct can be declared in local header file and > this header file will not be visible to anyone but gntdev.c and > gntdev-dmabuf.c. Ok, this is what I meant: I will need to move private structures and some function prototypes from gntdev.c into a header file, thus extending its API: before the header nothing were exposed. Sorry for not being clear here. > You can, for example, put this into gntdev-dmabuf.h > (and then rename it to something else, like gntdev-common.h). Sure, I will move all I need into that shared header > > >> But that doesn't seem good to me and what is more a-d are really >> gntdev.c's >> functionality, not dma-buf's which only needs pages and doesn't really >> care from >> where those come. >> That was the reason I partitioned export into 2 chunks: gntdev + >> gntdev-dmabuf. >> >> You might also ask why importing side does Xen related things >> (granting references+) >> in gntdev-dmabuf, not gntdev so it is consistent with the dma-buf >> exporter? >> This is because importer uses grant-table's API which seems to be not >> natural for gntdev.c, >> so it can leave in gntdev-dmabuf.c which has a use-case for that, >> while gntdev >> remains the same. > > Yet another reason why this code should be moved: importing and > exporting functionalities logically belong together. The fat that they > are implemented using different methods is not relevant IMO. > > If you have code which is under ifdef CONFIG_GNTDEV_DMABUF and you have > file called gntdev-dmabuf.c it sort of implies that this code should > live in that file (unless that code is intertwined with other code, > which is not the case here). Ok, will move as discussed above > > -boris Thank you, Oleksandr > > >>> Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in >>> gntdev-dmabuf.c? In my view that's the file where all dma-related >>> "stuff" lives. >> Agree, but IMO grant_map stuff for dma-buf importer is right in its >> place in gntdev.c >> and all the rest of dma-buf specifics live in gntdev-dmabuf.c as they >> should >>> -boris >>> >>> >>> -boris >>> >> Thank you, >> Oleksandr
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig index 39536ddfbce4..52d64e4b6b81 100644 --- a/drivers/xen/Kconfig +++ b/drivers/xen/Kconfig @@ -152,6 +152,16 @@ config XEN_GNTDEV help Allows userspace processes to use grants. +config XEN_GNTDEV_DMABUF + bool "Add support for dma-buf grant access device driver extension" + depends on XEN_GNTDEV && XEN_GRANT_DMA_ALLOC && DMA_SHARED_BUFFER + help + Allows userspace processes and kernel modules to use Xen backed + dma-buf implementation. With this extension grant references to + the pages of an imported dma-buf can be exported for other domain + use and grant references coming from a foreign domain can be + converted into a local dma-buf for local export. + config XEN_GRANT_DEV_ALLOC tristate "User-space grant reference allocator driver" depends on XEN diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile index 3c87b0c3aca6..33afb7b2b227 100644 --- a/drivers/xen/Makefile +++ b/drivers/xen/Makefile @@ -41,5 +41,6 @@ obj-$(CONFIG_XEN_PVCALLS_BACKEND) += pvcalls-back.o obj-$(CONFIG_XEN_PVCALLS_FRONTEND) += pvcalls-front.o xen-evtchn-y := evtchn.o xen-gntdev-y := gntdev.o +xen-gntdev-$(CONFIG_XEN_GNTDEV_DMABUF) += gntdev-dmabuf.o xen-gntalloc-y := gntalloc.o xen-privcmd-y := privcmd.o diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c new file mode 100644 index 000000000000..6bedd1387bd9 --- /dev/null +++ b/drivers/xen/gntdev-dmabuf.c @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Xen dma-buf functionality for gntdev. + * + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. + */ + +#include <linux/slab.h> + +#include "gntdev-dmabuf.h" + +struct gntdev_dmabuf_priv { + int dummy; +}; + +/* ------------------------------------------------------------------ */ +/* DMA buffer export support. */ +/* ------------------------------------------------------------------ */ + +/* ------------------------------------------------------------------ */ +/* Implementation of wait for exported DMA buffer to be released. */ +/* ------------------------------------------------------------------ */ + +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, + int wait_to_ms) +{ + return -EINVAL; +} + +/* ------------------------------------------------------------------ */ +/* DMA buffer export support. */ +/* ------------------------------------------------------------------ */ + +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args) +{ + return -EINVAL; +} + +/* ------------------------------------------------------------------ */ +/* DMA buffer import support. */ +/* ------------------------------------------------------------------ */ + +struct gntdev_dmabuf * +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, + int fd, int count, int domid) +{ + return ERR_PTR(-ENOMEM); +} + +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf) +{ + return NULL; +} + +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd) +{ + return -EINVAL; +} + +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void) +{ + struct gntdev_dmabuf_priv *priv; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return ERR_PTR(-ENOMEM); + + return priv; +} + +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv) +{ + kfree(priv); +} diff --git a/drivers/xen/gntdev-dmabuf.h b/drivers/xen/gntdev-dmabuf.h new file mode 100644 index 000000000000..040b2de904ac --- /dev/null +++ b/drivers/xen/gntdev-dmabuf.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* + * Xen dma-buf functionality for gntdev. + * + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. + */ + +#ifndef _GNTDEV_DMABUF_H +#define _GNTDEV_DMABUF_H + +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/types.h> + +struct gntdev_dmabuf_priv; +struct gntdev_dmabuf; +struct device; + +struct gntdev_dmabuf_export_args { + int dummy; +}; + +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void); + +void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv); + +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args); + +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd, + int wait_to_ms); + +struct gntdev_dmabuf * +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, + int fd, int count, int domid); + +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf); + +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd); + +#endif diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 9813fc440c70..7d58dfb3e5e8 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -6,6 +6,7 @@ * * Copyright (c) 2006-2007, D G Murray. * (c) 2009 Gerd Hoffmann <kraxel@redhat.com> + * (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of @@ -50,6 +51,10 @@ #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> +#ifdef CONFIG_XEN_GNTDEV_DMABUF +#include "gntdev-dmabuf.h" +#endif + MODULE_LICENSE("GPL"); MODULE_AUTHOR("Derek G. Murray <Derek.Murray@cl.cam.ac.uk>, " "Gerd Hoffmann <kraxel@redhat.com>"); @@ -80,6 +85,10 @@ struct gntdev_priv { /* Device for which DMA memory is allocated. */ struct device *dma_dev; #endif + +#ifdef CONFIG_XEN_GNTDEV_DMABUF + struct gntdev_dmabuf_priv *dmabuf_priv; +#endif }; struct unmap_notify { @@ -615,6 +624,15 @@ static int gntdev_open(struct inode *inode, struct file *flip) INIT_LIST_HEAD(&priv->freeable_maps); mutex_init(&priv->lock); +#ifdef CONFIG_XEN_GNTDEV_DMABUF + priv->dmabuf_priv = gntdev_dmabuf_init(); + if (IS_ERR(priv->dmabuf_priv)) { + ret = PTR_ERR(priv->dmabuf_priv); + kfree(priv); + return ret; + } +#endif + if (use_ptemod) { priv->mm = get_task_mm(current); if (!priv->mm) { @@ -664,8 +682,13 @@ static int gntdev_release(struct inode *inode, struct file *flip) WARN_ON(!list_empty(&priv->freeable_maps)); mutex_unlock(&priv->lock); +#ifdef CONFIG_XEN_GNTDEV_DMABUF + gntdev_dmabuf_fini(priv->dmabuf_priv); +#endif + if (use_ptemod) mmu_notifier_unregister(&priv->mn, priv->mm); + kfree(priv); return 0; } @@ -1035,6 +1058,111 @@ static long gntdev_ioctl_grant_copy(struct gntdev_priv *priv, void __user *u) return ret; } +#ifdef CONFIG_XEN_GNTDEV_DMABUF +/* ------------------------------------------------------------------ */ +/* DMA buffer export support. */ +/* ------------------------------------------------------------------ */ + +int gntdev_dmabuf_exp_from_refs(struct gntdev_priv *priv, int flags, + int count, u32 domid, u32 *refs, u32 *fd) +{ + /* XXX: this will need to work with gntdev's map, so leave it here. */ + *fd = -1; + return -EINVAL; +} + +/* ------------------------------------------------------------------ */ +/* DMA buffer IOCTL support. */ +/* ------------------------------------------------------------------ */ + +static long +gntdev_ioctl_dmabuf_exp_from_refs(struct gntdev_priv *priv, + struct ioctl_gntdev_dmabuf_exp_from_refs __user *u) +{ + struct ioctl_gntdev_dmabuf_exp_from_refs op; + u32 *refs; + long ret; + + if (copy_from_user(&op, u, sizeof(op)) != 0) + return -EFAULT; + + refs = kcalloc(op.count, sizeof(*refs), GFP_KERNEL); + if (!refs) + return -ENOMEM; + + if (copy_from_user(refs, u->refs, sizeof(*refs) * op.count) != 0) { + ret = -EFAULT; + goto out; + } + + ret = gntdev_dmabuf_exp_from_refs(priv, op.flags, op.count, + op.domid, refs, &op.fd); + if (ret) + goto out; + + if (copy_to_user(u, &op, sizeof(op)) != 0) + ret = -EFAULT; + +out: + kfree(refs); + return ret; +} + +static long +gntdev_ioctl_dmabuf_exp_wait_released(struct gntdev_priv *priv, + struct ioctl_gntdev_dmabuf_exp_wait_released __user *u) +{ + struct ioctl_gntdev_dmabuf_exp_wait_released op; + + if (copy_from_user(&op, u, sizeof(op)) != 0) + return -EFAULT; + + return gntdev_dmabuf_exp_wait_released(priv->dmabuf_priv, op.fd, + op.wait_to_ms); +} + +static long +gntdev_ioctl_dmabuf_imp_to_refs(struct gntdev_priv *priv, + struct ioctl_gntdev_dmabuf_imp_to_refs __user *u) +{ + struct ioctl_gntdev_dmabuf_imp_to_refs op; + struct gntdev_dmabuf *gntdev_dmabuf; + long ret; + + if (copy_from_user(&op, u, sizeof(op)) != 0) + return -EFAULT; + + gntdev_dmabuf = gntdev_dmabuf_imp_to_refs(priv->dmabuf_priv, + priv->dma_dev, op.fd, + op.count, op.domid); + if (IS_ERR(gntdev_dmabuf)) + return PTR_ERR(gntdev_dmabuf); + + if (copy_to_user(u->refs, gntdev_dmabuf_imp_get_refs(gntdev_dmabuf), + sizeof(*u->refs) * op.count) != 0) { + ret = -EFAULT; + goto out_release; + } + return 0; + +out_release: + gntdev_dmabuf_imp_release(priv->dmabuf_priv, op.fd); + return ret; +} + +static long +gntdev_ioctl_dmabuf_imp_release(struct gntdev_priv *priv, + struct ioctl_gntdev_dmabuf_imp_release __user *u) +{ + struct ioctl_gntdev_dmabuf_imp_release op; + + if (copy_from_user(&op, u, sizeof(op)) != 0) + return -EFAULT; + + return gntdev_dmabuf_imp_release(priv->dmabuf_priv, op.fd); +} +#endif + static long gntdev_ioctl(struct file *flip, unsigned int cmd, unsigned long arg) { @@ -1057,6 +1185,20 @@ static long gntdev_ioctl(struct file *flip, case IOCTL_GNTDEV_GRANT_COPY: return gntdev_ioctl_grant_copy(priv, ptr); +#ifdef CONFIG_XEN_GNTDEV_DMABUF + case IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS: + return gntdev_ioctl_dmabuf_exp_from_refs(priv, ptr); + + case IOCTL_GNTDEV_DMABUF_EXP_WAIT_RELEASED: + return gntdev_ioctl_dmabuf_exp_wait_released(priv, ptr); + + case IOCTL_GNTDEV_DMABUF_IMP_TO_REFS: + return gntdev_ioctl_dmabuf_imp_to_refs(priv, ptr); + + case IOCTL_GNTDEV_DMABUF_IMP_RELEASE: + return gntdev_ioctl_dmabuf_imp_release(priv, ptr); +#endif + default: pr_debug("priv %p, unknown cmd %x\n", priv, cmd); return -ENOIOCTLCMD; diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h index 4b9d498a31d4..fe4423e518c6 100644 --- a/include/uapi/xen/gntdev.h +++ b/include/uapi/xen/gntdev.h @@ -5,6 +5,7 @@ * Interface to /dev/xen/gntdev. * * Copyright (c) 2007, D G Murray + * Copyright (c) 2018, Oleksandr Andrushchenko, EPAM Systems Inc. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License version 2 @@ -215,4 +216,94 @@ struct ioctl_gntdev_grant_copy { */ #define GNTDEV_DMA_FLAG_COHERENT (1 << 1) +/* + * Create a dma-buf [1] from grant references @refs of count @count provided + * by the foreign domain @domid with flags @flags. + * + * By default dma-buf is backed by system memory pages, but by providing + * one of the GNTDEV_DMA_FLAG_XXX flags it can also be created as + * a DMA write-combine or coherent buffer, e.g. allocated with dma_alloc_wc/ + * dma_alloc_coherent. + * + * Returns 0 if dma-buf was successfully created and the corresponding + * dma-buf's file descriptor is returned in @fd. + * + * [1] Documentation/driver-api/dma-buf.rst + */ + +#define IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS \ + _IOC(_IOC_NONE, 'G', 9, \ + sizeof(struct ioctl_gntdev_dmabuf_exp_from_refs)) +struct ioctl_gntdev_dmabuf_exp_from_refs { + /* IN parameters. */ + /* Specific options for this dma-buf: see GNTDEV_DMA_FLAG_XXX. */ + __u32 flags; + /* Number of grant references in @refs array. */ + __u32 count; + /* OUT parameters. */ + /* File descriptor of the dma-buf. */ + __u32 fd; + /* The domain ID of the grant references to be mapped. */ + __u32 domid; + /* Variable IN parameter. */ + /* Array of grant references of size @count. */ + __u32 refs[1]; +}; + +/* + * This will block until the dma-buf with the file descriptor @fd is + * released. This is only valid for buffers created with + * IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS. + * + * If within @wait_to_ms milliseconds the buffer is not released + * then -ETIMEDOUT error is returned. + * If the buffer with the file descriptor @fd does not exist or has already + * been released, then -ENOENT is returned. For valid file descriptors + * this must not be treated as error. + */ +#define IOCTL_GNTDEV_DMABUF_EXP_WAIT_RELEASED \ + _IOC(_IOC_NONE, 'G', 10, \ + sizeof(struct ioctl_gntdev_dmabuf_exp_wait_released)) +struct ioctl_gntdev_dmabuf_exp_wait_released { + /* IN parameters */ + __u32 fd; + __u32 wait_to_ms; +}; + +/* + * Import a dma-buf with file descriptor @fd and export granted references + * to the pages of that dma-buf into array @refs of size @count. + */ +#define IOCTL_GNTDEV_DMABUF_IMP_TO_REFS \ + _IOC(_IOC_NONE, 'G', 11, \ + sizeof(struct ioctl_gntdev_dmabuf_imp_to_refs)) +struct ioctl_gntdev_dmabuf_imp_to_refs { + /* IN parameters. */ + /* File descriptor of the dma-buf. */ + __u32 fd; + /* Number of grant references in @refs array. */ + __u32 count; + /* The domain ID for which references to be granted. */ + __u32 domid; + /* Reserved - must be zero. */ + __u32 reserved; + /* OUT parameters. */ + /* Array of grant references of size @count. */ + __u32 refs[1]; +}; + +/* + * This will close all references to the imported buffer with file descriptor + * @fd, so it can be released by the owner. This is only valid for buffers + * created with IOCTL_GNTDEV_DMABUF_IMP_TO_REFS. + */ +#define IOCTL_GNTDEV_DMABUF_IMP_RELEASE \ + _IOC(_IOC_NONE, 'G', 12, \ + sizeof(struct ioctl_gntdev_dmabuf_imp_release)) +struct ioctl_gntdev_dmabuf_imp_release { + /* IN parameters */ + __u32 fd; + __u32 reserved; +}; + #endif /* __LINUX_PUBLIC_GNTDEV_H__ */