[PATCH v2 0/8] misc: fastrpc: Add missing DSP FastRPC features

[PATCH v2 0/8] misc: fastrpc: Add missing DSP FastRPC features

[Srinivas Kandagatla]

This patchset adds below DSP FastRPC features that have been missing in
upstream fastrpc driver and also cleans up channel context structure with kref.

- Add ablity to reflect if the DSP domain is secure/unsecure by creating
 seperate device nodes for secured domain, this would used by SE policy
 to restrict applications loading process on the DSP.
- Add new IOCTL to get DSP capabilites
- Add IOCTL to support mapping memory on the DSP.

Tested this series on DragonBoard 845c with TensorFlow.

dt bindings patch has dependency this yaml conversion patch:
"dt-bindings: misc: fastrpc convert bindings to yaml"
https://lore.kernel.org/lkml/20211208101508.24582-1-david@ixit.cz/T/

Jeya R (6):
  misc: fastrpc: add remote process attributes
  misc: fastrpc: add support for FASTRPC_IOCTL_MEM_MAP/UNMAP
  misc: fastrpc: Add support to get DSP capabilities
  dt-bindings: misc: add property to support non-secure DSP
  misc: fastrpc: check before loading process to the DSP
  arm64: dts: qcom: add non-secure domain property to fastrpc nodes

Srinivas Kandagatla (2):
  misc: fastrpc: separate fastrpc device from channel context
  misc: fastrpc: add secure domain support

 .../bindings/misc/qcom,fastrpc.yaml           |   5 +
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |   1 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   2 +
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   3 +
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |   3 +
 arch/arm64/boot/dts/qcom/sm8350.dtsi          |   3 +
 drivers/misc/fastrpc.c                        | 390 +++++++++++++++++-
 include/uapi/misc/fastrpc.h                   |  76 ++++
 8 files changed, 470 insertions(+), 13 deletions(-)

-- 
2.21.0

[PATCH v2 1/8] misc: fastrpc: separate fastrpc device from channel context

[Srinivas Kandagatla]

Currently fastrpc misc device instance is within channel context struct
with a kref. So we have 2 structs with refcount, both of them managing the
same channel context structure.

Separate fastrpc device from channel context and by adding a dedicated
fastrpc_device structure, this should clean the structures a bit and also help
when adding secure device node support.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 48 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 39aca7753719..71d818fed8b8 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -78,7 +78,7 @@
 #define USER_PD		(1)
 #define SENSORS_PD	(2)
 
-#define miscdev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, miscdev)
+#define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
 
 static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
 						"sdsp", "cdsp"};
