: Add hypervisor call support to enable mss rproc on msm8996
Subsystems (4)
cover : Add hypervisor call support to enable mss rproc on msm8996
This Patch series consist two patches.
1- Add hypervisor call support to enable stage two translation applicable for armv8 and above. 2- Add and enable mss remoteproc support for msm8996
This patchseries is dependednt on https://patchwork.kernel.org/patch/9492177/
Avaneesh Kumar Dwivedi (2):
soc: qcom: Add hypervisor stage two translation request support.
remoteproc: qcom: Add mss rproc support for msm8996.
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 +- drivers/firmware/qcom_scm-64.c | 16 ++ drivers/firmware/qcom_scm.c | 13 + drivers/firmware/qcom_scm.h | 10 + drivers/remoteproc/qcom_q6v5_pil.c | 275 ++++++++++++++++++--- drivers/soc/qcom/Kconfig | 8 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/secure_buffer.c | 229 +++++++++++++++++ include/linux/qcom_scm.h | 1 + include/soc/qcom/secure_buffer.h | 51 ++++ 10 files changed, 574 insertions(+), 32 deletions(-) create mode 100644 drivers/soc/qcom/secure_buffer.c create mode 100644 include/soc/qcom/secure_buffer.h
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
RFC 1/2 soc: qcom: Add hypervisor stage two translation request support.
This patch add hypervisor support for mss bring up on msm8996. MSS rproc driver make hypervisor request to add certain region in IPA table owned by hepervisor and assign access permission to modem. These regions are used to load MBA, MDT, FW into DDR.
--- drivers/firmware/qcom_scm-64.c | 16 +++ drivers/firmware/qcom_scm.c | 13 +++ drivers/firmware/qcom_scm.h | 10 ++ drivers/remoteproc/qcom_q6v5_pil.c | 96 +++++++++++++++- drivers/soc/qcom/Kconfig | 8 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/secure_buffer.c | 229 +++++++++++++++++++++++++++++++++++++ include/linux/qcom_scm.h | 1 + include/soc/qcom/secure_buffer.h | 51 +++++++++ 9 files changed, 419 insertions(+), 6 deletions(-) create mode 100644 drivers/soc/qcom/secure_buffer.c create mode 100644 include/soc/qcom/secure_buffer.h
drivers/firmware/qcom_scm-64.c +16 -0
@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)return ret ? : res.a1; } + +int __qcom_scm_assign_mem (struct device *dev,void *data) +{ +int ret; +struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data; +struct arm_smccc_res res; + + desc->arginfo =QCOM_SCM_ARGS (7 ,SCM_RO ,SCM_VAL ,SCM_RO , +SCM_VAL ,SCM_RO ,SCM_VAL ,SCM_VAL ); + + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + desc, &res); + +return ret ? : res.a1; +}
drivers/firmware/qcom_scm.c +13 -0
@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown); +/** +* qcom_scm_assign_mem() - +* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc) +{ + +return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {
drivers/firmware/qcom_scm.h +10 -0
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_init_image (struct device *dev,u32 peripheral,dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,extern int __qcom_scm_pas_auth_and_reset (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_shutdown (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_mss_reset (struct device *dev,bool reset); +extern int __qcom_scm_assign_mem (struct device *dev,void *desc);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12
@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err) return -EINVAL; } +enum scm_arg_types { +SCM_VAL , +SCM_RO , +SCM_RW , +SCM_BUFVAL , +}; +#endif
drivers/remoteproc/qcom_q6v5_pil.c +90 -6
197 lines not on this page · raw message
drivers/soc/qcom/Kconfig +8 -0
@@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL help Client driver for the WCNSS_CTRL SMD channel, used to download nv firmware to a newly booted WCNSS chip. + +config QCOM_SECURE_BUFFER + bool "Helper functions for securing buffers through TZ" + help + Say 'Y' here for targets that need to call into TZ to secure + memory buffers. This ensures that only the correct clients can + use this memory and no unauthorized access is made to the + buffer
drivers/soc/qcom/Makefile +1 -0
@@ -6,4 +6,5 @@ obj-$(CONFIG_QCOM_SMEM) += smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o +obj-$(CONFIG_QCOM_SECURE_BUFFER) += secure_buffer.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
drivers/soc/qcom/secure_buffer.c new file +229 -0
229 lines not on this page · raw message
include/linux/qcom_scm.h +1 -0
@@ -36,6 +36,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);extern int qcom_scm_pas_auth_and_reset (u32 peripheral);extern int qcom_scm_pas_shutdown (u32 peripheral); +extern int qcom_scm_assign_mem (void *desc);#define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0#define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
include/soc/qcom/secure_buffer.h new file +51 -0
51 lines not on this page · raw message
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.
Quoted 3 lines · Avaneesh Kumar Dwivedi
This patch add hypervisor support for mss bring up on msm8996. MSS rproc driver make hypervisor request to add certain region in IPA table owned by hepervisor and assign access permission
Please drop the use of IPA here. There's an IPA acronym for the IP accelerator and that is confused with Intermediate Physical Address. Instead, say something like "in the stage 2 page tables of the SMMU". Also hypervisor is misspelled here.
Quoted 8 lines
to modem. These regions are used to load MBA, MDT, FW into DDR.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/firmware/qcom_scm-64.c | 16 +++ drivers/firmware/qcom_scm.c | 13 +++ drivers/firmware/qcom_scm.h | 10 ++ drivers/remoteproc/qcom_q6v5_pil.c | 96 +++++++++++++++-
Please split the remoteproc code off from this patch.
Quoted 14 lines
drivers/soc/qcom/Kconfig | 8 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/secure_buffer.c | 229 +++++++++++++++++++++++++++++++++++++ include/linux/qcom_scm.h | 1 + include/soc/qcom/secure_buffer.h | 51 +++++++++ 9 files changed, 419 insertions(+), 6 deletions(-) create mode 100644 drivers/soc/qcom/secure_buffer.c create mode 100644 include/soc/qcom/secure_buffer.hdrivers/firmware/qcom_scm-64.c +0
-0
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 365
Quoted 8 lines
drivers/firmware/qcom_scm-64.c +5
-0@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)return ret ? : res.a1; } + +int __qcom_scm_assign_mem (struct device *dev,void *data) +{ +int ret; +struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;
Useless cast from void.
Quoted 16 lines
drivers/firmware/qcom_scm-64.c +11
-0+struct arm_smccc_res res; + + desc->arginfo =QCOM_SCM_ARGS (7 ,SCM_RO ,SCM_VAL ,SCM_RO , +SCM_VAL ,SCM_RO ,SCM_VAL ,SCM_VAL ); + + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + desc, &res); + +return ret ? : res.a1; +}drivers/firmware/qcom_scm.c +0
-0
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 296
Quoted 5 lines
drivers/firmware/qcom_scm.c +2
-0@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown); +/** +* qcom_scm_assign_mem() -
Some words on what the function does?
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 303
Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc) +{ +
Nitpick: Drop the extra newline
Quoted 29 lines
drivers/firmware/qcom_scm.c +4
-0+return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +3
-0@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_init_image (struct device *dev,u32 peripheral,dma_addr_t metadata_phys);@@ -55,6 +57,7 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,extern int __qcom_scm_pas_auth_and_reset (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_shutdown (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_mss_reset (struct device *dev,bool reset); +extern int __qcom_scm_assign_mem (struct device *dev,void *desc);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12
drivers/firmware/qcom_scm.h static inline int qcom_scm_remap_error(int err) · line 94
Quoted 9 lines
drivers/firmware/qcom_scm.h +6
-0@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)return -EINVAL ; } +enum scm_arg_types { +SCM_VAL , +SCM_RO , +SCM_RW , +SCM_BUFVAL , +};
We already have this enum? It's just prepended with QCOM_ instead.
Quoted 15 lines
drivers/firmware/qcom_scm.h +1
-0+#endif drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -31,6 +31,7 @@#include <linux/soc/qcom/smem.h> #include <linux/soc/qcom/smem_state.h> #include <linux/of_device.h> +#include <soc/qcom/secure_buffer.h> #include "remoteproc_internal.h" #include "qcom_mdt_loader.h"
drivers/remoteproc/qcom_q6v5_pil.c struct qcom_mss_reg_res { · line 112
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0@@ -105,12 +106,19 @@ struct qcom_mss_reg_res {int uA; }; +struct src_dest_vmid { +int *srcVM; +int *destVM; +int *destVMperm;
Why not have an array of these structures instead of a structure with three arrays inside? Can you map one source VM to multiple destination VMs?
Quoted 42 lines
drivers/remoteproc/qcom_q6v5_pil.c +15
-0+}; +struct rproc_hexagon_res {const char *hexagon_mba_image;struct qcom_mss_reg_res proxy_supply[4 ];struct qcom_mss_reg_res active_supply[2 ];char **proxy_clk_names;char **active_clk_names; +int version; };struct q6v5 {@@ -127,6 +135,8 @@ struct q6v5 {struct reset_control *mss_restart; +struct src_dest_vmid vmid_details; +struct qcom_smem_state *state;unsigned stop_bit;@@ -152,6 +162,13 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +int version; +}; + +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , }; static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,@@ -273,13 +290,46 @@ static void q6v5_clk_disable(struct device *dev,clk_disable_unprepare (clks[i]); } +int hyp_mem_access (struct q6v5 *qproc,phys_addr_t addr,size_t size) +{ +int src_count =0 ;
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 305
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+int dest_count =0 ; +int ret; +int i; + +if (qproc->version !=MSS_MSM8996 ) +return 0 ; + +for (i =0 ; i <2 ; i++) { +if (qproc->vmid_details.srcVM[i] !=0 ) + src_count++;
Bad tabbing here.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 307
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0+if (qproc->vmid_details.destVM[i] !=0 ) + dest_count++;
And here.
When is it ever == 0? The hardcoded 2 in the for loop is also suspect.
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 308
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+ }
Add newline
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 312
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+ ret =hyp_assign_phys (qproc->dev, addr, size, + qproc->vmid_details.srcVM, + src_count, qproc->vmid_details.destVM, + qproc->vmid_details.destVMperm, dest_count);
At the least this API could take a vmid_details structure instead of all these arguments:
struct vm_perms {
u32 vm;
u32 perm;
};
struct vmid_details {
u32 *src;
size_t src_count;
struct vm_perms *dest;
size_t dest_count;
};Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+if (ret) +dev_err (qproc->dev, +"%s: failed for %pa address of size %zx\n" , + __func__, &addr, size); +return ret; +} +
[...]
drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 80
Quoted 10 lines
drivers/soc/qcom/Kconfig +2
-0@@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL help Client driver for the WCNSS_CTRL SMD channel, used to download nv firmware to a newly booted WCNSS chip. + +config QCOM_SECURE_BUFFER
Please no. Just fold this into the qcom_scm.c code and drop the new config and new file.
Quoted 17 lines
drivers/soc/qcom/Kconfig +6
-0+ bool "Helper functions for securing buffers through TZ" + help + Say 'Y' here for targets that need to call into TZ to secure + memory buffers. This ensures that only the correct clients can + use this memory and no unauthorized access is made to the + bufferdrivers/soc/qcom/secure_buffer.c new file +5
-0@@ -0,0 +1,229 @@ +/* +* Copyright (c) 2011-2017, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and
drivers/soc/qcom/secure_buffer.c line 15
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ + +#include <linux/highmem.h>
Used?
drivers/soc/qcom/secure_buffer.c line 17
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+#include <linux/kernel.h> +#include <linux/kref.h>
Used?
drivers/soc/qcom/secure_buffer.c line 26
Quoted 9 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+#include <linux/mutex.h> +#include <linux/scatterlist.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> +#include <linux/qcom_scm.h> +#include <linux/dma-mapping.h> +#include <soc/qcom/secure_buffer.h> + +DEFINE_MUTEX (secure_buffer_mutex);
static?
drivers/soc/qcom/secure_buffer.c line 31
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+ +struct scm_desc { +u32 arginfo; +u64 args[10 ]; +};
This would be in qcom_scm-64.c already.
drivers/soc/qcom/secure_buffer.c line 35
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+ +struct mem_prot_info { +phys_addr_t addr; +u64 size;
Both should be __le64 presumably.
drivers/soc/qcom/secure_buffer.c line 46
Quoted 11 lines
drivers/soc/qcom/secure_buffer.c new file +11
-0+}; + +struct info_list { +struct mem_prot_info *list_head; +u64 list_size; +}; + +struct dest_vm_and_perm_info { +u32 vm; +u32 perm; +u32 *ctx;
Why is this a pointer? Is this structure put into memory and passed to the firmware? We should use the appropriate __le32 and __le64 types then.
drivers/soc/qcom/secure_buffer.c line 53
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+u32 ctx_size; +}; + +struct dest_info_list { +struct dest_vm_and_perm_info *dest_info; +u64 list_size; +};
These structures are confusing. From what I can tell we need an array of struct mem_prot_info and an array of struct dest_vm_and_perm_info in our pre-allocated buffer. The other info_list and dest_info_list structures are bookkeeping things that we pass to the firmware via registers. So let's drop the info_list structures and have the functions that populate arrays take pointers to the memory region to populate and return size_t?
static size_t populate_dest_info(struct dest_vm_and_perm_info *info,
struct int *dest_vmids, int nelements, int *dest_perms);
static size_t populate_prot_info_from_table(struct mem_prot_info *info,
struct sg_table *table);
And then the callers can add the size returned to the memory chunk pointer.
void *qcom_secure_mem; struct mem_prot_info *prot_info = qcom_secure_mem; struct dest_vm_and_perm_info *dest_info; size_t size; size = populate_prot_info_from_table(prot_info, ...); ... dest_info = qcom_secure_mem + size; size = populate_dest_info(dest_info, ...); smc_call()...
}
drivers/soc/qcom/secure_buffer.c line 57
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+ +static void *qcom_secure_mem; +#define QCOM_SECURE_MEM_SIZE (512 * 1024) +#define PADDING 32
What is the padding for? cache aligning? 32 seems magical.
drivers/soc/qcom/secure_buffer.c line 60
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ +static void populate_dest_info(int *dest_vmids, int nelements, + int *dest_perms, struct dest_info_list **list,
s/list/list_p/
drivers/soc/qcom/secure_buffer.c line 66
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c new file +6
-0+ void *current_qcom_secure_mem) +{ +struct dest_vm_and_perm_info *dest_info; +int i; + + dest_info = (struct dest_vm_and_perm_info *)current_qcom_secure_mem;
Useless cast, please remove.
drivers/soc/qcom/secure_buffer.c line 75
Quoted 9 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+ +for (i =0 ; i < nelements; i++) { + dest_info[i].vm = dest_vmids[i]; + dest_info[i].perm = dest_perms[i]; + dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + } + + *list = (struct dest_info_list *)&dest_info[i];
Grow a local variable:
struct dest_info_list *list = *list_p; list = &dest_info[i]; list->dest_info = dest_info; list->list_size = nelements * sizeof(*dest_info);
Of course this may all change anyway.
drivers/soc/qcom/secure_buffer.c line 88
Quoted 13 lines
drivers/soc/qcom/secure_buffer.c new file +13
-0+ + (*list)->dest_info = dest_info; + (*list)->list_size = nelements *sizeof (struct dest_vm_and_perm_info ); +} + +static void get_info_list_from_table (struct sg_table *table, +struct info_list **list) +{ +int i; +struct scatterlist *sg; +struct mem_prot_info *info; + + info = (struct mem_prot_info *)qcom_secure_mem;
Useless cast from void.
Quoted 40 lines
drivers/soc/qcom/secure_buffer.c new file +40
-0+ +for_each_sg (table->sgl, sg, table->nents, i) { + info[i].addr =page_to_phys (sg_page (sg)); + info[i].size = sg->length; + } + + *list = (struct info_list *)&(info[i]); + + (*list)->list_head = info; + (*list)->list_size = table->nents *sizeof (struct mem_prot_info ); +} + +int hyp_assign_table (struct device *dev,struct sg_table *table, +u32 *source_vm_list,int source_nelems, +int *dest_vmids,int *dest_perms, +int dest_nelems) +{ +int ret; +struct info_list *info_list =NULL ; +struct dest_info_list *dest_info_list =NULL ; +struct scm_desc desc = {0 }; +u32 *source_vm_copy; +void *current_qcom_secure_mem; + +size_t reqd_size = dest_nelems *sizeof (struct dest_vm_and_perm_info ) + + table->nents *sizeof (struct mem_prot_info ) + +sizeof (dest_info_list) +sizeof (info_list) +PADDING ; + +if (!qcom_secure_mem) { +pr_err ("%s is not functional as qcom_secure_mem is not allocated.\n" , + __func__); +return -ENOMEM ; + } + +if (reqd_size >QCOM_SECURE_MEM_SIZE ) { +pr_err ("%s: Not enough memory allocated. Required size %zd\n" , + __func__, reqd_size); +return -EINVAL ; + } +
drivers/soc/qcom/secure_buffer.c line 138
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+/* +* We can only pass cache-aligned sizes to hypervisor, so we need +* to kmalloc and memcpy the source_vm_list here. +*/ + source_vm_copy =kmalloc_array ( + source_nelems,sizeof (*source_vm_copy),GFP_KERNEL ); +if (!source_vm_copy) +return -ENOMEM ; +memcpy (source_vm_copy, source_vm_list, +sizeof (*source_vm_list) * source_nelems);
All of these u32s need an endian swap on big-endian platforms when they're copied.
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c new file +8
-0+ +mutex_lock (&secure_buffer_mutex); + +get_info_list_from_table (table, &info_list); + + current_qcom_secure_mem = &(info_list[1 ]); +populate_dest_info (dest_vmids, dest_nelems, dest_perms, + &dest_info_list, current_qcom_secure_mem);
drivers/soc/qcom/secure_buffer.c line 156
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+ + desc.args[0 ] =virt_to_phys (info_list->list_head); + desc.args[1 ] = info_list->list_size; + desc.args[2 ] =virt_to_phys (source_vm_copy); + desc.args[3 ] =sizeof (*source_vm_copy) * source_nelems; + desc.args[4 ] =virt_to_phys (dest_info_list->dest_info); + desc.args[5 ] = dest_info_list->list_size; + desc.args[6 ] =0 ; + +dma_set_mask (dev,DMA_BIT_MASK (64 ));
The dev passed here should be the scm->dev because we don't want to allocate memory from a memory region that may be associated with some random device using this API, and also we want to be able to have the right mask set for communicating with the firmware, which may be different than whatever mask is needed for DMA with a device.
drivers/soc/qcom/secure_buffer.c line 157
Quoted 1 line
drivers/soc/qcom/secure_buffer.c new file +1
-0+ dma_map_single(dev, (void *)source_vm_copy,
Useless cast to void.
drivers/soc/qcom/secure_buffer.c line 158
Quoted 1 line
drivers/soc/qcom/secure_buffer.c new file +1
-0+ (size_t)(source_vm_copy + source_nelems),
This looks wrong? source_vm_copy is a pointer and we're adding source_nelems and then casting that address to a size_t which would be potentially very large? Shouldn't it just be desc.args[3]?
drivers/soc/qcom/secure_buffer.c line 165
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+ DMA_TO_DEVICE); +dma_map_single (dev, (void *)info_list->list_head, + (size_t )(info_list->list_head + + (info_list->list_size /sizeof (*info_list->list_head))), +DMA_TO_DEVICE ); + dma_map_single(dev, + (void *)dest_info_list->dest_info,
Useless cast to void.
drivers/soc/qcom/secure_buffer.c line 171
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c new file +6
-0+ (size_t )(dest_info_list->dest_info + + (dest_info_list->list_size / +sizeof (*dest_info_list->dest_info))), + DMA_TO_DEVICE); + + ret =qcom_scm_assign_mem ((void *)&desc);
Useless cast to void.
drivers/soc/qcom/secure_buffer.c line 173
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+if (ret) + pr_info("%s: Failed to assign memory protection, ret = %d\n",
pr_err?
drivers/soc/qcom/secure_buffer.c line 176
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ __func__, ret); + +mutex_unlock (&secure_buffer_mutex);
Missing the dma_unmap calls here? Failure to do that could lead to a leak if we bounce the page.
Also, shouldn't we skip the dma mapping if we have allocated coherent memory instead of kmalloc for the qcom_secure_mem buffer? The code seems to ignore the dma allocation case.
drivers/soc/qcom/secure_buffer.c line 189
Quoted 13 lines
drivers/soc/qcom/secure_buffer.c new file +13
-0+kfree (source_vm_copy); +return ret; +} + +int hyp_assign_phys (struct device *dev,phys_addr_t addr,u64 size, +u32 *source_vm_list,int source_nelems, +int *dest_vmids,int *dest_perms, +int dest_nelems) +{ +struct sg_table *table; +int ret; + + table =kzalloc (sizeof (struct sg_table ),GFP_KERNEL );
sizeof(*table)
drivers/soc/qcom/secure_buffer.c line 191
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+if (!table) +return -ENOMEM ;
Newline here
drivers/soc/qcom/secure_buffer.c line 200
Quoted 9 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+ ret =sg_alloc_table (table,1 ,GFP_KERNEL ); +if (ret) + goto err1; + +sg_set_page (table->sgl,phys_to_page (addr), size,0 ); + + ret =hyp_assign_table (dev, table, source_vm_list, + source_nelems, dest_vmids, + dest_perms, dest_nelems);
I'd prefer two user facing APIs exist. One that takes a single address and size argument, and one that takes an sg_table. Both APIs can then call some common function that passes that info off to the firmware, but the first one can be used here without requiring us to make an sg_table for no reason besides undoing the sg_table in hyp_assign_table().
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+if (ret) + goto err2; + +return ret;
drivers/soc/qcom/secure_buffer.c line 214
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+err2: +sg_free_table (table); +err1: +kfree (table); +return ret; +} + +static int __init alloc_secure_shared_memory (void ) +{ +int ret =0 ;
ret is 0...
drivers/soc/qcom/secure_buffer.c line 219
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+ + qcom_secure_mem =kzalloc (QCOM_SECURE_MEM_SIZE ,GFP_KERNEL ); +if (!qcom_secure_mem) { +/* Fallback to CMA-DMA memory */ + qcom_secure_mem = dma_alloc_coherent(NULL, QCOM_SECURE_MEM_SIZE,
We can't really pass NULL here anymore. Use the scm device.
drivers/soc/qcom/secure_buffer.c line 222
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ NULL, GFP_KERNEL); +if (!qcom_secure_mem) { +pr_err ("Couldn't allocate memory for secure use-cases. hyp_assign_table will not work\n" );
Memory allocation messages are practically useless. Please remove them.
drivers/soc/qcom/secure_buffer.c line 178
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+return -ENOMEM ; + } + } + +return ret;
And that's all for ret. Always 0.
drivers/soc/qcom/secure_buffer.c line 229
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+} +pure_initcall (alloc_secure_shared_memory);
pure initcall? Maybe we should take the lazy approach and allocate this big chunk once someone calls into the API the first time? If we merge this with scm device, then we can probably do the allocation when scm probes and we have a proper device for dma allocation.
Quoted 17 lines
include/soc/qcom/secure_buffer.h new file +11
-0@@ -0,0 +1,51 @@ +/* +* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details.
include/soc/qcom/secure_buffer.h line 21
Quoted 10 lines
include/soc/qcom/secure_buffer.h new file +10
-0+* +*/ + +#ifndef __MSM_SECURE_BUFFER_H__ +#define __MSM_SECURE_BUFFER_H__ + +enum vmid { +VMID_HLOS =0x3 , +VMID_MSS_MSA =0xF , + VMID_INVAL = -1
Just drop the enum and use #defines please.
Quoted 5 lines
include/soc/qcom/secure_buffer.h new file +5
-0+}; + +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.
Quoted 8 lines · Stephen Boyd
Quoted 3 lines · Avaneesh Kumar Dwivedi
This patch add hypervisor support for mss bring up on msm8996. MSS rproc driver make hypervisor request to add certain region in IPA table owned by hepervisor and assign access permission
Please drop the use of IPA here. There's an IPA acronym for the IP accelerator and that is confused with Intermediate Physical Address. Instead, say something like "in the stage 2 page tables of the SMMU". Also hypervisor is misspelled here.
OK.
Quoted 10 lines
Quoted 9 lines
to modem. These regions are used to load MBA, MDT, FW into DDR.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- drivers/firmware/qcom_scm-64.c | 16 +++ drivers/firmware/qcom_scm.c | 13 +++ drivers/firmware/qcom_scm.h | 10 ++ drivers/remoteproc/qcom_q6v5_pil.c | 96 +++++++++++++++-Please split the remoteproc code off from this patch.
Ok.
Quoted 16 lines
Quoted 16 lines
drivers/soc/qcom/Kconfig | 8 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/secure_buffer.c | 229 +++++++++++++++++++++++++++++++++++++ include/linux/qcom_scm.h | 1 + include/soc/qcom/secure_buffer.h | 51 +++++++++ 9 files changed, 419 insertions(+), 6 deletions(-)create mode 100644
drivers/soc/qcom/secure_buffer.ccreate mode 100644include/soc/qcom/secure_buffer.hdrivers/firmware/qcom_scm-64.c +0
-0@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
Quoted 8 lines
drivers/firmware/qcom_scm-64.c int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) · line 365
Quoted 7 lines
drivers/firmware/qcom_scm-64.c +5
-0return ret ? : res.a1; } + +int __qcom_scm_assign_mem (struct device *dev,void *data) +{ +int ret; +struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;Useless cast from void.
Yes, will correct.
Quoted 19 lines
Quoted 19 lines
drivers/firmware/qcom_scm-64.c +11
-0+struct arm_smccc_res res; + + desc->arginfo =QCOM_SCM_ARGS (7 ,SCM_RO ,SCM_VAL ,SCM_RO , +SCM_VAL ,SCM_RO ,SCM_VAL ,SCM_VAL ); + + ret =qcom_scm_call (dev,QCOM_SCM_SVC_MP , +QCOM_MEM_PROT_ASSIGN_ID , + desc, &res); + +return ret ? : res.a1; +}drivers/firmware/qcom_scm.c +0
-0@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown);
Quoted 3 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 296
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+/** +* qcom_scm_assign_mem() - Some words on what the function does?
OK.
Quoted 9 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 303
Quoted 8 lines
drivers/firmware/qcom_scm.c +7
-0+* @desc: descriptor to send to hypervisor +* +* Return 0 on success. +*/ +int qcom_scm_assign_mem (void *desc) +{ +Nitpick: Drop the extra newline
OK.
Quoted 33 lines
Quoted 33 lines
drivers/firmware/qcom_scm.c +4
-0+return __qcom_scm_assign_mem (__scm->dev, desc); +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {drivers/firmware/qcom_scm.h +3
-0@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,#define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6#define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7#define QCOM_SCM_PAS_MSS_RESET 0xa +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16extern bool __qcom_scm_pas_supported (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_init_image (struct device *dev,u32 peripheral,dma_addr_t metadata_phys);@@ -55,6 +57,7 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,extern int __qcom_scm_pas_auth_and_reset (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_shutdown (struct device *dev,u32 peripheral);extern int __qcom_scm_pas_mss_reset (struct device *dev,bool reset); +extern int __qcom_scm_assign_mem (struct device *dev,void *desc);/* common error codes */ #define QCOM_SCM_V2_EBUSY -12@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)return -EINVAL ; }
Quoted 8 lines
drivers/firmware/qcom_scm.h static inline int qcom_scm_remap_error(int err) · line 94
Quoted 6 lines
drivers/firmware/qcom_scm.h +6
-0+enum scm_arg_types { +SCM_VAL , +SCM_RO , +SCM_RW , +SCM_BUFVAL , +};We already have this enum? It's just prepended with
QCOM_instead.
I think i missed somehow, will see and correct.
Quoted 19 lines
Quoted 19 lines
drivers/firmware/qcom_scm.h +1
-0+#endif drivers/remoteproc/qcom_q6v5_pil.c +1
-0@@ -31,6 +31,7 @@#include <linux/soc/qcom/smem.h> #include <linux/soc/qcom/smem_state.h> #include <linux/of_device.h> +#include <soc/qcom/secure_buffer.h> #include "remoteproc_internal.h" #include "qcom_mdt_loader.h" @@ -105,12 +106,19 @@ struct qcom_mss_reg_res {int uA; };
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c struct qcom_mss_reg_res { · line 112
Quoted 4 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+struct src_dest_vmid { +int *srcVM; +int *destVM; +int *destVMperm;Why not have an array of these structures instead of a structure with three arrays inside? Can you map one source VM to multiple destination VMs?
Yes one to multiple or multiple to one mapping is possible, i.e. grant access from HLOS only to HLOS+MSA.
Quoted 40 lines
Quoted 40 lines
drivers/remoteproc/qcom_q6v5_pil.c +12
-0+}; +struct rproc_hexagon_res {const char *hexagon_mba_image;struct qcom_mss_reg_res proxy_supply[4 ];struct qcom_mss_reg_res active_supply[2 ];char **proxy_clk_names;char **active_clk_names; +int version; };struct q6v5 {@@ -127,6 +135,8 @@ struct q6v5 {struct reset_control *mss_restart; +struct src_dest_vmid vmid_details; +struct qcom_smem_state *state;unsigned stop_bit;@@ -152,6 +162,13 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +int version; +}; + +enum { +MSS_MSM8916 , +MSS_MSM8974 , +MSS_MSM8996 , }; static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,@@ -273,13 +290,46 @@ static void q6v5_clk_disable(struct device *dev,clk_disable_unprepare (clks[i]); }
Quoted 14 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 305
Quoted 13 lines
drivers/remoteproc/qcom_q6v5_pil.c +13
-0+int hyp_mem_access (struct q6v5 *qproc,phys_addr_t addr,size_t size) +{ +int src_count =0 ; +int dest_count =0 ; +int ret; +int i; + +if (qproc->version !=MSS_MSM8996 ) +return 0 ; + +for (i =0 ; i <2 ; i++) { +if (qproc->vmid_details.srcVM[i] !=0 ) + src_count++;Bad tabbing here.
Yes, will correct.
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 307
Quoted 3 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0+if (qproc->vmid_details.destVM[i] !=0 ) + dest_count++;And here.
When is it ever == 0? The hardcoded 2 in the for loop is also suspect.
In downstream, VMID 0 is not assigned for any subsystem, my purpose here is to check if destination vmid is valid for some subsystem. On hard coded value question, will try to define a macro which will indicate MAX number of source or destination subsys VMID. which in this case is two i.e. HLOS and MODEM can be source or destination.
Quoted 2 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 308
Quoted 1 line
drivers/remoteproc/qcom_q6v5_pil.c +1
-0+ }Add newline
OK.
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c static void q6v5_clk_disable(struct device *dev, · line 312
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_pil.c +4
-0+ ret =hyp_assign_phys (qproc->dev, addr, size, + qproc->vmid_details.srcVM, + src_count, qproc->vmid_details.destVM, + qproc->vmid_details.destVMperm, dest_count);At the least this API could take a
vmid_detailsstructure instead of all these arguments:
Quoted 11 lines
struct vm_perms {
u32 vm;
u32 perm;
};
struct vmid_details {
u32 *src;
size_t src_count;
struct vm_perms *dest;
size_t dest_count;
};OK, will try to correct.
Quoted 9 lines
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_pil.c +7
-0+if (ret) +dev_err (qproc->dev, +"%s: failed for %pa address of size %zx\n" , + __func__, &addr, size); +return ret; +} +[...]
Quoted 12 lines
drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 80
Quoted 10 lines
drivers/soc/qcom/Kconfig +2
-0@@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL help Client driver for the WCNSS_CTRL SMD channel, used to download nv firmware to a newly booted WCNSS chip. + +config QCOM_SECURE_BUFFERPlease no. Just fold this into the
qcom_scm.ccode and drop the new config and new file.
Will try to incorporate this functionality in qcom_scm related source files.
Quoted 21 lines
Quoted 21 lines
drivers/soc/qcom/Kconfig +6
-0+ bool "Helper functions for securing buffers through TZ" + help + Say 'Y' here for targets that need to call into TZ to secure + memory buffers. This ensures that only the correct clients can + use this memory and no unauthorized access is made to the + bufferdrivers/soc/qcom/secure_buffer.c new file +8
-0@@ -0,0 +1,229 @@ +/* +* Copyright (c) 2011-2017, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful,
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 15
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ + +#include <linux/highmem.h> Used?
I think i missed to remove these, will remove them.
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c line 17
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+#include <linux/kernel.h> +#include <linux/kref.h> Used?
Will check and remove.
Quoted 11 lines
drivers/soc/qcom/secure_buffer.c line 26
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+#include <linux/mutex.h> +#include <linux/scatterlist.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> +#include <linux/qcom_scm.h> +#include <linux/dma-mapping.h> +#include <soc/qcom/secure_buffer.h> + +DEFINE_MUTEX (secure_buffer_mutex);static?
OK, will correct.
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c line 31
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+ +struct scm_desc { +u32 arginfo; +u64 args[10 ]; +};This would be in
qcom_scm-64.calready.
OK.
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c line 35
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+ +struct mem_prot_info { +phys_addr_t addr; +u64 size;Both should be
__le64presumably.
Yes, it should be in basic data type, but then i took it from downstream which keep address and size detail in above format.
Quoted 4 lines
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+}; + +struct info_list { +struct mem_prot_info *list_head;
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 46
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+u64 list_size; +}; + +struct dest_vm_and_perm_info { +u32 vm; +u32 perm; +u32 *ctx;Why is this a pointer?
I had taken reference from 3.18 kernel based downstream source code, it is not used and initialized with NULL but that is how TZ accept parameters.
Quoted 3 lines
Is this structure put into memory and
passed to the firmware? We should use the appropriate__le32and__le64types then.
OK.
Quoted 38 lines
drivers/soc/qcom/secure_buffer.c line 53
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+u32 ctx_size; +}; + +struct dest_info_list { +struct dest_vm_and_perm_info *dest_info; +u64 list_size; +};These structures are confusing. From what I can tell we need an array of struct
mem_prot_infoand an array of structdest_vm_and_perm_infoin our pre-allocated buffer. The otherinfo_listanddest_info_liststructures are bookkeeping things that we pass to the firmware via registers. So let's drop theinfo_liststructures and have the functions that populate arrays take pointers to the memory region to populate and returnsize_t?static
size_tpopulate_dest_info(structdest_vm_and_perm_info*info,struct int *dest_vmids, int nelements, int *dest_perms);static
size_tpopulate_prot_info_from_table(structmem_prot_info*info,struct sg_table *table);And then the callers can add the size returned to the memory chunk pointer.
void *qcom_secure_mem; struct mem_prot_info *prot_info = qcom_secure_mem; struct dest_vm_and_perm_info *dest_info; size_t size; size = populate_prot_info_from_table(prot_info, ...); ... dest_info = qcom_secure_mem + size; size = populate_dest_info(dest_info, ...); smc_call()...
Quoted 1 line
}
OK, Will try to modify as above.
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c line 57
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+ +static void *qcom_secure_mem; +#define QCOM_SECURE_MEM_SIZE (512 * 1024) +#define PADDING 32What is the padding for? cache aligning? 32 seems magical.
I have taken bare minimum version of this driver to make hyp_assign call for msm8996, this PADDING does not seem doing anything useful. It is not related with cache alignment. its only purpose seems that size of qcom_secure_mem should be at least 32 byte more than size of all info to be passed.
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c line 60
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ +static void populate_dest_info(int *dest_vmids, int nelements, + int *dest_perms, struct dest_info_list **list,s/list/list_p/
Did not get what is ask here?
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 66
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +6
-0+ void *current_qcom_secure_mem) +{ +struct dest_vm_and_perm_info *dest_info; +int i; + + dest_info = (struct dest_vm_and_perm_info *)current_qcom_secure_mem;Useless cast, please remove.
OK
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c line 75
Quoted 9 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+ +for (i =0 ; i < nelements; i++) { + dest_info[i].vm = dest_vmids[i]; + dest_info[i].perm = dest_perms[i]; + dest_info[i].ctx =NULL ; + dest_info[i].ctx_size =0 ; + } + + *list = (struct dest_info_list *)&dest_info[i];Grow a local variable:
OK
Quoted 8 lines
struct dest_info_list *list = *list_p; list = &dest_info[i]; list->dest_info = dest_info; list->list_size = nelements * sizeof(*dest_info);Of course this may all change anyway.
OK.
Quoted 15 lines
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ + (*list)->dest_info = dest_info; + (*list)->list_size = nelements *sizeof (struct dest_vm_and_perm_info );drivers/soc/qcom/secure_buffer.c line 88
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+} + +static void get_info_list_from_table (struct sg_table *table, +struct info_list **list) +{ +int i; +struct scatterlist *sg; +struct mem_prot_info *info; + + info = (struct mem_prot_info *)qcom_secure_mem;Useless cast from void.
OK
Quoted 45 lines
Quoted 45 lines
drivers/soc/qcom/secure_buffer.c new file +44
-0+ +for_each_sg (table->sgl, sg, table->nents, i) { + info[i].addr =page_to_phys (sg_page (sg)); + info[i].size = sg->length; + } + + *list = (struct info_list *)&(info[i]); + + (*list)->list_head = info; + (*list)->list_size = table->nents *sizeof (struct mem_prot_info ); +} + +int hyp_assign_table (struct device *dev,struct sg_table *table, +u32 *source_vm_list,int source_nelems, +int *dest_vmids,int *dest_perms, +int dest_nelems) +{ +int ret; +struct info_list *info_list =NULL ; +struct dest_info_list *dest_info_list =NULL ; +struct scm_desc desc = {0 }; +u32 *source_vm_copy; +void *current_qcom_secure_mem; + +size_t reqd_size = dest_nelems *sizeof (struct dest_vm_and_perm_info ) + + table->nents *sizeof (struct mem_prot_info ) + +sizeof (dest_info_list) +sizeof (info_list) +PADDING ; + +if (!qcom_secure_mem) { +pr_err ("%s is not functional as qcom_secure_mem is not allocated.\n" , + __func__); +return -ENOMEM ; + } + +if (reqd_size >QCOM_SECURE_MEM_SIZE ) { +pr_err ("%s: Not enough memory allocated. Required size %zd\n" , + __func__, reqd_size); +return -EINVAL ; + } + +/* +* We can only pass cache-aligned sizes to hypervisor, so we need +* to kmalloc and memcpy the source_vm_list here. +*/
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 138
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c new file +6
-0+ source_vm_copy =kmalloc_array ( + source_nelems,sizeof (*source_vm_copy),GFP_KERNEL ); +if (!source_vm_copy) +return -ENOMEM ; +memcpy (source_vm_copy, source_vm_list, +sizeof (*source_vm_list) * source_nelems);All of these u32s need an endian swap on big-endian platforms when they're copied.
OK.
Quoted 14 lines
Quoted 14 lines
drivers/soc/qcom/secure_buffer.c new file +13
-0+ +mutex_lock (&secure_buffer_mutex); + +get_info_list_from_table (table, &info_list); + + current_qcom_secure_mem = &(info_list[1 ]); +populate_dest_info (dest_vmids, dest_nelems, dest_perms, + &dest_info_list, current_qcom_secure_mem); + + desc.args[0 ] =virt_to_phys (info_list->list_head); + desc.args[1 ] = info_list->list_size; + desc.args[2 ] =virt_to_phys (source_vm_copy); + desc.args[3 ] =sizeof (*source_vm_copy) * source_nelems;
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 156
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+ desc.args[4 ] =virt_to_phys (dest_info_list->dest_info); + desc.args[5 ] = dest_info_list->list_size; + desc.args[6 ] =0 ; + +dma_set_mask (dev,DMA_BIT_MASK (64 ));The dev passed here should be the
scm->devbecause we don't want to allocate memory from a memory region that may be associated with some random device using this API, and
OK.
Quoted 4 lines
also we want to be
able to have the right mask set for communicating with the firmware, which may be different than whatever mask is needed for DMA with a device.
dma_map_single need a non NULL dev pointer to set mask, which scm->dev will provide.
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c line 157
Quoted 1 line
drivers/soc/qcom/secure_buffer.c new file +1
-0+ dma_map_single(dev, (void *)source_vm_copy,Useless cast to void.
OK.
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c line 158
Quoted 2 lines
drivers/soc/qcom/secure_buffer.c new file +1
-0+ (size_t)(source_vm_copy + source_nelems),This looks wrong?
source_vm_copyis a pointer and we're addingsource_nelemsand then casting that address to asize_twhich would be potentially very large? Shouldn't it just be desc.args[3]?
My Bad, yes will correct.
Quoted 9 lines
drivers/soc/qcom/secure_buffer.c line 165
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c new file +7
-0+ DMA_TO_DEVICE); +dma_map_single (dev, (void *)info_list->list_head, + (size_t )(info_list->list_head + + (info_list->list_size /sizeof (*info_list->list_head))), +DMA_TO_DEVICE ); + dma_map_single(dev, + (void *)dest_info_list->dest_info,Useless cast to void.
OK.
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 171
Quoted 7 lines
drivers/soc/qcom/secure_buffer.c new file +6
-0+ (size_t )(dest_info_list->dest_info + + (dest_info_list->list_size / +sizeof (*dest_info_list->dest_info))), + DMA_TO_DEVICE); + + ret =qcom_scm_assign_mem ((void *)&desc);Useless cast to void.
OK.
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c line 173
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+if (ret) + pr_info("%s: Failed to assign memory protection, ret = %d\n",
pr_err?
OK.
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c line 176
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ __func__, ret); + +mutex_unlock (&secure_buffer_mutex);Missing the
dma_unmapcalls here? Failure to do that could lead to a leak if we bounce the page.Also, shouldn't we skip the dma mapping if we have allocated coherent memory instead of kmalloc for the
qcom_secure_membuffer? The code seems to ignore the dma allocation case.
problem with coherent memory allocation is uncertainty of getting such chunk or carving it out for always.
Quoted 15 lines
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+kfree (source_vm_copy); +return ret; +}drivers/soc/qcom/secure_buffer.c line 189
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+ +int hyp_assign_phys (struct device *dev,phys_addr_t addr,u64 size, +u32 *source_vm_list,int source_nelems, +int *dest_vmids,int *dest_perms, +int dest_nelems) +{ +struct sg_table *table; +int ret; + + table =kzalloc (sizeof (struct sg_table ),GFP_KERNEL );
sizeof(*table)
OK.
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c line 191
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+if (!table) +return -ENOMEM ;Newline here
OK.
Quoted 16 lines
drivers/soc/qcom/secure_buffer.c line 200
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +9
-0+ ret =sg_alloc_table (table,1 ,GFP_KERNEL ); +if (ret) + goto err1; + +sg_set_page (table->sgl,phys_to_page (addr), size,0 ); + + ret =hyp_assign_table (dev, table, source_vm_list, + source_nelems, dest_vmids, + dest_perms, dest_nelems);I'd prefer two user facing APIs exist. One that takes a single address and size argument, and one that takes an
sg_table. Both APIs can then call some common function that passes that info off to the firmware, but the first one can be used here without requiring us to make ansg_tablefor no reason besides undoing thesg_tableinhyp_assign_table().
if you are OK, i will use a private API which will do away requirement of allocating sg_table as well as doing dma_single_map() i will directly pass src and dest VM info to TZ.
Quoted 17 lines
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +4
-0+if (ret) + goto err2; + +return ret;drivers/soc/qcom/secure_buffer.c line 214
Quoted 10 lines
drivers/soc/qcom/secure_buffer.c new file +10
-0+err2: +sg_free_table (table); +err1: +kfree (table); +return ret; +} + +static int __init alloc_secure_shared_memory (void ) +{ +int ret =0 ;ret is 0...
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c line 219
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+ + qcom_secure_mem =kzalloc (QCOM_SECURE_MEM_SIZE ,GFP_KERNEL ); +if (!qcom_secure_mem) { +/* Fallback to CMA-DMA memory */ + qcom_secure_mem = dma_alloc_coherent(NULL, QCOM_SECURE_MEM_SIZE,We can't really pass NULL here anymore. Use the scm device.
OK.
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c line 222
Quoted 4 lines
drivers/soc/qcom/secure_buffer.c new file +3
-0+ NULL, GFP_KERNEL); +if (!qcom_secure_mem) { +pr_err ("Couldn't allocate memory for secure use-cases. hyp_assign_table will not work\n" );Memory allocation messages are practically useless. Please remove them.
OK.
Quoted 6 lines
drivers/soc/qcom/secure_buffer.c line 178
Quoted 5 lines
drivers/soc/qcom/secure_buffer.c new file +5
-0+return -ENOMEM ; + } + } + +return ret;And that's all for ret. Always 0.
OK.
Quoted 8 lines
drivers/soc/qcom/secure_buffer.c line 229
Quoted 3 lines
drivers/soc/qcom/secure_buffer.c new file +2
-0+} +pure_initcall (alloc_secure_shared_memory);pure initcall? Maybe we should take the lazy approach and allocate this big chunk once someone calls into the API the first time? If we merge this with scm device, then we can probably do the allocation when scm probes and we have a proper device for dma allocation.
OK.
Quoted 21 lines
Quoted 21 lines
include/soc/qcom/secure_buffer.h new file +14
-0@@ -0,0 +1,51 @@ +/* +* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ +
Quoted 8 lines
include/soc/qcom/secure_buffer.h line 21
Quoted 7 lines
include/soc/qcom/secure_buffer.h new file +7
-0+#ifndef __MSM_SECURE_BUFFER_H__ +#define __MSM_SECURE_BUFFER_H__ + +enum vmid { +VMID_HLOS =0x3 , +VMID_MSS_MSA =0xF , + VMID_INVAL = -1Just drop the enum and use #defines please.
OK.
Quoted 6 lines
Quoted 6 lines
include/soc/qcom/secure_buffer.h new file +5
-0+}; + +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
RFC 2/2 remoteproc: qcom: Add mss rproc support for msm8996.
In reply to Avaneesh Kumar DwivediThis patch add msm8996 specific code piece to
take mss out of reset.
--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 +- drivers/remoteproc/qcom_q6v5_pil.c | 179 ++++++++++++++++++--- 2 files changed, 155 insertions(+), 26 deletions(-)
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -1
@@ -10,7 +10,7 @@ on the Qualcomm Hexagon core. "qcom,q6v5-pil" "qcom,msm8916-mss-pil" "qcom,msm8974-mss-pil" - + "qcom,msm8996-mss-pil" - reg: Usage: required Value type: <prop-encoded-array>
drivers/remoteproc/qcom_q6v5_pil.c +154 -25
235 lines not on this page · raw message
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.