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/[email protected]/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
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 <[email protected]>
---
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
From: Jeya R <[email protected]>
Add fastrpc remote process attributes. These attributes are passed as
part of process create ioctl request.
Signed-off-by: Jeya R <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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
From: Jeya R <[email protected]>
Add support for IOCTL requests to map and unmap on DSP based on map
flags.
Signed-off-by: Jeya R <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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
From: Jeya R <[email protected]>
Add support to get DSP capabilities. The capability information is cached
on driver.
Signed-off-by: Jeya R <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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
From: Jeya R <[email protected]>
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 <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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
From: Jeya R <[email protected]>
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 <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
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
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 <[email protected]>
---
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
From: Jeya R <[email protected]>
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 <[email protected]>
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
This patch has dependency this yaml conversion patch:
https://lore.kernel.org/lkml/[email protected]/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
On Thu, Dec 09, 2021 at 12:06:23PM +0000, Srinivas Kandagatla wrote:
> From: Jeya R <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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
>
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 <[email protected]>
> > >
> > > 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
On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> From: Jeya R <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> ---
> 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
>
On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> From: Jeya R <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> ---
>
> This patch has dependency this yaml conversion patch:
> https://lore.kernel.org/lkml/[email protected]/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
>
On 13/12/2021 15:36, Bjorn Andersson wrote:
> On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
>
>> From: Jeya R <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Srinivas Kandagatla <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>
On 13/12/2021 15:46, Bjorn Andersson wrote:
> On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
>
>> From: Jeya R <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Srinivas Kandagatla <[email protected]>
>> ---
>>
>> This patch has dependency this yaml conversion patch:
>> https://lore.kernel.org/lkml/[email protected]/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
>>
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 <[email protected]>
>>>>
>>>> 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
>