@@ -212,8 +212,13 @@ struct fastrpc_channel_ctx {
 	spinlock_t lock;
 	struct idr ctx_idr;
 	struct list_head users;
-	struct miscdevice miscdev;
 	struct kref refcount;
+	struct fastrpc_device *fdevice;
+};
+
+struct fastrpc_device {
+	struct fastrpc_channel_ctx *cctx;
+	struct miscdevice miscdev;
 };
 
 struct fastrpc_user {
@@ -1218,10 +1223,14 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 
 static int fastrpc_device_open(struct inode *inode, struct file *filp)
 {
-	struct fastrpc_channel_ctx *cctx = miscdev_to_cctx(filp->private_data);
+	struct fastrpc_channel_ctx *cctx = NULL;
+	struct fastrpc_device *fdevice = NULL;
 	struct fastrpc_user *fl = NULL;
 	unsigned long flags;
 
+	fdevice = miscdev_to_fdevice(filp->private_data);
+	cctx = fdevice->cctx;
+
 	fl = kzalloc(sizeof(*fl), GFP_KERNEL);
 	if (!fl)
 		return -ENOMEM;
@@ -1606,6 +1615,29 @@ static struct platform_driver fastrpc_cb_driver = {
 	},
 };
 
+static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
+				   const char *domain)
+{
+	struct fastrpc_device *fdev;
+	int err;
+
+	fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
+	if (!fdev)
+		return -ENOMEM;
+
+	fdev->cctx = cctx;
+	fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
+	fdev->miscdev.fops = &fastrpc_fops;
+	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
+	err = misc_register(&fdev->miscdev);
+	if (err)
+		kfree(fdev);
+	else
+		cctx->fdevice = fdev;
+
+	return err;
+}
+
 static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 {
 	struct device *rdev = &rpdev->dev;
@@ -1635,11 +1667,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (!data)
 		return -ENOMEM;
 
-	data->miscdev.minor = MISC_DYNAMIC_MINOR;
-	data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s",
-					    domains[domain_id]);
-	data->miscdev.fops = &fastrpc_fops;
-	err = misc_register(&data->miscdev);
+	err = fastrpc_device_register(rdev, data, domains[domain_id]);
 	if (err) {
 		kfree(data);
 		return err;
@@ -1679,7 +1707,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 		fastrpc_notify_users(user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
-	misc_deregister(&cctx->miscdev);
+	if (cctx->fdevice)
+		misc_deregister(&cctx->fdevice->miscdev);
+
 	of_platform_depopulate(&rpdev->dev);
 
 	cctx->rpdev = NULL;
-- 
2.21.0

[PATCH v2 2/8] misc: fastrpc: add remote process attributes

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

Add fastrpc remote process attributes. These attributes are passed as
part of process create ioctl request.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/uapi/misc/fastrpc.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index 0a89f95463f6..b74407d19ed5 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -14,6 +14,23 @@
 #define FASTRPC_IOCTL_MUNMAP		_IOWR('R', 7, struct fastrpc_req_munmap)
 #define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)
 
+enum fastrpc_proc_attr {
+	/* Macro for Debug attr */
+	FASTRPC_MODE_DEBUG		= (1 << 0),
+	/* Macro for Ptrace */
+	FASTRPC_MODE_PTRACE		= (1 << 1),
+	/* Macro for CRC Check */
+	FASTRPC_MODE_CRC		= (1 << 2),
+	/* Macro for Unsigned PD */
+	FASTRPC_MODE_UNSIGNED_MODULE	= (1 << 3),
+	/* Macro for Adaptive QoS */
+	FASTRPC_MODE_ADAPTIVE_QOS	= (1 << 4),
+	/* Macro for System Process */
+	FASTRPC_MODE_SYSTEM_PROCESS	= (1 << 5),
+	/* Macro for Prvileged Process */
+	FASTRPC_MODE_PRIVILEGED		= (1 << 6),
+};
+
 struct fastrpc_invoke_args {
 	__u64 ptr;
 	__u64 length;
-- 
2.21.0

[PATCH v2 3/8] misc: fastrpc: add support for FASTRPC_IOCTL_MEM_MAP/UNMAP

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

Add support for IOCTL requests to map and unmap on DSP based on map
flags.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c      | 155 ++++++++++++++++++++++++++++++++++++
 include/uapi/misc/fastrpc.h |  51 ++++++++++++
 2 files changed, 206 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 71d818fed8b8..c2f194dc0e66 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -72,6 +72,8 @@
 #define FASTRPC_RMID_INIT_CREATE	6
 #define FASTRPC_RMID_INIT_CREATE_ATTR	7
 #define FASTRPC_RMID_INIT_CREATE_STATIC	8
+#define FASTRPC_RMID_INIT_MEM_MAP      10
+#define FASTRPC_RMID_INIT_MEM_UNMAP    11
 
 /* Protection Domain(PD) ids */
 #define AUDIO_PD	(0) /* also GUEST_OS PD? */
@@ -108,12 +110,29 @@ struct fastrpc_mmap_req_msg {
 	s32 num;
 };
 
+struct fastrpc_mem_map_req_msg {
+	s32 pgid;
+	s32 fd;
+	s32 offset;
+	u32 flags;
+	u64 vaddrin;
+	s32 num;
+	s32 data_len;
+};
+
 struct fastrpc_munmap_req_msg {
 	s32 pgid;
 	u64 vaddr;
 	u64 size;
 };
 
+struct fastrpc_mem_unmap_req_msg {
+	s32 pgid;
+	s32 fd;
+	u64 vaddrin;
+	u64 len;
+};
+
 struct fastrpc_msg {
 	int pid;		/* process group id */
 	int tid;		/* thread id */
@@ -170,6 +189,7 @@ struct fastrpc_map {
 	u64 size;
 	void *va;
 	u64 len;
+	u64 raddr;
 	struct kref refcount;
 };
 
@@ -1491,6 +1511,135 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 	return err;
 }
 
+static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_mem_unmap *req)
+{
+	struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
+	struct fastrpc_map *map = NULL, *m;
+	struct fastrpc_mem_unmap_req_msg req_msg = { 0 };
+	int err = 0;
+	u32 sc;
+	struct device *dev = fl->sctx->dev;
+
+	spin_lock(&fl->lock);
+	list_for_each_entry_safe(map, m, &fl->maps, node) {
+		if ((req->fd < 0 || map->fd == req->fd) && (map->raddr == req->vaddr))
+			break;
+		map = NULL;
+	}
+
+	spin_unlock(&fl->lock);
+
+	if (!map) {
+		dev_err(dev, "map not in list\n");
+		return -EINVAL;
+	}
+
+	req_msg.pgid = fl->tgid;
+	req_msg.len = map->len;
+	req_msg.vaddrin = map->raddr;
+	req_msg.fd = map->fd;
+
+	args[0].ptr = (u64) &req_msg;
+	args[0].length = sizeof(req_msg);
+
+	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0);
+	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc,
+				      &args[0]);
+	fastrpc_map_put(map);
+	if (err)
+		dev_err(dev, "unmmap\tpt fd = %d, 0x%09llx error\n",  map->fd, map->raddr);
+
+	return err;
+}
+
+static int fastrpc_req_mem_unmap(struct fastrpc_user *fl, char __user *argp)
+{
+	struct fastrpc_mem_unmap req;
+
+	if (copy_from_user(&req, argp, sizeof(req)))
+		return -EFAULT;
+
+	return fastrpc_req_mem_unmap_impl(fl, &req);
+}
+
+static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
+{
+	struct fastrpc_invoke_args args[4] = { [0 ... 3] = { 0 } };
+	struct fastrpc_mem_map_req_msg req_msg = { 0 };
+	struct fastrpc_mmap_rsp_msg rsp_msg = { 0 };
+	struct fastrpc_mem_unmap req_unmap = { 0 };
+	struct fastrpc_phy_page pages = { 0 };
+	struct fastrpc_mem_map req;
+	struct device *dev = fl->sctx->dev;
+	struct fastrpc_map *map = NULL;
+	int err;
+	u32 sc;
+
+	if (copy_from_user(&req, argp, sizeof(req)))
+		return -EFAULT;
+
+	/* create SMMU mapping */
+	err = fastrpc_map_create(fl, req.fd, req.length, &map);
+	if (err) {
+		dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
+		return err;
+	}
+
+	req_msg.pgid = fl->tgid;
+	req_msg.fd = req.fd;
+	req_msg.offset = req.offset;
+	req_msg.vaddrin = req.vaddrin;
+	map->va = (void *) req.vaddrin;
+	req_msg.flags = req.flags;
+	req_msg.num = sizeof(pages);
+	req_msg.data_len = 0;
+
+	args[0].ptr = (u64) &req_msg;
+	args[0].length = sizeof(req_msg);
+
+	pages.addr = map->phys;
+	pages.size = map->size;
+
+	args[1].ptr = (u64) &pages;
+	args[1].length = sizeof(pages);
+
+	args[2].ptr = (u64) &pages;
+	args[2].length = 0;
+
+	args[3].ptr = (u64) &rsp_msg;
+	args[3].length = sizeof(rsp_msg);
+
+	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_MEM_MAP, 3, 1);
+	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]);
+	if (err) {
+		dev_err(dev, "mem mmap error, fd %d, vaddr %llx, size %lld\n",
+			req.fd, req.vaddrin, map->size);
+		goto err_invoke;
+	}
+
+	/* update the buffer to be able to deallocate the memory on the DSP */
+	map->raddr = rsp_msg.vaddr;
+
+	/* let the client know the address to use */
+	req.vaddrout = rsp_msg.vaddr;
+
+	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
+		/* unmap the memory and release the buffer */
+		req_unmap.vaddr = (uintptr_t) rsp_msg.vaddr;
+		req_unmap.length = map->size;
+		fastrpc_req_mem_unmap_impl(fl, &req_unmap);
+		return -EFAULT;
+	}
+
+	return 0;
+
+err_invoke:
+	if (map)
+		fastrpc_map_put(map);
+
+	return err;
+}
+
 static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg)
 {
@@ -1520,6 +1669,12 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 	case FASTRPC_IOCTL_MUNMAP:
 		err = fastrpc_req_munmap(fl, argp);
 		break;
+	case FASTRPC_IOCTL_MEM_MAP:
+		err = fastrpc_req_mem_map(fl, argp);
+		break;
+	case FASTRPC_IOCTL_MEM_UNMAP:
+		err = fastrpc_req_mem_unmap(fl, argp);
+		break;
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index b74407d19ed5..2308650e4a6e 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -13,6 +13,37 @@
 #define FASTRPC_IOCTL_MMAP		_IOWR('R', 6, struct fastrpc_req_mmap)
 #define FASTRPC_IOCTL_MUNMAP		_IOWR('R', 7, struct fastrpc_req_munmap)
 #define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)
+#define FASTRPC_IOCTL_MEM_MAP		_IOWR('R', 10, struct fastrpc_mem_map)
+#define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
+
+/**
+ * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
+ * @FASTRPC_MAP_STATIC: Map memory pages with RW- permission and CACHE WRITEBACK.
+ * The driver is responsible for cache maintenance when passed
+ * the buffer to FastRPC calls. Same virtual address will be
+ * assigned for subsequent FastRPC calls.
+ * @FASTRPC_MAP_RESERVED: Reserved
+ * @FASTRPC_MAP_FD: Map memory pages with RW- permission and CACHE WRITEBACK.
+ * Mapping tagged with a file descriptor. User is responsible for
+ * CPU and DSP cache maintenance for the buffer. Get virtual address
+ * of buffer on DSP using HAP_mmap_get() and HAP_mmap_put() APIs.
+ * @FASTRPC_MAP_FD_DELAYED: Mapping delayed until user call HAP_mmap() and HAP_munmap()
+ * functions on DSP. It is useful to map a buffer with cache modes
+ * other than default modes. User is responsible for CPU and DSP
+ * cache maintenance for the buffer.
+ * @FASTRPC_MAP_FD_NOMAP: This flag is used to skip CPU mapping,
+ * otherwise behaves similar to FASTRPC_MAP_FD_DELAYED flag.
+ * @FASTRPC_MAP_MAX: max count for flags
+ *
+ */
+enum fastrpc_map_flags {
+	FASTRPC_MAP_STATIC = 0,
+	FASTRPC_MAP_RESERVED,
+	FASTRPC_MAP_FD = 2,
+	FASTRPC_MAP_FD_DELAYED,
+	FASTRPC_MAP_FD_NOMAP = 16,
+	FASTRPC_MAP_MAX,
+};
 
 enum fastrpc_proc_attr {
 	/* Macro for Debug attr */
@@ -66,9 +97,29 @@ struct fastrpc_req_mmap {
 	__u64 vaddrout;	/* dsp virtual address */
 };
 
+struct fastrpc_mem_map {
+	__s32 version;
+	__s32 fd;		/* fd */
+	__s32 offset;		/* buffer offset */
+	__u32 flags;		/* flags defined in enum fastrpc_map_flags */
+	__u64 vaddrin;		/* buffer virtual address */
+	__u64 length;		/* buffer length */
+	__u64 vaddrout;		/* [out] remote virtual address */
+	__s32 attrs;		/* buffer attributes used for SMMU mapping */
+	__s32 reserved[4];
+};
+
 struct fastrpc_req_munmap {
 	__u64 vaddrout;	/* address to unmap */
 	__u64 size;	/* size */
 };
 
+struct fastrpc_mem_unmap {
+	__s32 vesion;
+	__s32 fd;		/* fd */
+	__u64 vaddr;		/* remote process (dsp) virtual address */
+	__u64 length;		/* buffer size */
+	__s32 reserved[5];
+};
+
 #endif /* __QCOM_FASTRPC_H__ */
-- 
2.21.0

[PATCH v2 4/8] misc: fastrpc: Add support to get DSP capabilities

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

Add support to get DSP capabilities. The capability information is cached
on driver.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c      | 105 ++++++++++++++++++++++++++++++++++++
 include/uapi/misc/fastrpc.h |   8 +++
 2 files changed, 113 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c2f194dc0e66..79fc59caacef 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -31,10 +31,14 @@
 #define FASTRPC_PHYS(p)	((p) & 0xffffffff)
 #define FASTRPC_CTX_MAX (256)
 #define FASTRPC_INIT_HANDLE	1
+#define FASTRPC_DSP_UTILITIES_HANDLE	2
 #define FASTRPC_CTXID_MASK (0xFF0)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
 #define ADSP_MMAP_ADD_PAGES 0x1000
+#define DSP_UNSUPPORTED_API (0x80000414)
+/* MAX NUMBER of DSP ATTRIBUTES SUPPORTED */
+#define FASTRPC_MAX_DSP_ATTRIBUTES (256)
 
 /* Retrives number of input buffers from the scalars parameter */
 #define REMOTE_SCALARS_INBUFS(sc)	(((sc) >> 16) & 0x0ff)
@@ -233,6 +237,9 @@ struct fastrpc_channel_ctx {
 	struct idr ctx_idr;
 	struct list_head users;
 	struct kref refcount;
+	/* Flag if dsp attributes are cached */
+	bool valid_attributes;
+	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
 	struct fastrpc_device *fdevice;
 };
 
@@ -1369,6 +1376,101 @@ static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
 	return err;
 }
 
+static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr_buf,
+				     uint32_t dsp_attr_buf_len)
+{
+	struct fastrpc_invoke_args args[2] = { 0 };
+
+	/* Capability filled in userspace */
+	dsp_attr_buf[0] = 0;
+
+	args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
+	args[0].length = sizeof(dsp_attr_buf_len);
+	args[0].fd = -1;
+	args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
+	args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
+	args[1].fd = -1;
+	fl->pd = 1;
+
+	return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
+				       FASTRPC_SCALARS(0, 1, 1), args);
+}
+
+static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
+					struct fastrpc_user *fl)
+{
+	struct fastrpc_channel_ctx *cctx = fl->cctx;
+	uint32_t attribute_id = cap->attribute_id;
+	uint32_t dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
+	unsigned long flags;
+	uint32_t domain = cap->domain;
+	int err;
+
+	spin_lock_irqsave(&cctx->lock, flags);
+	/* check if we already have queried dsp for attributes */
+	if (cctx->valid_attributes) {
+		spin_unlock_irqrestore(&cctx->lock, flags);
+		goto done;
+	}
+	spin_unlock_irqrestore(&cctx->lock, flags);
+
+	err = fastrpc_get_info_from_dsp(fl, &dsp_attributes[0], FASTRPC_MAX_DSP_ATTRIBUTES);
+	if (err == DSP_UNSUPPORTED_API) {
+		dev_info(&cctx->rpdev->dev,
+			 "Warning: DSP capabilities not supported on domain: %d\n", domain);
+		return -EOPNOTSUPP;
+	} else if (err) {
+		dev_err(&cctx->rpdev->dev, "Error: dsp information is incorrect err: %d\n", err);
+		return err;
+	}
+
+	spin_lock_irqsave(&cctx->lock, flags);
+	memcpy(cctx->dsp_attributes, dsp_attributes, sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES);
+	cctx->valid_attributes = true;
+	spin_unlock_irqrestore(&cctx->lock, flags);
+done:
+	cap->capability = cctx->dsp_attributes[attribute_id];
+
+	return 0;
+}
+
+static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
+{
+	struct fastrpc_ioctl_capability cap = {0};
+	int err = 0;
+
+	if (copy_from_user(&cap, argp, sizeof(cap)))
+		return  -EFAULT;
+
+	cap.capability = 0;
+	if (cap.domain >= FASTRPC_DEV_MAX) {
+		dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
+			cap.domain, err);
+		return -ECHRNG;
+	}
+
+	/* Fastrpc Capablities does not support modem domain */
+	if (cap.domain == MDSP_DOMAIN_ID) {
+		dev_err(&fl->cctx->rpdev->dev, "Error: modem not supported %d\n", err);
+		return -ECHRNG;
+	}
+
+	if (cap.attribute_id >= FASTRPC_MAX_DSP_ATTRIBUTES) {
+		dev_err(&fl->cctx->rpdev->dev, "Error: invalid attribute: %d, err: %d\n",
+			cap.attribute_id, err);
+		return -EOVERFLOW;
+	}
+
+	err = fastrpc_get_info_from_kernel(&cap, fl);
+	if (err)
+		return err;
+
+	if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int fastrpc_req_munmap_impl(struct fastrpc_user *fl,
 				   struct fastrpc_req_munmap *req)
 {
@@ -1675,6 +1777,9 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 	case FASTRPC_IOCTL_MEM_UNMAP:
 		err = fastrpc_req_mem_unmap(fl, argp);
 		break;
+	case FASTRPC_IOCTL_GET_DSP_INFO:
+		err = fastrpc_get_dsp_info(fl, argp);
+		break;
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index 2308650e4a6e..f39edac20305 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -15,6 +15,7 @@
 #define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)
 #define FASTRPC_IOCTL_MEM_MAP		_IOWR('R', 10, struct fastrpc_mem_map)
 #define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
+#define FASTRPC_IOCTL_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
 
 /**
  * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
@@ -122,4 +123,11 @@ struct fastrpc_mem_unmap {
 	__s32 reserved[5];
 };
 
+struct fastrpc_ioctl_capability {
+	__u32 domain;
+	__u32 attribute_id;
+	__u32 capability;   /* dsp capability */
+	__u32 reserved[4];
+};
+
 #endif /* __QCOM_FASTRPC_H__ */
