Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
Thanks Arnd for the review comments,
On 30/11/18 13:26, Arnd Bergmann wrote:
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
+
+static int fastrpc_init_process(struct fastrpc_user *fl,
+ struct fastrpc_ioctl_init *init)
+{
+ struct fastrpc_ioctl_invoke *ioctl;
+ struct fastrpc_phy_page pages[1];
+ struct fastrpc_map *file = NULL, *mem = NULL;
+ struct fastrpc_buf *imem = NULL;
+ int err = 0;
+
+ ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
+ if (!ioctl)
+ return -ENOMEM;
+
+ if (init->flags == FASTRPC_INIT_ATTACH) {
+ remote_arg_t ra[1];
+ int tgid = fl->tgid;
+
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(tgid);
+ ioctl->handle = 1;
+ ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
+ ioctl->pra = ra;
+ fl->pd = 0;
+
+ err = fastrpc_internal_invoke(fl, 1, ioctl);
+ if (err)
+ goto bail;
It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?
Yes, I totally agree with you, Will rework this part as suggested.
+ } else if (init->flags == FASTRPC_INIT_CREATE) {
How about splitting each flags== case into a separate function?
Once I move this to a command code then make this a separate function.
diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
index 8fec66601337..6b596fc7ddf3 100644
--- a/include/uapi/linux/fastrpc.h
+++ b/include/uapi/linux/fastrpc.h
@@ -6,6 +6,12 @@
#include
#define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke)
+#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init)
+
+/* INIT a new process or attach to guestos */
+#define FASTRPC_INIT_ATTACH 0
+#define FASTRPC_INIT_CREATE 1
+#define FASTRPC_INIT_CREATE_STATIC 2
Maybe use three command codes here, and remove the 'flags' member?
Make sense, will do it in next version.
@@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
unsigned int *crc;
};
+struct fastrpc_ioctl_init {
+ uint32_t flags; /* one of FASTRPC_INIT_* macros */
+ uintptr_t file; /* pointer to elf file */
+ uint32_t filelen; /* elf file length */
+ int32_t filefd; /* ION fd for the file */
What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.
Yes, the driver just uses dma_buf, it looks like leftover from downstream!
+ uintptr_t mem; /* mem for the PD */
+ uint32_t memlen;/* mem length */
+ int32_t memfd; /* fd for the mem */
+ int attrs;
+ unsigned int siglen;
+};
This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.
Sure, Will recheck all the structures before sending next version!
--srini
Arnd
Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
Thanks Arnd for the review comments,
On 30/11/18 13:26, Arnd Bergmann wrote:
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
+
+static int fastrpc_init_process(struct fastrpc_user *fl,
+ struct fastrpc_ioctl_init *init)
+{
+ struct fastrpc_ioctl_invoke *ioctl;
+ struct fastrpc_phy_page pages[1];
+ struct fastrpc_map *file = NULL, *mem = NULL;
+ struct fastrpc_buf *imem = NULL;
+ int err = 0;
+
+ ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
+ if (!ioctl)
+ return -ENOMEM;
+
+ if (init->flags == FASTRPC_INIT_ATTACH) {
+ remote_arg_t ra[1];
+ int tgid = fl->tgid;
+
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(tgid);
+ ioctl->handle = 1;
+ ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
+ ioctl->pra = ra;
+ fl->pd = 0;
+
+ err = fastrpc_internal_invoke(fl, 1, ioctl);
+ if (err)
+ goto bail;
It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?
Yes, I totally agree with you, Will rework this part as suggested.
+ } else if (init->flags == FASTRPC_INIT_CREATE) {
How about splitting each flags== case into a separate function?
Once I move this to a command code then make this a separate function.
diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
index 8fec66601337..6b596fc7ddf3 100644
--- a/include/uapi/linux/fastrpc.h
+++ b/include/uapi/linux/fastrpc.h
@@ -6,6 +6,12 @@
#include
#define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke)
+#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init)
+
+/* INIT a new process or attach to guestos */
+#define FASTRPC_INIT_ATTACH 0
+#define FASTRPC_INIT_CREATE 1
+#define FASTRPC_INIT_CREATE_STATIC 2
Maybe use three command codes here, and remove the 'flags' member?
Make sense, will do it in next version.
@@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
unsigned int *crc;
};
+struct fastrpc_ioctl_init {
+ uint32_t flags; /* one of FASTRPC_INIT_* macros */
+ uintptr_t file; /* pointer to elf file */
+ uint32_t filelen; /* elf file length */
+ int32_t filefd; /* ION fd for the file */
What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.
Yes, the driver just uses dma_buf, it looks like leftover from downstream!
+ uintptr_t mem; /* mem for the PD */
+ uint32_t memlen;/* mem length */
+ int32_t memfd; /* fd for the mem */
+ int attrs;
+ unsigned int siglen;
+};
This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.
Sure, Will recheck all the structures before sending next version!
--srini
Arnd
Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
> +
> +static int fastrpc_init_process(struct fastrpc_user *fl,
> + struct fastrpc_ioctl_init *init)
> +{
> + struct fastrpc_ioctl_invoke *ioctl;
> + struct fastrpc_phy_page pages[1];
> + struct fastrpc_map *file = NULL, *mem = NULL;
> + struct fastrpc_buf *imem = NULL;
> + int err = 0;
> +
> + ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
> + if (!ioctl)
> + return -ENOMEM;
> +
> + if (init->flags == FASTRPC_INIT_ATTACH) {
> + remote_arg_t ra[1];
> + int tgid = fl->tgid;
> +
> + ra[0].buf.pv = (void *)
> + ra[0].buf.len = sizeof(tgid);
> + ioctl->handle = 1;
> + ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> + ioctl->pra = ra;
> + fl->pd = 0;
> +
> + err = fastrpc_internal_invoke(fl, 1, ioctl);
> + if (err)
> + goto bail;
It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?
> + } else if (init->flags == FASTRPC_INIT_CREATE) {
How about splitting each flags== case into a separate function?
> diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
> index 8fec66601337..6b596fc7ddf3 100644
> --- a/include/uapi/linux/fastrpc.h
> +++ b/include/uapi/linux/fastrpc.h
> @@ -6,6 +6,12 @@
> #include
>
> #define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke)
> +#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init)
> +
> +/* INIT a new process or attach to guestos */
> +#define FASTRPC_INIT_ATTACH 0
> +#define FASTRPC_INIT_CREATE 1
> +#define FASTRPC_INIT_CREATE_STATIC 2
Maybe use three command codes here, and remove the 'flags' member?
> @@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
> unsigned int *crc;
> };
>
> +struct fastrpc_ioctl_init {
> + uint32_t flags; /* one of FASTRPC_INIT_* macros */
> + uintptr_t file; /* pointer to elf file */
> + uint32_t filelen; /* elf file length */
> + int32_t filefd; /* ION fd for the file */
What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.
> + uintptr_t mem; /* mem for the PD */
> + uint32_t memlen;/* mem length */
> + int32_t memfd; /* fd for the mem */
> + int attrs;
> + unsigned int siglen;
> +};
This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.
Arnd
Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
wrote:
> +
> +static int fastrpc_init_process(struct fastrpc_user *fl,
> + struct fastrpc_ioctl_init *init)
> +{
> + struct fastrpc_ioctl_invoke *ioctl;
> + struct fastrpc_phy_page pages[1];
> + struct fastrpc_map *file = NULL, *mem = NULL;
> + struct fastrpc_buf *imem = NULL;
> + int err = 0;
> +
> + ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
> + if (!ioctl)
> + return -ENOMEM;
> +
> + if (init->flags == FASTRPC_INIT_ATTACH) {
> + remote_arg_t ra[1];
> + int tgid = fl->tgid;
> +
> + ra[0].buf.pv = (void *)
> + ra[0].buf.len = sizeof(tgid);
> + ioctl->handle = 1;
> + ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> + ioctl->pra = ra;
> + fl->pd = 0;
> +
> + err = fastrpc_internal_invoke(fl, 1, ioctl);
> + if (err)
> + goto bail;
It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?
> + } else if (init->flags == FASTRPC_INIT_CREATE) {
How about splitting each flags== case into a separate function?
> diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
> index 8fec66601337..6b596fc7ddf3 100644
> --- a/include/uapi/linux/fastrpc.h
> +++ b/include/uapi/linux/fastrpc.h
> @@ -6,6 +6,12 @@
> #include
>
> #define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke)
> +#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init)
> +
> +/* INIT a new process or attach to guestos */
> +#define FASTRPC_INIT_ATTACH 0
> +#define FASTRPC_INIT_CREATE 1
> +#define FASTRPC_INIT_CREATE_STATIC 2
Maybe use three command codes here, and remove the 'flags' member?
> @@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
> unsigned int *crc;
> };
>
> +struct fastrpc_ioctl_init {
> + uint32_t flags; /* one of FASTRPC_INIT_* macros */
> + uintptr_t file; /* pointer to elf file */
> + uint32_t filelen; /* elf file length */
> + int32_t filefd; /* ION fd for the file */
What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.
> + uintptr_t mem; /* mem for the PD */
> + uint32_t memlen;/* mem length */
> + int32_t memfd; /* fd for the mem */
> + int attrs;
> + unsigned int siglen;
> +};
This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.
Arnd
[RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
This patch adds support to create or attach remote shell process.
The shell process called fastrpc_shell_0 is usually loaded on the DSP
when a user process is spawned.
Most of the work is derived from various downstream Qualcomm kernels.
Credits to various Qualcomm authors who have contributed to this code.
Specially Tharun Kumar Merugu
Signed-off-by: Srinivas Kandagatla
---
drivers/char/fastrpc.c | 172 +++
include/uapi/linux/fastrpc.h | 18
2 files changed, 190 insertions(+)
diff --git a/drivers/char/fastrpc.c b/drivers/char/fastrpc.c
index 5bb224adc24f..3630e883d3f4 100644
--- a/drivers/char/fastrpc.c
+++ b/drivers/char/fastrpc.c
@@ -30,6 +30,8 @@
#define FASTRPC_PHYS(p)(p & 0x)
#define FASTRPC_CTX_MAX (256)
#define FASTRPC_CTXID_MASK (0xFF0)
+#define INIT_FILELEN_MAX (2*1024*1024)
+#define INIT_MEMLEN_MAX (8*1024*1024)
#define FASTRPC_DEVICE_NAME"fastrpc"
/* Retrives number of input buffers from the scalars parameter */
@@ -59,6 +61,14 @@
#define FASTRPC_SCALARS(method, in, out) \
FASTRPC_BUILD_SCALARS(0, method, in, out, 0, 0)
+
+/* Remote Method id table */
+#define FASTRPC_RMID_INIT_ATTACH 0
+#define FASTRPC_RMID_INIT_RELEASE 1
+#define FASTRPC_RMID_INIT_CREATE 6
+#define FASTRPC_RMID_INIT_CREATE_ATTR 7
+#define FASTRPC_RMID_INIT_CREATE_STATIC8
+
#define cdev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, cdev)
static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
@@ -735,6 +745,130 @@ static int fastrpc_internal_invoke(struct fastrpc_user
*fl,
return err;
}
+
+static int fastrpc_init_process(struct fastrpc_user *fl,
+ struct fastrpc_ioctl_init *init)
+{
+ struct fastrpc_ioctl_invoke *ioctl;
+ struct fastrpc_phy_page pages[1];
+ struct fastrpc_map *file = NULL, *mem = NULL;
+ struct fastrpc_buf *imem = NULL;
+ int err = 0;
+
+ ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
+ if (!ioctl)
+ return -ENOMEM;
+
+ if (init->flags == FASTRPC_INIT_ATTACH) {
+ remote_arg_t ra[1];
+ int tgid = fl->tgid;
+
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(tgid);
+ ioctl->handle = 1;
+ ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
+ ioctl->pra = ra;
+ fl->pd = 0;
+
+ err = fastrpc_internal_invoke(fl, 1, ioctl);
+ if (err)
+ goto bail;
+ } else if (init->flags == FASTRPC_INIT_CREATE) {
+ int memlen;
+ remote_arg_t ra[6];
+ int fds[6];
+ struct {
+ int pgid;
+ unsigned int namelen;
+ unsigned int filelen;
+ unsigned int pageslen;
+ int attrs;
+ int siglen;
+ } inbuf;
+
+ inbuf.pgid = fl->tgid;
+ inbuf.namelen = strlen(current->comm) + 1;
+ inbuf.filelen = init->filelen;
+ fl->pd = 1;
+
+ if (init->filelen) {
+ err = fastrpc_map_create(fl, init->filefd,
+ init->file, init->filelen,
+ );
+ if (err)
+ goto bail;
+ }
+ inbuf.pageslen = 1;
+
+ if (init->mem) {
+ err = -EINVAL;
+ pr_err("adsprpc: %s: %s: ERROR: donated memory
allocated in userspace\n",
+ current->comm, __func__);
+ goto bail;
+ }
+ memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init->filelen * 4),
+ 1024 * 1024);
+ err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
+);
+ if (err)
+ goto bail;
+
+ fl->init_mem = imem;
+ inbuf.pageslen = 1;
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(inbuf);
+ fds[0] = 0;
+
+ ra[1].buf.pv = (void *)current->comm;
+ ra[1].buf.len = inbuf.namelen;
+ fds[1] = 0;
+
+ ra[2].buf.pv = (void *)init->file;
+ ra[2].buf.len = inbuf.filelen;
+ fds[2] = init->filefd;
+
+ pages[0].addr = imem->phys;
+ pages[0].size = imem->size;
+
+ ra[3].buf.pv = (void *)pages;
+ ra[3].buf.len = 1 * sizeof(*pages);
+ fds[3] = 0;
+
+ inbuf.attrs = init->attrs;
+ ra[4].buf.pv = (void *)&(inbuf.attrs);
+ ra[4].buf.len = sizeof(inbuf.attrs);
+
[RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
This patch adds support to create or attach remote shell process.
The shell process called fastrpc_shell_0 is usually loaded on the DSP
when a user process is spawned.
Most of the work is derived from various downstream Qualcomm kernels.
Credits to various Qualcomm authors who have contributed to this code.
Specially Tharun Kumar Merugu
Signed-off-by: Srinivas Kandagatla
---
drivers/char/fastrpc.c | 172 +++
include/uapi/linux/fastrpc.h | 18
2 files changed, 190 insertions(+)
diff --git a/drivers/char/fastrpc.c b/drivers/char/fastrpc.c
index 5bb224adc24f..3630e883d3f4 100644
--- a/drivers/char/fastrpc.c
+++ b/drivers/char/fastrpc.c
@@ -30,6 +30,8 @@
#define FASTRPC_PHYS(p)(p & 0x)
#define FASTRPC_CTX_MAX (256)
#define FASTRPC_CTXID_MASK (0xFF0)
+#define INIT_FILELEN_MAX (2*1024*1024)
+#define INIT_MEMLEN_MAX (8*1024*1024)
#define FASTRPC_DEVICE_NAME"fastrpc"
/* Retrives number of input buffers from the scalars parameter */
@@ -59,6 +61,14 @@
#define FASTRPC_SCALARS(method, in, out) \
FASTRPC_BUILD_SCALARS(0, method, in, out, 0, 0)
+
+/* Remote Method id table */
+#define FASTRPC_RMID_INIT_ATTACH 0
+#define FASTRPC_RMID_INIT_RELEASE 1
+#define FASTRPC_RMID_INIT_CREATE 6
+#define FASTRPC_RMID_INIT_CREATE_ATTR 7
+#define FASTRPC_RMID_INIT_CREATE_STATIC8
+
#define cdev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, cdev)
static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
@@ -735,6 +745,130 @@ static int fastrpc_internal_invoke(struct fastrpc_user
*fl,
return err;
}
+
+static int fastrpc_init_process(struct fastrpc_user *fl,
+ struct fastrpc_ioctl_init *init)
+{
+ struct fastrpc_ioctl_invoke *ioctl;
+ struct fastrpc_phy_page pages[1];
+ struct fastrpc_map *file = NULL, *mem = NULL;
+ struct fastrpc_buf *imem = NULL;
+ int err = 0;
+
+ ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
+ if (!ioctl)
+ return -ENOMEM;
+
+ if (init->flags == FASTRPC_INIT_ATTACH) {
+ remote_arg_t ra[1];
+ int tgid = fl->tgid;
+
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(tgid);
+ ioctl->handle = 1;
+ ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
+ ioctl->pra = ra;
+ fl->pd = 0;
+
+ err = fastrpc_internal_invoke(fl, 1, ioctl);
+ if (err)
+ goto bail;
+ } else if (init->flags == FASTRPC_INIT_CREATE) {
+ int memlen;
+ remote_arg_t ra[6];
+ int fds[6];
+ struct {
+ int pgid;
+ unsigned int namelen;
+ unsigned int filelen;
+ unsigned int pageslen;
+ int attrs;
+ int siglen;
+ } inbuf;
+
+ inbuf.pgid = fl->tgid;
+ inbuf.namelen = strlen(current->comm) + 1;
+ inbuf.filelen = init->filelen;
+ fl->pd = 1;
+
+ if (init->filelen) {
+ err = fastrpc_map_create(fl, init->filefd,
+ init->file, init->filelen,
+ );
+ if (err)
+ goto bail;
+ }
+ inbuf.pageslen = 1;
+
+ if (init->mem) {
+ err = -EINVAL;
+ pr_err("adsprpc: %s: %s: ERROR: donated memory
allocated in userspace\n",
+ current->comm, __func__);
+ goto bail;
+ }
+ memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init->filelen * 4),
+ 1024 * 1024);
+ err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
+);
+ if (err)
+ goto bail;
+
+ fl->init_mem = imem;
+ inbuf.pageslen = 1;
+ ra[0].buf.pv = (void *)
+ ra[0].buf.len = sizeof(inbuf);
+ fds[0] = 0;
+
+ ra[1].buf.pv = (void *)current->comm;
+ ra[1].buf.len = inbuf.namelen;
+ fds[1] = 0;
+
+ ra[2].buf.pv = (void *)init->file;
+ ra[2].buf.len = inbuf.filelen;
+ fds[2] = init->filefd;
+
+ pages[0].addr = imem->phys;
+ pages[0].size = imem->size;
+
+ ra[3].buf.pv = (void *)pages;
+ ra[3].buf.len = 1 * sizeof(*pages);
+ fds[3] = 0;
+
+ inbuf.attrs = init->attrs;
+ ra[4].buf.pv = (void *)&(inbuf.attrs);
+ ra[4].buf.len = sizeof(inbuf.attrs);
+