-- 
2.21.0

[PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

Add property to set DSP domain as non-secure.

ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
secured/unsecured.
non-secured Compute DSP would allow users to load unsigned process
and run hexagon instructions, but limiting access to secured hardware
within the DSP.

Based on this flag device nodes for secured and unsecured are created.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---

This patch has dependency this yaml conversion patch:
https://lore.kernel.org/lkml/20211208101508.24582-1-david@ixit.cz/T/

 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index f42ab208a7fc..f0df0a3bf69f 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -29,6 +29,11 @@ properties:
         - sdsp
         - cdsp
 
+  qcom,non-secure-domain:
+    type: boolean
+    description: >
+      Property to specify that dsp domain is non-secure.
+
   '#address-cells':
     const: 1
 
-- 
2.21.0

[PATCH v2 6/8] misc: fastrpc: add secure domain support

[Srinivas Kandagatla]

ADSP/MDSP/SDSP are by default secured, which means it can only be loaded
with a Signed process.
Where as CDSP can be either be secured/unsecured. non-secured Compute DSP
would allow users to load unsigned process and run hexagon instructions,
but blocking access to secured hardware within the DSP. Where as signed
process with secure CDSP would be allowed to access all the dsp resources.

This patch adds basic code to create device nodes as per device tree property.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 61 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 79fc59caacef..50f8e23b6b04 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -240,12 +240,15 @@ struct fastrpc_channel_ctx {
 	/* Flag if dsp attributes are cached */
 	bool valid_attributes;
 	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
+	struct fastrpc_device *secure_fdevice;
 	struct fastrpc_device *fdevice;
+	bool secure;
 };
 
 struct fastrpc_device {
 	struct fastrpc_channel_ctx *cctx;
 	struct miscdevice miscdev;
+	bool secure;
 };
 
 struct fastrpc_user {
@@ -1876,7 +1879,7 @@ static struct platform_driver fastrpc_cb_driver = {
 };
 
 static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
-				   const char *domain)
+				   bool is_secured, const char *domain)
 {
 	struct fastrpc_device *fdev;
 	int err;
@@ -1885,15 +1888,21 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
 	if (!fdev)
 		return -ENOMEM;
 
+	fdev->secure = is_secured;
 	fdev->cctx = cctx;
 	fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
 	fdev->miscdev.fops = &fastrpc_fops;
-	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
+	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s%s",
+					    domain, is_secured ? "-secure" : "");
 	err = misc_register(&fdev->miscdev);
-	if (err)
+	if (err) {
 		kfree(fdev);
-	else
-		cctx->fdevice = fdev;
+	} else {
+		if (is_secured)
+			cctx->secure_fdevice = fdev;
+		else
+			cctx->fdevice = fdev;
+	}
 
 	return err;
 }
@@ -1904,6 +1913,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	struct fastrpc_channel_ctx *data;
 	int i, err, domain_id = -1;
 	const char *domain;
+	bool secure_dsp = false;
 
 	err = of_property_read_string(rdev->of_node, "label", &domain);
 	if (err) {
@@ -1927,10 +1937,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (!data)
 		return -ENOMEM;
 
-	err = fastrpc_device_register(rdev, data, domains[domain_id]);
-	if (err) {
-		kfree(data);
-		return err;
+
+	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
+	data->secure = secure_dsp;
+
+	switch (domain_id) {
+	case ADSP_DOMAIN_ID:
+	case MDSP_DOMAIN_ID:
+	case SDSP_DOMAIN_ID:
+		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
+		if (err)
+			goto fdev_error;
+		break;
+	case CDSP_DOMAIN_ID:
+		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
+		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
+		if (err)
+			goto fdev_error;
+
+		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
+		if (err)
+			goto fdev_error;
+		break;
+	default:
+		err = -EINVAL;
+		goto fdev_error;
 	}
 
 	kref_init(&data->refcount);
@@ -1943,7 +1974,14 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	data->domain_id = domain_id;
 	data->rpdev = rpdev;
 
-	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
+	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
+	dev_info(rdev, "%s complete for %s with secure flag(%d) return: %d\n",
+			__func__, domains[domain_id], secure_dsp, err);
+	return err;
+
+fdev_error:
+	kfree(data);
+	return err;
 }
 
 static void fastrpc_notify_users(struct fastrpc_user *user)
@@ -1970,6 +2008,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	if (cctx->fdevice)
 		misc_deregister(&cctx->fdevice->miscdev);
 
+	if (cctx->secure_fdevice)
+		misc_deregister(&cctx->secure_fdevice->miscdev);
+
 	of_platform_depopulate(&rpdev->dev);
 
 	cctx->rpdev = NULL;
-- 
2.21.0

[PATCH v2 7/8] misc: fastrpc: check before loading process to the DSP

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

Reject session if DSP domain is secure, device node is non-secure and signed
PD is requested. Secure device node can access DSP without any restriction.

Unsigned PD offload is only allowed for the DSP domain that can support
unsigned offloading.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/misc/fastrpc.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 50f8e23b6b04..898c30a60902 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -243,6 +243,7 @@ struct fastrpc_channel_ctx {
 	struct fastrpc_device *secure_fdevice;
 	struct fastrpc_device *fdevice;
 	bool secure;
+	bool unsigned_support;
 };
 
 struct fastrpc_device {
@@ -263,6 +264,7 @@ struct fastrpc_user {
 
 	int tgid;
 	int pd;
+	bool is_secure_dev;
 	/* Lock for lists */
 	spinlock_t lock;
 	/* lock for allocations */
@@ -1049,6 +1051,24 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	return err;
 }
 
+static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_request)
+{
+	/* Check if the device node is non-secure and channel is secure*/
+	if (!fl->is_secure_dev && fl->cctx->secure) {
+		/*
+		 * Allow untrusted applications to offload only to Unsigned PD when
+		 * channel is configured as secure and block untrusted apps on channel
+		 * that does not support unsigned PD offload
+		 */
+		if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
+			dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD");
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static int fastrpc_init_create_process(struct fastrpc_user *fl,
 					char __user *argp)
 {
@@ -1068,6 +1088,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		u32 siglen;
 	} inbuf;
 	u32 sc;
+	bool unsigned_module = false;
 
 	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
 	if (!args)
@@ -1078,6 +1099,14 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
+	if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
+		unsigned_module = true;
+
+	if (is_session_rejected(fl, unsigned_module)) {
+		err = -ECONNREFUSED;
+		goto err;
+	}
+
 	if (init.filelen > INIT_FILELEN_MAX) {
 		err = -EINVAL;
 		goto err;
@@ -1277,6 +1306,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	INIT_LIST_HEAD(&fl->user);
 	fl->tgid = current->tgid;
 	fl->cctx = cctx;
+	fl->is_secure_dev = fdevice->secure;
 
 	fl->sctx = fastrpc_session_alloc(cctx);
 	if (!fl->sctx) {
@@ -1945,11 +1975,14 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	case ADSP_DOMAIN_ID:
 	case MDSP_DOMAIN_ID:
 	case SDSP_DOMAIN_ID:
+		/* Unsigned PD offloading is only supported on CDSP*/
+		data->unsigned_support = false;
 		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
 		if (err)
 			goto fdev_error;
 		break;
 	case CDSP_DOMAIN_ID:
+		data->unsigned_support = true;
 		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
 		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
 		if (err)
-- 
2.21.0

[PATCH v2 8/8] arm64: dts: qcom: add non-secure domain property to fastrpc nodes

[Srinivas Kandagatla]

From: Jeya R <jeyr@codeaurora.org>

FastRPC DSP domain would be set as secure if non-secure dsp property is not
added to the fastrpc DT node. Add this property to DT files of msm8916,
sdm845, sm8150, sm8250 and sm8350 so that nothing is broken after secure
domain patchset.

This patch is purely for backward compatibility reasons.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi  | 2 ++
 arch/arm64/boot/dts/qcom/sm8150.dtsi  | 3 +++
 arch/arm64/boot/dts/qcom/sm8250.dtsi  | 3 +++
 arch/arm64/boot/dts/qcom/sm8350.dtsi  | 3 +++
 5 files changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index c1c42f26b61e..137a479449d4 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1365,6 +1365,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,smd-channels = "fastrpcsmd-apps-dsp";
 					label = "adsp";
+					qcom,non-secure-domain;
 
 					#address-cells = <1>;
 					#size-cells = <0>;
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 526087586ba4..4aebfed4ec00 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -838,6 +838,7 @@
 				compatible = "qcom,fastrpc";
 				qcom,glink-channels = "fastrpcglink-apps-dsp";
 				label = "adsp";
+				qcom,non-secure-domain;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
@@ -888,6 +889,7 @@
 				compatible = "qcom,fastrpc";
 				qcom,glink-channels = "fastrpcglink-apps-dsp";
 				label = "cdsp";
+				qcom,non-secure-domain;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 81b4ff2cc4cd..9ac213bb96b7 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -1751,6 +1751,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "sdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -2994,6 +2995,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "cdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -3439,6 +3441,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "adsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index f0d342aa662d..06be221ad5b6 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2265,6 +2265,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "sdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -2330,6 +2331,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "cdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -4100,6 +4102,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "adsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index d134280e2939..80f753cbe91c 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -1278,6 +1278,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "sdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -1347,6 +1348,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "cdsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
@@ -1643,6 +1645,7 @@
 					compatible = "qcom,fastrpc";
 					qcom,glink-channels = "fastrpcglink-apps-dsp";
 					label = "adsp";
+					qcom,non-secure-domain;
 					#address-cells = <1>;
 					#size-cells = <0>;
 
-- 
2.21.0

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Stephan Gerhold]

On Thu, Dec 09, 2021 at 12:06:23PM +0000, Srinivas Kandagatla wrote:
> From: Jeya R <jeyr@codeaurora.org>
> 
> Add property to set DSP domain as non-secure.
> 
> ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
> secured/unsecured.

Wouldn't it be easier to avoid the negation and add a "qcom,secure-domain"
property instead? Given PATCH 8/8 ("arm64: dts: qcom: add non-secure
domain property to fastrpc nodes") it looks like you are intentionally
breaking DT compatibility here, but this patch does not justify why this
is necessary.

Thanks,
Stephan

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Srinivas Kandagatla]

On 13/12/2021 10:57, Stephan Gerhold wrote:
> On Thu, Dec 09, 2021 at 12:06:23PM +0000, Srinivas Kandagatla wrote:
>> From: Jeya R <jeyr@codeaurora.org>
>>
>> Add property to set DSP domain as non-secure.
>>
>> ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
>> secured/unsecured.
> 
> Wouldn't it be easier to avoid the negation and add a "qcom,secure-domain"
> property instead? Given PATCH 8/8 ("arm64: dts: qcom: add non-secure
> domain property to fastrpc nodes") it looks like you are intentionally
> breaking DT compatibility here, but this patch does not justify why this
> is necessary.

By default all ADSP/MDSP/SDSP are secured, so this property is only 
required for something that is not default. Only case that is 
configurable is the CDSP case where in by adding this flag we should be 
able to load an unsigned process to dsp using unsecured node.

Having said that, TBH When we first added the fastrpc patchset we did 
not take care of this security feature properly :-)

 From security point of view, its better to keep the default as secured 
rather than unsecured in DT too.

With this DTS patch older dts should continue to work.

--srini

> 
> Thanks,
> Stephan
> 

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Stephan Gerhold]

On Mon, Dec 13, 2021 at 12:35:40PM +0000, Srinivas Kandagatla wrote:
> On 13/12/2021 10:57, Stephan Gerhold wrote:
> > On Thu, Dec 09, 2021 at 12:06:23PM +0000, Srinivas Kandagatla wrote:
> > > From: Jeya R <jeyr@codeaurora.org>
> > > 
> > > Add property to set DSP domain as non-secure.
> > > 
> > > ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
> > > secured/unsecured.
> > 
> > Wouldn't it be easier to avoid the negation and add a "qcom,secure-domain"
> > property instead? Given PATCH 8/8 ("arm64: dts: qcom: add non-secure
> > domain property to fastrpc nodes") it looks like you are intentionally
> > breaking DT compatibility here, but this patch does not justify why this
> > is necessary.
> 
> By default all ADSP/MDSP/SDSP are secured, so this property is only required
> for something that is not default. Only case that is configurable is the
> CDSP case where in by adding this flag we should be able to load an unsigned
> process to dsp using unsecured node.
> 
> Having said that, TBH When we first added the fastrpc patchset we did not
> take care of this security feature properly :-)
> 
> From security point of view, its better to keep the default as secured
> rather than unsecured in DT too.
> 
> With this DTS patch older dts should continue to work.
> 

Is this a "default" on newer platforms only? Why do the existing
platforms not use the "secure" setup then? Or is this perhaps firmware
version/configuration specific?

Basically I'm confused because you say that the "default" is the secured
setup, but DT patch (8/8) suggests that non-secure is the default on
pretty much all currently supported platforms (msm8916, sdm845, sm8150,
sm8250, sm8350). :)

Thanks,
Stephan

Re: [PATCH v2 8/8] arm64: dts: qcom: add non-secure domain property to fastrpc nodes

[Bjorn Andersson]

On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:

> From: Jeya R <jeyr@codeaurora.org>
> 
> FastRPC DSP domain would be set as secure if non-secure dsp property is not
> added to the fastrpc DT node. Add this property to DT files of msm8916,
> sdm845, sm8150, sm8250 and sm8350 so that nothing is broken after secure
> domain patchset.
> 
> This patch is purely for backward compatibility reasons.
> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 +
>  arch/arm64/boot/dts/qcom/sdm845.dtsi  | 2 ++
>  arch/arm64/boot/dts/qcom/sm8150.dtsi  | 3 +++
>  arch/arm64/boot/dts/qcom/sm8250.dtsi  | 3 +++
>  arch/arm64/boot/dts/qcom/sm8350.dtsi  | 3 +++
>  5 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index c1c42f26b61e..137a479449d4 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -1365,6 +1365,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,smd-channels = "fastrpcsmd-apps-dsp";
>  					label = "adsp";
> +					qcom,non-secure-domain;

I was under the impression that the support for loading unsigned fastrpc
applications was introduced in SM8150 or SM8250, can you confirm that
this has been possible all along?

Regards,
Bjorn

>  
>  					#address-cells = <1>;
>  					#size-cells = <0>;
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 526087586ba4..4aebfed4ec00 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -838,6 +838,7 @@
>  				compatible = "qcom,fastrpc";
>  				qcom,glink-channels = "fastrpcglink-apps-dsp";
>  				label = "adsp";
> +				qcom,non-secure-domain;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> @@ -888,6 +889,7 @@
>  				compatible = "qcom,fastrpc";
>  				qcom,glink-channels = "fastrpcglink-apps-dsp";
>  				label = "cdsp";
> +				qcom,non-secure-domain;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index 81b4ff2cc4cd..9ac213bb96b7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -1751,6 +1751,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "sdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -2994,6 +2995,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "cdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -3439,6 +3441,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "adsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index f0d342aa662d..06be221ad5b6 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2265,6 +2265,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "sdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -2330,6 +2331,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "cdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -4100,6 +4102,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "adsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> index d134280e2939..80f753cbe91c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> @@ -1278,6 +1278,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "sdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -1347,6 +1348,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "cdsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> @@ -1643,6 +1645,7 @@
>  					compatible = "qcom,fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "adsp";
> +					qcom,non-secure-domain;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  
> -- 
> 2.21.0
> 

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Bjorn Andersson]

On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:

> From: Jeya R <jeyr@codeaurora.org>
> 
> Add property to set DSP domain as non-secure.
> 
> ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
> secured/unsecured.
> non-secured Compute DSP would allow users to load unsigned process
> and run hexagon instructions, but limiting access to secured hardware
> within the DSP.
> 
> Based on this flag device nodes for secured and unsecured are created.
> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> 
> This patch has dependency this yaml conversion patch:
> https://lore.kernel.org/lkml/20211208101508.24582-1-david@ixit.cz/T/
> 
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> index f42ab208a7fc..f0df0a3bf69f 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> @@ -29,6 +29,11 @@ properties:
>          - sdsp
>          - cdsp
>  
> +  qcom,non-secure-domain:
> +    type: boolean
> +    description: >
> +      Property to specify that dsp domain is non-secure.

"non-secure" feels vague, how about expressing it as "Specifies that the
domains of this DSP instance may run unsigned programs."

Perhaps even go so far to name the property
qcom,allow-unsigned-programs? (Or some other word for "program"?)

Regards,
Bjorn

> +
>    '#address-cells':
>      const: 1
>  
> -- 
> 2.21.0
> 

Re: [PATCH v2 8/8] arm64: dts: qcom: add non-secure domain property to fastrpc nodes

[Srinivas Kandagatla]

On 13/12/2021 15:36, Bjorn Andersson wrote:
> On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> 
>> From: Jeya R <jeyr@codeaurora.org>
>>
>> FastRPC DSP domain would be set as secure if non-secure dsp property is not
>> added to the fastrpc DT node. Add this property to DT files of msm8916,
>> sdm845, sm8150, sm8250 and sm8350 so that nothing is broken after secure
>> domain patchset.
>>
>> This patch is purely for backward compatibility reasons.
>>
>> Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 +
>>   arch/arm64/boot/dts/qcom/sdm845.dtsi  | 2 ++
>>   arch/arm64/boot/dts/qcom/sm8150.dtsi  | 3 +++
>>   arch/arm64/boot/dts/qcom/sm8250.dtsi  | 3 +++
>>   arch/arm64/boot/dts/qcom/sm8350.dtsi  | 3 +++
>>   5 files changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> index c1c42f26b61e..137a479449d4 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> @@ -1365,6 +1365,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,smd-channels = "fastrpcsmd-apps-dsp";
>>   					label = "adsp";
>> +					qcom,non-secure-domain;
> 
> I was under the impression that the support for loading unsigned fastrpc
> applications was introduced in SM8150 or SM8250, can you confirm that
> this has been possible all along?


Ekansh did confirm that this was introduced from sm8150.


--srini
> 
> Regards,
> Bjorn
> 
>>   
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 526087586ba4..4aebfed4ec00 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -838,6 +838,7 @@
>>   				compatible = "qcom,fastrpc";
>>   				qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   				label = "adsp";
>> +				qcom,non-secure-domain;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>> @@ -888,6 +889,7 @@
>>   				compatible = "qcom,fastrpc";
>>   				qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   				label = "cdsp";
>> +				qcom,non-secure-domain;
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>>   
>> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> index 81b4ff2cc4cd..9ac213bb96b7 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
>> @@ -1751,6 +1751,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "sdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -2994,6 +2995,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "cdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -3439,6 +3441,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "adsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> index f0d342aa662d..06be221ad5b6 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>> @@ -2265,6 +2265,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "sdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -2330,6 +2331,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "cdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -4100,6 +4102,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "adsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> index d134280e2939..80f753cbe91c 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> @@ -1278,6 +1278,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "sdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -1347,6 +1348,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "cdsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> @@ -1643,6 +1645,7 @@
>>   					compatible = "qcom,fastrpc";
>>   					qcom,glink-channels = "fastrpcglink-apps-dsp";
>>   					label = "adsp";
>> +					qcom,non-secure-domain;
>>   					#address-cells = <1>;
>>   					#size-cells = <0>;
>>   
>> -- 
>> 2.21.0
>>

Re: [PATCH v2 6/8] misc: fastrpc: add secure domain support

[Bjorn Andersson]

On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:

> ADSP/MDSP/SDSP are by default secured, which means it can only be loaded
> with a Signed process.
> Where as CDSP can be either be secured/unsecured. non-secured Compute DSP
> would allow users to load unsigned process and run hexagon instructions,
> but blocking access to secured hardware within the DSP. Where as signed
> process with secure CDSP would be allowed to access all the dsp resources.
> 
> This patch adds basic code to create device nodes as per device tree property.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/misc/fastrpc.c | 61 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 79fc59caacef..50f8e23b6b04 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -240,12 +240,15 @@ struct fastrpc_channel_ctx {
>  	/* Flag if dsp attributes are cached */
>  	bool valid_attributes;
>  	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
> +	struct fastrpc_device *secure_fdevice;
>  	struct fastrpc_device *fdevice;
> +	bool secure;
>  };
>  
>  struct fastrpc_device {
>  	struct fastrpc_channel_ctx *cctx;
>  	struct miscdevice miscdev;
> +	bool secure;
>  };
>  
>  struct fastrpc_user {
> @@ -1876,7 +1879,7 @@ static struct platform_driver fastrpc_cb_driver = {
>  };
>  
>  static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
> -				   const char *domain)
> +				   bool is_secured, const char *domain)
>  {
>  	struct fastrpc_device *fdev;
>  	int err;
> @@ -1885,15 +1888,21 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>  	if (!fdev)
>  		return -ENOMEM;
>  
> +	fdev->secure = is_secured;
>  	fdev->cctx = cctx;
>  	fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	fdev->miscdev.fops = &fastrpc_fops;
> -	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
> +	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s%s",
> +					    domain, is_secured ? "-secure" : "");

Will this not result in existing userspace using the wrong misc device?

>  	err = misc_register(&fdev->miscdev);
> -	if (err)
> +	if (err) {
>  		kfree(fdev);
> -	else
> -		cctx->fdevice = fdev;
> +	} else {
> +		if (is_secured)
> +			cctx->secure_fdevice = fdev;
> +		else
> +			cctx->fdevice = fdev;
> +	}
>  
>  	return err;
>  }
> @@ -1904,6 +1913,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	struct fastrpc_channel_ctx *data;
>  	int i, err, domain_id = -1;
>  	const char *domain;
> +	bool secure_dsp = false;

Afaict this is only every accessed after first being written. So no need
to initialize it.

>  
>  	err = of_property_read_string(rdev->of_node, "label", &domain);
>  	if (err) {
> @@ -1927,10 +1937,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	err = fastrpc_device_register(rdev, data, domains[domain_id]);
> -	if (err) {
> -		kfree(data);
> -		return err;
> +
> +	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
> +	data->secure = secure_dsp;
> +
> +	switch (domain_id) {
> +	case ADSP_DOMAIN_ID:
> +	case MDSP_DOMAIN_ID:
> +	case SDSP_DOMAIN_ID:
> +		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +		break;
> +	case CDSP_DOMAIN_ID:
> +		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
> +		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +
> +		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
> +		if (err)
> +			goto fdev_error;
> +		break;
> +	default:
> +		err = -EINVAL;
> +		goto fdev_error;
>  	}
>  
>  	kref_init(&data->refcount);
> @@ -1943,7 +1974,14 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	data->domain_id = domain_id;
>  	data->rpdev = rpdev;
>  
> -	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	dev_info(rdev, "%s complete for %s with secure flag(%d) return: %d\n",
> +			__func__, domains[domain_id], secure_dsp, err);

I would prefer that we don't spam the kernel log with such useful
information, in particular since it will happen every time we start or
restart a remoteproc with fastrpc. So dev_dbg perhaps?

> +	return err;

I think that in the event that of_platform_populate() actually failed,
you will return an error here, fastrpc_rpmsg_remove() won't be called,
so you won't release the misc device or release &data->refcount. This
issue exists in the code today though...

Regards,
Bjorn

> +
> +fdev_error:
> +	kfree(data);
> +	return err;
>  }
>  
>  static void fastrpc_notify_users(struct fastrpc_user *user)
> @@ -1970,6 +2008,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>  	if (cctx->fdevice)
>  		misc_deregister(&cctx->fdevice->miscdev);
>  
> +	if (cctx->secure_fdevice)
> +		misc_deregister(&cctx->secure_fdevice->miscdev);
> +
>  	of_platform_depopulate(&rpdev->dev);
>  
>  	cctx->rpdev = NULL;
> -- 
> 2.21.0
> 

Re: [PATCH v2 6/8] misc: fastrpc: add secure domain support

[Srinivas Kandagatla]

On 13/12/2021 18:37, Bjorn Andersson wrote:
> On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> 
>> ADSP/MDSP/SDSP are by default secured, which means it can only be loaded
>> with a Signed process.
>> Where as CDSP can be either be secured/unsecured. non-secured Compute DSP
>> would allow users to load unsigned process and run hexagon instructions,
>> but blocking access to secured hardware within the DSP. Where as signed
>> process with secure CDSP would be allowed to access all the dsp resources.
>>
>> This patch adds basic code to create device nodes as per device tree property.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/misc/fastrpc.c | 61 +++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 51 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 79fc59caacef..50f8e23b6b04 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -240,12 +240,15 @@ struct fastrpc_channel_ctx {
>>   	/* Flag if dsp attributes are cached */
>>   	bool valid_attributes;
>>   	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>> +	struct fastrpc_device *secure_fdevice;
>>   	struct fastrpc_device *fdevice;
>> +	bool secure;
>>   };
>>   
>>   struct fastrpc_device {
>>   	struct fastrpc_channel_ctx *cctx;
>>   	struct miscdevice miscdev;
>> +	bool secure;
>>   };
>>   
>>   struct fastrpc_user {
>> @@ -1876,7 +1879,7 @@ static struct platform_driver fastrpc_cb_driver = {
>>   };
>>   
>>   static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
>> -				   const char *domain)
>> +				   bool is_secured, const char *domain)
>>   {
>>   	struct fastrpc_device *fdev;
>>   	int err;
>> @@ -1885,15 +1888,21 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>   	if (!fdev)
>>   		return -ENOMEM;
>>   
>> +	fdev->secure = is_secured;
>>   	fdev->cctx = cctx;
>>   	fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
>>   	fdev->miscdev.fops = &fastrpc_fops;
>> -	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
>> +	fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s%s",
>> +					    domain, is_secured ? "-secure" : "");
> 
> Will this not result in existing userspace using the wrong misc device?

No, we will end up with

fastrpc-cdsp
fastrpc-cdsp-secure

based on the qcom,non-secure-domain DT property

so we still have the same old name, as long as the dt-property is 
correctly set.

> 
>>   	err = misc_register(&fdev->miscdev);
>> -	if (err)
>> +	if (err) {
>>   		kfree(fdev);
>> -	else
>> -		cctx->fdevice = fdev;
>> +	} else {
>> +		if (is_secured)
>> +			cctx->secure_fdevice = fdev;
>> +		else
>> +			cctx->fdevice = fdev;
>> +	}
>>   
>>   	return err;
>>   }
>> @@ -1904,6 +1913,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   	struct fastrpc_channel_ctx *data;
>>   	int i, err, domain_id = -1;
>>   	const char *domain;
>> +	bool secure_dsp = false;
> 

> Afaict this is only every accessed after first being written. So no need
> to initialize it.

that's true, I can remove this in next spin.

> 
>>   
>>   	err = of_property_read_string(rdev->of_node, "label", &domain);
>>   	if (err) {
>> @@ -1927,10 +1937,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   	if (!data)
>>   		return -ENOMEM;
>>   
>> -	err = fastrpc_device_register(rdev, data, domains[domain_id]);
>> -	if (err) {
>> -		kfree(data);
>> -		return err;
>> +
>> +	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>> +	data->secure = secure_dsp;
>> +
>> +	switch (domain_id) {
>> +	case ADSP_DOMAIN_ID:
>> +	case MDSP_DOMAIN_ID:
>> +	case SDSP_DOMAIN_ID:
>> +		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>> +		if (err)
>> +			goto fdev_error;
>> +		break;
>> +	case CDSP_DOMAIN_ID:
>> +		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
>> +		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>> +		if (err)
>> +			goto fdev_error;
>> +
>> +		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>> +		if (err)
>> +			goto fdev_error;
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		goto fdev_error;
>>   	}
>>   
>>   	kref_init(&data->refcount);
>> @@ -1943,7 +1974,14 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   	data->domain_id = domain_id;
>>   	data->rpdev = rpdev;
>>   
>> -	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
>> +	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
>> +	dev_info(rdev, "%s complete for %s with secure flag(%d) return: %d\n",
>> +			__func__, domains[domain_id], secure_dsp, err);
> 
> I would prefer that we don't spam the kernel log with such useful
> information, in particular since it will happen every time we start or
> restart a remoteproc with fastrpc. So dev_dbg perhaps?

agree, will change.
> 
>> +	return err;
> 
> I think that in the event that of_platform_populate() actually failed,
> you will return an error here, fastrpc_rpmsg_remove() won't be called,
> so you won't release the misc device or release &data->refcount. This
> issue exists in the code today though...

Thanks, that is a good point I will see if I can fix that in next spin.

--srini

> 
> Regards,
> Bjorn
> 
>> +
>> +fdev_error:
>> +	kfree(data);
>> +	return err;
>>   }
>>   
>>   static void fastrpc_notify_users(struct fastrpc_user *user)
>> @@ -1970,6 +2008,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>   	if (cctx->fdevice)
>>   		misc_deregister(&cctx->fdevice->miscdev);
>>   
>> +	if (cctx->secure_fdevice)
>> +		misc_deregister(&cctx->secure_fdevice->miscdev);
>> +
>>   	of_platform_depopulate(&rpdev->dev);
>>   
>>   	cctx->rpdev = NULL;
>> -- 
>> 2.21.0
>>

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Srinivas Kandagatla]

On 13/12/2021 15:46, Bjorn Andersson wrote:
> On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> 
>> From: Jeya R <jeyr@codeaurora.org>
>>
>> Add property to set DSP domain as non-secure.
>>
>> ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
>> secured/unsecured.
>> non-secured Compute DSP would allow users to load unsigned process
>> and run hexagon instructions, but limiting access to secured hardware
>> within the DSP.
>>
>> Based on this flag device nodes for secured and unsecured are created.
>>
>> Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>
>> This patch has dependency this yaml conversion patch:
>> https://lore.kernel.org/lkml/20211208101508.24582-1-david@ixit.cz/T/
>>
>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> index f42ab208a7fc..f0df0a3bf69f 100644
>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> @@ -29,6 +29,11 @@ properties:
>>           - sdsp
>>           - cdsp
>>   
>> +  qcom,non-secure-domain:
>> +    type: boolean
>> +    description: >
>> +      Property to specify that dsp domain is non-secure.

> 
> "non-secure" feels vague, how about expressing it as "Specifies that the
> domains of this DSP instance may run unsigned programs."

TBH I dont mind either of this, but looking at some existing bindings I 
see similar pattern of properties like.. "st,non-secure-otp"

> 
> Perhaps even go so far to name the property
> qcom,allow-unsigned-programs? (Or some other word for "program"?)

Do you think adding more details in the description would help?

--srini
> 
> Regards,
> Bjorn
> 
>> +
>>     '#address-cells':
>>       const: 1
>>   
>> -- 
>> 2.21.0
>>

Re: [PATCH v2 5/8] dt-bindings: misc: add property to support non-secure DSP

[Srinivas Kandagatla]

On 13/12/2021 13:19, Stephan Gerhold wrote:
> On Mon, Dec 13, 2021 at 12:35:40PM +0000, Srinivas Kandagatla wrote:
>> On 13/12/2021 10:57, Stephan Gerhold wrote:
>>> On Thu, Dec 09, 2021 at 12:06:23PM +0000, Srinivas Kandagatla wrote:
>>>> From: Jeya R <jeyr@codeaurora.org>
>>>>
>>>> Add property to set DSP domain as non-secure.
>>>>
>>>> ADSP/MDSP/SDSP are by default secured, where as CDSP can be either be
>>>> secured/unsecured.
>>>
>>> Wouldn't it be easier to avoid the negation and add a "qcom,secure-domain"
>>> property instead? Given PATCH 8/8 ("arm64: dts: qcom: add non-secure
>>> domain property to fastrpc nodes") it looks like you are intentionally
>>> breaking DT compatibility here, but this patch does not justify why this
>>> is necessary.
>>
>> By default all ADSP/MDSP/SDSP are secured, so this property is only required
>> for something that is not default. Only case that is configurable is the
>> CDSP case where in by adding this flag we should be able to load an unsigned
>> process to dsp using unsecured node.
>>
>> Having said that, TBH When we first added the fastrpc patchset we did not
>> take care of this security feature properly :-)
>>
>>  From security point of view, its better to keep the default as secured
>> rather than unsecured in DT too.
>>
>> With this DTS patch older dts should continue to work.
>>
> 
> Is this a "default" on newer platforms only? Why do the existing
> platforms not use the "secure" setup then? Or is this perhaps firmware
> version/configuration specific?

So I did bit of digging at old msm kernels spoke to Qualcomm on this. 
This feature was added in Dec 2018 and after. So ADSP/MDSP/SDSP are by 
secured by default for SoCs SDM845 and after.

However when we upstreamed the first fastrpc driver (end of 2018 early 
2019) we did not take this new feature into consideration and we now 
ended up with most recent SoCs accessing the only available non secured 
device node.


This new property serves three purposes

1. supporting the older SoCs (msm8916 msm8996) that did not have this 
secure node,

2. Allow CDSP configuration of secured/unsecured.

3. keep the new SoCs working (sdm845, sm8150, sm8250, sm8350) with 
existing upstream driver. (This is purely for not breaking existing 
applications).

We could do the right thing here by making only msm8916 non-secured and 
let all the new SoCs like sdm845 and later be by default secured on 
ADSP/MDSP/SDSP and only configure CDSP.

> 
> Basically I'm confused because you say that the "default" is the secured
> setup, but DT patch (8/8) suggests that non-secure is the default on
> pretty much all currently supported platforms (msm8916, sdm845, sm8150,
> sm8250, sm8350). :)

I agree there is a bit of confusion, I hope my reply clears this.

--srini

> 
> Thanks,
> Stephan
>