Add memory ownership switch support and enable mss rproc on msm8996
v5 cover Add memory ownership switch support and enable mss rproc on msm8996
This patch set does following
1- Adds new scm call which helps in stage two translation of a memory region
so that memory ownership sharing and switching can be achieved on armv8 and later.
2- Enable mss remoteproc on msm8996
Major changes since last patch:
1- New SCM API now returns effective owner's set for the memory. 2- Logic to call SCM API to switch ownership of mem region has been changed
in MSS rproc driver.
3- proxy and active supply initialization has been left out in case of msm 8996 4- documentation of the SCM API has been changed as per comment. 5- logging of fail cases during memory ownership swiitch has been readjusted. 5- Other indentation and small errors.
Avaneesh Kumar Dwivedi (4):
firmware: scm: Add new SCM call API for switching memory ownership
remoteproc: qcom: refactor mss fw image loading sequence
remoteproc: qcom: Make secure world call for mem ownership switch
remoteproc: qcom: Add support for mss remoteproc on msm8996
.../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/firmware/qcom_scm-32.c | 6 + drivers/firmware/qcom_scm-64.c | 27 +++ drivers/firmware/qcom_scm.c | 92 +++++++ drivers/firmware/qcom_scm.h | 5 + drivers/remoteproc/qcom_q6v5_pil.c | 263 ++++++++++++++++++--- include/linux/qcom_scm.h | 16 ++ 7 files changed, 373 insertions(+), 37 deletions(-)
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.
PATCH v5 1/4 firmware: scm: Add new SCM call API for switching memory ownership
Two different processors on a SOC need to switch memory ownership during load/unload. To enable this, level second memory map table need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for memory ownership switching request.
--- drivers/firmware/qcom_scm-32.c | 6 +++ drivers/firmware/qcom_scm-64.c | 27 +++++++++++++ drivers/firmware/qcom_scm.c | 92 ++++++++++++++++++++++++++++++++++++++++++ drivers/firmware/qcom_scm.h | 5 +++ include/linux/qcom_scm.h | 16 ++++++++ 5 files changed, 146 insertions(+)
drivers/firmware/qcom_scm-32.c +6 -0
@@ -596,3 +596,9 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size, {return -ENODEV ; } +int __qcom_scm_assign_mem (struct device *dev,phys_addr_t mem_region, +size_t mem_sz,phys_addr_t src,size_t src_sz, +phys_addr_t dest,size_t dest_sz) +{ +return -ENODEV ; +}
drivers/firmware/qcom_scm-64.c +27 -0
@@ -439,3 +439,30 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,return ret; } + +int __qcom_scm_assign_mem (struct device *dev,phys_addr_t mem_region, +size_t mem_sz,phys_addr_t src,size_t src_sz, +phys_addr_t dest,size_t dest_sz) +{ +int ret; +struct qcom_scm_desc desc = {0 }; +struct arm_smccc_res res; + + desc.args[0 ] = mem_region; + desc.args[1 ] = mem_sz; + desc.args[2 ] = src; + desc.args[3 ] = src_sz; + desc.args[4 ] = dest; + desc.args[5 ] = dest_sz; + desc.args[6 ] =0 ; + + desc.arginfo =QCOM_SCM_ARGS (7 ,QCOM_SCM_RO ,QCOM_SCM_VAL , +QCOM_SCM_RO ,QCOM_SCM_VAL ,QCOM_SCM_RO , +QCOM_SCM_VAL ,QCOM_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 +92 -0
104 lines not on this page · raw message
drivers/firmware/qcom_scm.h +5 -0
@@ -95,5 +95,10 @@ extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare, size_t *size);extern int __qcom_scm_iommu_secure_ptbl_init (struct device *dev,u64 addr,u32 size,u32 spare); +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem (struct device *dev, +phys_addr_t mem_region,size_t mem_sz, +phys_addr_t src,size_t src_sz, +phys_addr_t dest,size_t dest_sz);#endif
include/linux/qcom_scm.h +16 -0
@@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req {u32 val; }; +struct qcom_scm_vmperm { +int vmid; +int perm; +}; + +#define QCOM_SCM_VMID_HLOS 0x3 +#define QCOM_SCM_VMID_MSS_MSA 0xF +#define QCOM_SCM_PERM_READ 0x4 +#define QCOM_SCM_PERM_WRITE 0x2 +#define QCOM_SCM_PERM_EXEC 0x1 +#define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE) +#define QCOM_SCM_PERM_RWX (QCOM_SCM_PERM_RW | QCOM_SCM_PERM_EXEC) +#if IS_ENABLED (CONFIG_QCOM_SCM )extern int qcom_scm_set_cold_boot_addr (void *entry,const cpumask_t *cpus);extern int qcom_scm_set_warm_boot_addr (void *entry,const cpumask_t *cpus);
@@ -37,6 +50,9 @@ 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 (phys_addr_t mem_addr,size_t mem_sz, +int src,struct qcom_scm_vmperm *newvm, +int dest_cnt);extern void qcom_scm_cpu_power_down (u32 flags);extern u32 qcom_scm_get_version (void );extern int qcom_scm_set_remote_state (u32 state,u32 id);
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: [PATCH v5 1/4] firmware: scm: Add new SCM call API for switching memory ownership
Quoted 7 lines · Avaneesh Kumar Dwivedi
Two different processors on a SOC need to switch memory ownership during load/unload. To enable this, level second memory map table need to be updated, which is done by secure layer.
This patch add the interface for making secure monitor call for memory ownership switching request.Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>
Regards,
Bjorn
Re: [PATCH v5 1/4] firmware: scm: Add new SCM call API for switching memory ownership
In reply to Avaneesh Kumar DwivediQuoted 2 lines · Avaneesh Kumar Dwivedi
Two different processors on a SOC need to switch memory ownership during load/unload. To enable this, level second memory map table
second level page tables instead of level second memory map table
Quoted 2 lines
need to be updated, which is done by secure layer. This patch add the interface for making secure monitor call for
s/add/adds/
Quoted 29 lines
memory ownership switching request.
drivers/firmware/qcom_scm.c +20
-0@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown); +/** +* qcom_scm_assign_mem() - Make a secure call to reassign memory ownership +* +* @mem_addr: mem region whose ownership need to be reassigned +* @mem_sz: size of the region. +* @srcvm: vmid for current set of owners, each set bit in +* flag indicate a unique owner +* @newvm: array having new owners and corrsponding permission +* flags +* @dest_cnt: number of owners in next set. +* Return next set of owners on success. +*/ +int qcom_scm_assign_mem (phys_addr_t mem_addr,size_t mem_sz,int srcvm, +struct qcom_scm_vmperm *newvm,int dest_cnt) +{ +unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ; +struct qcom_scm_current_perm_info *destvm; +struct qcom_scm_mem_map_info *mem; +phys_addr_t memory_phys; +phys_addr_t dest_phys;
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 336
Quoted 10 lines
drivers/firmware/qcom_scm.c +10
-0+phys_addr_t src_phys; +size_t mem_all_sz; +size_t memory_sz; +size_t dest_sz; +size_t src_sz; +int next_vm; +__le32 *src; +void *ptr; +int ret; +int i;
Yay reverse christmas tre.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 338
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+ + src_sz =hweight_long (srcvm)*sizeof (*src);
Please add space around that '*':
src_sz = hweight_long(srcvm) * sizeof(*src);
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 346
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ memory_sz =sizeof (*mem); + dest_sz = dest_cnt*sizeof (*destvm); + mem_all_sz = src_sz + memory_sz + dest_sz; + + ptr =dma_alloc_attrs (__scm->dev,ALIGN (mem_all_sz,SZ_64 ), + &src_phys,GFP_KERNEL , dma_attrs); +if (!ptr) { +pr_err ("mem alloc failed\n" );
We don't want memory allocation failure prints. Please remove.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 348
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+return -ENOMEM ; + }
Newline here!
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 350
Quoted 2 lines
drivers/firmware/qcom_scm.c +2
-0+/* Fill source vmid detail */ + src = (__le32 *)(ptr);
Drop useless parenthesis around ptr please.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 351
Quoted 1 line
drivers/firmware/qcom_scm.c +1
-0+ ret =hweight_long (srcvm);
len = hweight_long(...)?
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 352
Quoted 1 line
drivers/firmware/qcom_scm.c +1
-0+for (i =0 ; i < ret; i++) {
i to ret is really weird looking!
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 355
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+ src[i] =cpu_to_le32 (ffs (srcvm) -1 ); + srcvm ^=1 << (ffs (srcvm) -1 ); + }
What if the loop was written like:
for_each_set_bit(i, &srcvm, sizeof(srcvm)) src[i] = cpu_to_le32(i);
I guess srvcm would have to be a long then.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 358
Quoted 3 lines
drivers/firmware/qcom_scm.c +3
-0+ +/* Fill details of mem buff to map */ + mem = (struct qcom_scm_mem_map_info *)(ptr +ALIGN (src_sz,SZ_64 ));
Useless cast from void *.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 366
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ memory_phys = src_phys +ALIGN (src_sz,SZ_64 ); + mem[0 ].mem_addr =cpu_to_le64 (mem_addr); + mem[0 ].mem_size =cpu_to_le64 (mem_sz); + + next_vm =0 ; +/* Fill details of next vmid detail */ + destvm = (struct qcom_scm_current_perm_info *) + (ptr +ALIGN (memory_sz,SZ_64 ) +ALIGN (src_sz,SZ_64 ));
Useless cast from void.
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 374
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ dest_phys = memory_phys +ALIGN (memory_sz,SZ_64 ); +for (i =0 ; i < dest_cnt; i++) { + destvm[i].vmid =cpu_to_le32 (newvm[i].vmid); + destvm[i].perm =cpu_to_le32 (newvm[i].perm); + destvm[i].ctx =0 ; + destvm[i].ctx_size =0 ; + next_vm |=BIT (newvm[i].vmid); + }
Newline please!
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 382
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ ret =__qcom_scm_assign_mem (__scm->dev, memory_phys, + memory_sz, src_phys, src_sz, dest_phys, dest_sz); +dma_free_attrs (__scm->dev,ALIGN (mem_all_sz,SZ_64 ), + ptr, src_phys, dma_attrs); +if (ret ==0 ) +return next_vm; +else if (ret >0 ) +return -ret;
When is ret > 0?
Quoted 7 lines
drivers/firmware/qcom_scm.c +4
-0+return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v5 1/4] firmware: scm: Add new SCM call API for switching memory ownership
Quoted 4 lines · Stephen Boyd
Quoted 2 lines · Avaneesh Kumar Dwivedi
Two different processors on a SOC need to switch memory ownership during load/unload. To enable this, level second memory map table
second level page tables instead of level second memory map table
OK
Quoted 4 lines
Quoted 3 lines
need to be updated, which is done by secure layer. This patch add the interface for making secure monitor call for
s/add/adds/
OK.
Quoted 33 lines
Quoted 33 lines
memory ownership switching request.
drivers/firmware/qcom_scm.c +23
-0@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral) }EXPORT_SYMBOL (qcom_scm_pas_shutdown); +/** +* qcom_scm_assign_mem() - Make a secure call to reassign memory ownership +* +* @mem_addr: mem region whose ownership need to be reassigned +* @mem_sz: size of the region. +* @srcvm: vmid for current set of owners, each set bit in +* flag indicate a unique owner +* @newvm: array having new owners and corrsponding permission +* flags +* @dest_cnt: number of owners in next set. +* Return next set of owners on success. +*/ +int qcom_scm_assign_mem (phys_addr_t mem_addr,size_t mem_sz,int srcvm, +struct qcom_scm_vmperm *newvm,int dest_cnt) +{ +unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ; +struct qcom_scm_current_perm_info *destvm; +struct qcom_scm_mem_map_info *mem; +phys_addr_t memory_phys; +phys_addr_t dest_phys; +phys_addr_t src_phys; +size_t mem_all_sz; +size_t memory_sz;
Quoted 8 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 336
Quoted 7 lines
drivers/firmware/qcom_scm.c +7
-0+size_t dest_sz; +size_t src_sz; +int next_vm; +__le32 *src; +void *ptr; +int ret; +int i;Yay reverse christmas tre.
Shall be reversed?
Quoted 4 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 338
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+ + src_sz =hweight_long (srcvm)*sizeof (*src);Please add space around that '*':
OK
Quoted 12 lines
src_sz = hweight_long(srcvm) * sizeof(*src);drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 346
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ memory_sz =sizeof (*mem); + dest_sz = dest_cnt*sizeof (*destvm); + mem_all_sz = src_sz + memory_sz + dest_sz; + + ptr =dma_alloc_attrs (__scm->dev,ALIGN (mem_all_sz,SZ_64 ), + &src_phys,GFP_KERNEL , dma_attrs); +if (!ptr) { +pr_err ("mem alloc failed\n" );We don't want memory allocation failure prints. Please remove.
OK
Quoted 4 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 348
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+return -ENOMEM ; + }Newline here!
OK
Quoted 4 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 350
Quoted 3 lines
drivers/firmware/qcom_scm.c +2
-0+/* Fill source vmid detail */ + src = (__le32 *)(ptr);Drop useless parenthesis around ptr please.
OK
Quoted 3 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 351
Quoted 2 lines
drivers/firmware/qcom_scm.c +1
-0+ ret =hweight_long (srcvm);len =
hweight_long(...)?
OK, thought to use less local variables. will change
Quoted 3 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 352
Quoted 2 lines
drivers/firmware/qcom_scm.c +1
-0+for (i =0 ; i < ret; i++) {i to ret is really weird looking!
Will check if can use below suggestion
Quoted 10 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 355
Quoted 4 lines
drivers/firmware/qcom_scm.c +3
-0+ src[i] =cpu_to_le32 (ffs (srcvm) -1 ); + srcvm ^=1 << (ffs (srcvm) -1 ); + }What if the loop was written like:
for_each_set_bit(i, &srcvm, sizeof(srcvm)) src[i] = cpu_to_le32(i);I guess srvcm would have to be a long then.
Will check and adopt
Quoted 5 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 358
Quoted 4 lines
drivers/firmware/qcom_scm.c +3
-0+ +/* Fill details of mem buff to map */ + mem = (struct qcom_scm_mem_map_info *)(ptr +ALIGN (src_sz,SZ_64 ));Useless cast from void *.
OK
Quoted 10 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 366
Quoted 9 lines
drivers/firmware/qcom_scm.c +8
-0+ memory_phys = src_phys +ALIGN (src_sz,SZ_64 ); + mem[0 ].mem_addr =cpu_to_le64 (mem_addr); + mem[0 ].mem_size =cpu_to_le64 (mem_sz); + + next_vm =0 ; +/* Fill details of next vmid detail */ + destvm = (struct qcom_scm_current_perm_info *) + (ptr +ALIGN (memory_sz,SZ_64 ) +ALIGN (src_sz,SZ_64 ));Useless cast from void.
OK
Quoted 9 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 374
Quoted 8 lines
drivers/firmware/qcom_scm.c +8
-0+ dest_phys = memory_phys +ALIGN (memory_sz,SZ_64 ); +for (i =0 ; i < dest_cnt; i++) { + destvm[i].vmid =cpu_to_le32 (newvm[i].vmid); + destvm[i].perm =cpu_to_le32 (newvm[i].perm); + destvm[i].ctx =0 ; + destvm[i].ctx_size =0 ; + next_vm |=BIT (newvm[i].vmid); + }Newline please!
OK
Quoted 10 lines
drivers/firmware/qcom_scm.c int qcom_scm_pas_shutdown(u32 peripheral) · line 382
Quoted 9 lines
drivers/firmware/qcom_scm.c +8
-0+ ret =__qcom_scm_assign_mem (__scm->dev, memory_phys, + memory_sz, src_phys, src_sz, dest_phys, dest_sz); +dma_free_attrs (__scm->dev,ALIGN (mem_all_sz,SZ_64 ), + ptr, src_phys, dma_attrs); +if (ret ==0 ) +return next_vm; +else if (ret >0 ) +return -ret;When is ret > 0?
SCM returns positive value in r1 register , and if r1 reg is non zero then that should be returned. Not sure what that indicate.
Quoted 8 lines
Quoted 8 lines
drivers/firmware/qcom_scm.c +4
-0+return ret; +} +EXPORT_SYMBOL (qcom_scm_assign_mem); +static int qcom_scm_pas_reset_assert (struct reset_controller_dev *rcdev,unsigned long idx) {
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.
PATCH v5 2/4 remoteproc: qcom: refactor mss fw image loading sequence
In reply to Avaneesh Kumar DwivediThis patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw.
--- drivers/remoteproc/qcom_q6v5_pil.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +8 -11
@@ -503,7 +503,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)bool relocate = false;char seg_name[10 ];ssize_t offset; -size_t size; +size_t size =0 ;void *ptr;int ret;int i;
@@ -541,7 +541,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; - +/* Load firmware segments */ for (i =0 ; i < ehdr->e_phnum; i++) { phdr = &phdrs[i];
@@ -574,18 +574,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)memset (ptr + phdr->p_filesz,0 , phdr->p_memsz - phdr->p_filesz); } - - size =readl (qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); -if (!size) { - boot_addr = relocate ? qproc->mpss_phys : min_addr; -writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); -writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); - } - size += phdr->p_memsz; -writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); } +/* Transfer ownership of modem ddr region with q6*/ + boot_addr = relocate ? qproc->mpss_phys : min_addr; +writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG ); +writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG ); +writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG ); + ret =q6v5_rmb_mba_wait (qproc,RMB_MBA_AUTH_COMPLETE ,10000 );if (ret == -ETIMEDOUT )dev_err (qproc->dev,"MPSS authentication timed out\n" );
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: [PATCH v5 2/4] remoteproc: qcom: refactor mss fw image loading sequence
Quoted 3 lines · Avaneesh Kumar Dwivedi
This patch refactor code to first load all firmware blobs and then update modem proc to authenticate and boot fw.
This looks good, but I have not had a chance to test this on older platforms yet.
Regards,
Bjorn
PATCH v5 3/4 remoteproc: qcom: Make secure world call for mem ownership switch
In reply to Avaneesh Kumar DwivediMSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem.
--- drivers/remoteproc/qcom_q6v5_pil.c | 81 +++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)
drivers/remoteproc/qcom_q6v5_pil.c +79 -2
@@ -110,6 +110,7 @@ struct rproc_hexagon_res {struct qcom_mss_reg_res *active_supply;char **proxy_clk_names;char **active_clk_names; +bool need_mem_protection; };struct q6v5 {
@@ -151,6 +152,7 @@ struct q6v5 {phys_addr_t mpss_reloc;void *mpss_region;size_t mpss_size; +bool need_mem_protection;struct qcom_rproc_subdev smd_subdev; };
@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_xfer_mem_ownership (struct q6v5 *qproc, +int image,phys_addr_t addr, +size_t size) +{ +static int current_owner[3 ][1 ] = {{BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )} }; +struct qcom_scm_vmperm next[] = {{0 } }; +int ret; + +if (!qproc->need_mem_protection) +return 0 ; + +if (current_owner[image][0 ] ==BIT (QCOM_SCM_VMID_HLOS )) { + next->vmid =QCOM_SCM_VMID_MSS_MSA ; + next->perm =QCOM_SCM_PERM_RW ; + }else { + next->vmid =QCOM_SCM_VMID_HLOS ; + next->perm =QCOM_SCM_PERM_RWX ; + } + + ret =qcom_scm_assign_mem (addr,ALIGN (size,SZ_4K ), + current_owner[image][0 ], next,1 ); +if (ret <0 ) { +pr_err ("Failed to assign %s memory access in range %p to %p ret = %d\n" , + (image ==0 ?"MDT" : image ==1 ?"MBA" :"MPSS" ), + (void *)addr, (void *)(addr + size), ret); +return ret; + } + + current_owner[image][0 ] = ret; +return 0 ; +} +static int q6v5_load (struct rproc *rproc,const struct firmware *fw) {struct q6v5 *qproc = rproc->priv;
@@ -450,6 +486,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) {unsigned long dma_attrs =DMA_ATTR_FORCE_CONTIGUOUS ;dma_addr_t phys; +int xferop_ret;void *ptr;int ret;
@@ -461,6 +498,10 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)memcpy (ptr, fw->data, fw->size); +/* Hypervisor mapping to access metadata by modem */ + ret =q6v5_xfer_mem_ownership (qproc,0 , phys, fw->size); +if (ret) +return -EAGAIN ;writel (phys, qproc->rmb_base +RMB_PMI_META_DATA_REG );writel (RMB_CMD_META_DATA_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );
@@ -470,6 +511,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)else if (ret <0 )dev_err (qproc->dev,"MPSS header authentication failed: %d\n" , ret); +/* Metadata authentication done, remove modem access */ + xferop_ret =q6v5_xfer_mem_ownership (qproc,0 , phys, fw->size); +if (xferop_ret) +dev_warn (qproc->dev, +"mdt buffer not reclaimed system may become unstable\n" );dma_free_attrs (qproc->dev, fw->size, ptr, phys, dma_attrs);return ret <0 ? ret :0 ;
@@ -579,6 +625,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc)/* Transfer ownership of modem ddr region with q6*/ boot_addr = relocate ? qproc->mpss_phys : min_addr; + ret =q6v5_xfer_mem_ownership (qproc,2 , + qproc->mpss_phys, qproc->mpss_size); +if (ret) +return -EAGAIN ;writel (boot_addr, qproc->rmb_base +RMB_PMI_CODE_START_REG );writel (RMB_CMD_LOAD_READY , qproc->rmb_base +RMB_MBA_COMMAND_REG );writel (size, qproc->rmb_base +RMB_PMI_CODE_LENGTH_REG );
@@ -598,6 +648,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)static int q6v5_start (struct rproc *rproc) {struct q6v5 *qproc = (struct q6v5 *)rproc->priv; +int xfermemop_ret;int ret; ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
@@ -633,6 +684,11 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } +/* Assign MBA image access in DDR to q6 */ + xfermemop_ret =q6v5_xfer_mem_ownership (qproc,1 , + qproc->mba_phys, qproc->mba_size); +if (xfermemop_ret) + goto assert_reset;writel (qproc->mba_phys, qproc->rmb_base +RMB_MBA_IMAGE_REG ); ret =q6v5proc_reset (qproc);
@@ -654,16 +710,21 @@ static int q6v5_start(struct rproc *rproc) ret =q6v5_mpss_load (qproc);if (ret) - goto halt_axi_ports; + goto reclaim_mem; ret =wait_for_completion_timeout (&qproc->start_done,msecs_to_jiffies (5000 ));if (ret ==0 ) {dev_err (qproc->dev,"start timed out\n" ); ret = -ETIMEDOUT ; - goto halt_axi_ports; + goto reclaim_mem; } + xfermemop_ret =q6v5_xfer_mem_ownership (qproc,1 , + qproc->mba_phys, qproc->mba_size); +if (xfermemop_ret) +dev_err (qproc->dev, +"Failed to reclaim mba buffer system may become unstable\n" ); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
@@ -673,12 +734,21 @@ static int q6v5_start(struct rproc *rproc)return 0 ; +reclaim_mem: + xfermemop_ret =q6v5_xfer_mem_ownership (qproc,2 , + qproc->mpss_phys, qproc->mpss_size); +WARN_ON (xfermemop_ret); halt_axi_ports:q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_q6);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count); + xfermemop_ret =q6v5_xfer_mem_ownership (qproc,1 , + qproc->mba_phys, qproc->mba_size); +if (xfermemop_ret) +dev_err (qproc->dev,"Failed to reclaim mba buffer, system may become unstable\n" ); + assert_reset:reset_control_assert (qproc->mss_restart); disable_vdd:
@@ -698,6 +768,7 @@ static int q6v5_stop(struct rproc *rproc) {struct q6v5 *qproc = (struct q6v5 *)rproc->priv;int ret; +u32 val; qproc->running = false;
@@ -715,6 +786,9 @@ static int q6v5_stop(struct rproc *rproc)q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_modem);q6v5proc_halt_axi_port (qproc, qproc->halt_map, qproc->halt_nc); + ret =q6v5_xfer_mem_ownership (qproc,2 , + qproc->mpss_phys, qproc->mpss_size); +WARN_ON (ret);reset_control_assert (qproc->mss_restart);q6v5_clk_disable (qproc->dev, qproc->active_clks, qproc->active_clk_count);
@@ -1012,6 +1086,7 @@ static int q6v5_probe(struct platform_device *pdev)if (ret) goto free_rproc; + qproc->need_mem_protection = desc->need_mem_protection; ret =q6v5_request_irq (qproc, pdev,"wdog" , q6v5_wdog_interrupt);if (ret <0 ) goto free_rproc;
@@ -1087,6 +1162,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .need_mem_protection = false, }; static const struct rproc_hexagon_res msm8974_mss = {
@@ -1124,6 +1200,7 @@ static int q6v5_remove(struct platform_device *pdev)"mem" ,NULL }, + .need_mem_protection = false, }; static const struct of_device_id q6v5_of_match[] = {
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: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Quoted 5 lines · Avaneesh Kumar Dwivedi
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem.
Overall this looks good now, I have two minor notes that I want you to fix up though.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 294
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, + int image, phys_addr_t addr,
Rather than relying on a static int to keep track of current permissions pass a "int *current_perms", that you update on success.
Add int mba_perm and int mpss_perm to the struct q6v5 and initialize them in probe and just keep the metadata_perm on the stack in q6v5_mpss_init_image.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+ size_t size) +{ +static int current_owner[3 ][1 ] = {{BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )} }; +struct qcom_scm_vmperm next[] = {{0 } };
You don't need to initialize this, and if you just keep it "struct qcom_scm_vmperm next" you can pass it as &next in the qcom_scm_assign_mem() call.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 306
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+int ret; + +if (!qproc->need_mem_protection) +return 0 ; + +if (current_owner[image][0 ] ==BIT (QCOM_SCM_VMID_HLOS )) {
And rather than making this flip back and forth with every call, I think it's more robust if you pass the new expected owner as a parameter to the function. Simplest way I can think of it to add a "bool remote_owner" as a parameter; it makes the changes minimal and works with the naming of the function.
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ next->vmid =QCOM_SCM_VMID_MSS_MSA ; + next->perm =QCOM_SCM_PERM_RW ; + }else { + next->vmid =QCOM_SCM_VMID_HLOS ; + next->perm =QCOM_SCM_PERM_RWX ; + } + + ret =qcom_scm_assign_mem (addr,ALIGN (size,SZ_4K ), + current_owner[image][0 ], next,1 ); +if (ret <0 ) {
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 326
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+pr_err ("Failed to assign %s memory access in range %p to %p ret = %d\n" , + (image ==0 ?"MDT" : image ==1 ?"MBA" :"MPSS" ), + (void *)addr, (void *)(addr + size), ret); +return ret; + } + + current_owner[image][0 ] = ret; +return 0 ; +} +
Regards,
Bjorn
Re: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Hi Bjorn,
Thanks lot many for such a blazing fast response :)
regarding your points.
a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two additional inputs i.e. *next_perm and *next_vmid
and that based on successful return of qcom_scm_assign () they
should be treated as *current_perm and *current_vmid
by caller? if so caller will have to do flipping between MSS and
HLOS, isn't it?
Also code churning will increase this way, and also logging the way
is being handled now may require to change again.
or am i misunderstanding your proposal?
Sorry for inconvenience, but if you could through little more light,
that will help.
-Avaneesh
Quoted 72 lines · Bjorn Andersson
Quoted 5 lines · Avaneesh Kumar Dwivedi
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem.
Overall this looks good now, I have two minor notes that I want you to fix up though.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 294
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, + int image, phys_addr_t addr,Rather than relying on a static int to keep track of current permissions pass a "int
*current_perms", that you update on success.Add int
mba_permand intmpss_permto the struct q6v5 and initialize them in probe and just keep themetadata_permon the stack inq6v5_mpss_init_image.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+ size_t size) +{ +static int current_owner[3 ][1 ] = {{BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )} }; +struct qcom_scm_vmperm next[] = {{0 } };You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmpermnext" you can pass it as &next in theqcom_scm_assign_mem()call.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 306
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+int ret; + +if (!qproc->need_mem_protection) +return 0 ; + +if (current_owner[image][0 ] ==BIT (QCOM_SCM_VMID_HLOS )) {And rather than making this flip back and forth with every call, I think it's more robust if you pass the new expected owner as a parameter to the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works with the naming of the function.Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ next->vmid =QCOM_SCM_VMID_MSS_MSA ; + next->perm =QCOM_SCM_PERM_RW ; + }else { + next->vmid =QCOM_SCM_VMID_HLOS ; + next->perm =QCOM_SCM_PERM_RWX ; + } + + ret =qcom_scm_assign_mem (addr,ALIGN (size,SZ_4K ), + current_owner[image][0 ], next,1 ); +if (ret <0 ) {drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 326
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+pr_err ("Failed to assign %s memory access in range %p to %p ret = %d\n" , + (image ==0 ?"MDT" : image ==1 ?"MBA" :"MPSS" ), + (void *)addr, (void *)(addr + size), ret); +return ret; + } + + current_owner[image][0 ] = ret; +return 0 ; +} +Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
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: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Quoted 9 lines · Avaneesh Kumar (avani)
Hi Bjorn,
Thanks lot many for such a blazing fast response :)
regarding your points.
a- Do you mean caller's of
q6v5_xfer_mem_ownership()should pass two additional inputs i.e.*next_permand*next_vmid
You have two cases; assign to HLOS and assign to MSS, so I imagine that you pass a single indicator of which you want to assign. I.e. rather than looking at what the current state is and flipping you pass the conditional of that if statement as a parameter.
Quoted 3 lines
and that based on successful return of
qcom_scm_assign() they should be
treated as*current_permand*current_vmid
Instead of your index, you take a "int *curr_perms", which you use as the current vmid list and you assign at the end of the function (like you do today).
So to transfer the ownership to the MSS you would make a function call like:
ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true);
mpss_owner would have to be initialize to HLOS before calling this, but will always be holding the current value.
Quoted 3 lines
by caller? if so caller will have to do flipping between MSS and HLOS,
isn't it?
As far as I can see each call to this driver is either a "transfer to MSS" or "transfer to HLOS", so that shouldn't be a problem.
Quoted 5 lines
Also code churning will increase this way, and also logging the way is
being handled now may require to change again.or am i misunderstanding your proposal?
Could be that I'm missing something, if above doesn't make sense please do let me know.
Quoted 3 lines
Sorry for inconvenience, but if you could through little more light, that
will help.
There's no inconvenience, thanks for reaching out for clarifications on my comments.
Regards,
Bjorn
Quoted 81 lines
-Avaneesh
Quoted 71 lines · Bjorn Andersson
Quoted 5 lines · Avaneesh Kumar Dwivedi
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem.
Overall this looks good now, I have two minor notes that I want you to fix up though.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 293
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, + int image, phys_addr_t addr,Rather than relying on a static int to keep track of current permissions pass a "int
*current_perms", that you update on success.Add int
mba_permand intmpss_permto the struct q6v5 and initialize them in probe and just keep themetadata_permon the stack inq6v5_mpss_init_image.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+ size_t size) +{ +static int current_owner[3 ][1 ] = {{BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )} }; +struct qcom_scm_vmperm next[] = {{0 } };You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmpermnext" you can pass it as &next in theqcom_scm_assign_mem()call.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 306
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+int ret; + +if (!qproc->need_mem_protection) +return 0 ; + +if (current_owner[image][0 ] ==BIT (QCOM_SCM_VMID_HLOS )) {And rather than making this flip back and forth with every call, I think it's more robust if you pass the new expected owner as a parameter to the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works with the naming of the function.Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ next->vmid =QCOM_SCM_VMID_MSS_MSA ; + next->perm =QCOM_SCM_PERM_RW ; + }else { + next->vmid =QCOM_SCM_VMID_HLOS ; + next->perm =QCOM_SCM_PERM_RWX ; + } + + ret =qcom_scm_assign_mem (addr,ALIGN (size,SZ_4K ), + current_owner[image][0 ], next,1 ); +if (ret <0 ) {drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 326
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+pr_err ("Failed to assign %s memory access in range %p to %p ret = %d\n" , + (image ==0 ?"MDT" : image ==1 ?"MBA" :"MPSS" ), + (void *)addr, (void *)(addr + size), ret); +return ret; + } + + current_owner[image][0 ] = ret; +return 0 ; +} +Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 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: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Quoted 8 lines · Bjorn Andersson
Quoted 6 lines · Avaneesh Kumar (avani)
Hi Bjorn,
Thanks lot many for such a blazing fast response :)
regarding your points.
Quoted 7 lines
Quoted 3 lines
a- Do you mean caller's of
q6v5_xfer_mem_ownership()should pass two additional inputs i.e.*next_permand*next_vmidYou have two cases; assign to HLOS and assign to MSS, so I imagine that you pass a single indicator of which you want to assign. I.e. rather than looking at what the current state is and flipping you pass the conditional of that if statement as a parameter.
OK
Quoted 8 lines
Quoted 4 lines
and that based on successful return of
qcom_scm_assign() they should be
treated as*current_permand*current_vmidInstead of your index, you take a "int
*curr_perms", which you use as the current vmid list and you assign at the end of the function (like you do today).
Quoted 7 lines
So to transfer the ownership to the MSS you would make a function call like:
ret =
q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true);
mpss_ownerwould have to be initialize to HLOS before calling this, but will always be holding the current value.
i am not finding compelling enough region to carry an input pointer to hold current ownership
specially when i am carrying a boolean flag to check whether next->vmid will be MSS or HLOS
I mean where am i going to use this current owner info in mss rproc driver, i am yet not getting enough reason. while the local array did job of maintaining and flipping the ownership based on info if which image ownership transfer is being called.
Quoted 6 lines
Quoted 4 lines
by caller? if so caller will have to do flipping between MSS and HLOS,
isn't it?As far as I can see each call to this driver is either a "transfer to MSS" or "transfer to HLOS", so that shouldn't be a problem.
Yes this job will be done by bool flag, but again what is then use of carry pointers mpss_owner, mba_owner.
Quoted 8 lines
Quoted 6 lines
Also code churning will increase this way, and also logging the way is
being handled now may require to change again.or am i misunderstanding your proposal?
Could be that I'm missing something, if above doesn't make sense please do let me know.
I can not say it does not make sense, probably something subtle i am missing to see.
Quoted 6 lines
Quoted 4 lines
Sorry for inconvenience, but if you could through little more light, that
will help.There's no inconvenience, thanks for reaching out for clarifications on my comments.
Thanks for such a nice gesture.
i feel that maintaining the local array for ownership switching looks, too raw way of doing something. may be i can improve that, but first i need to get understanding of your vision in suggesting above changes.
Quoted 83 lines
Regards,
BjornQuoted 79 lines
-Avaneesh
Quoted 71 lines · Bjorn Andersson
Quoted 5 lines · Avaneesh Kumar Dwivedi
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem.
Overall this looks good now, I have two minor notes that I want you to fix up though.
drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 293
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +2
-0@@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,return &table; } +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, + int image, phys_addr_t addr,Rather than relying on a static int to keep track of current permissions pass a "int
*current_perms", that you update on success.Add int
mba_permand intmpss_permto the struct q6v5 and initialize them in probe and just keep themetadata_permon the stack inq6v5_mpss_init_image.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 300
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+ size_t size) +{ +static int current_owner[3 ][1 ] = {{BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )}, + {BIT (QCOM_SCM_VMID_HLOS )} }; +struct qcom_scm_vmperm next[] = {{0 } };You don't need to initialize this, and if you just keep it "struct
qcom_scm_vmpermnext" you can pass it as &next in theqcom_scm_assign_mem()call.drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 306
Quoted 6 lines
drivers/remoteproc/qcom_q6v5_pil.c +6
-0+int ret; + +if (!qproc->need_mem_protection) +return 0 ; + +if (current_owner[image][0 ] ==BIT (QCOM_SCM_VMID_HLOS )) {And rather than making this flip back and forth with every call, I think it's more robust if you pass the new expected owner as a parameter to the function. Simplest way I can think of it to add a "bool
remote_owner" as a parameter; it makes the changes minimal and works with the naming of the function.Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+ next->vmid =QCOM_SCM_VMID_MSS_MSA ; + next->perm =QCOM_SCM_PERM_RW ; + }else { + next->vmid =QCOM_SCM_VMID_HLOS ; + next->perm =QCOM_SCM_PERM_RWX ; + } + + ret =qcom_scm_assign_mem (addr,ALIGN (size,SZ_4K ), + current_owner[image][0 ], next,1 ); +if (ret <0 ) {drivers/remoteproc/qcom_q6v5_pil.c static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, · line 326
Quoted 10 lines
drivers/remoteproc/qcom_q6v5_pil.c +10
-0+pr_err ("Failed to assign %s memory access in range %p to %p ret = %d\n" , + (image ==0 ?"MDT" : image ==1 ?"MBA" :"MPSS" ), + (void *)addr, (void *)(addr + size), ret); +return ret; + } + + current_owner[image][0 ] = ret; +return 0 ; +} +Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 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.
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: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Quoted 34 lines · Avaneesh Kumar (avani)
Quoted 8 lines · Bjorn Andersson
Quoted 6 lines · Avaneesh Kumar (avani)
Hi Bjorn,
Thanks lot many for such a blazing fast response :)
regarding your points.
Quoted 7 lines
Quoted 3 lines
a- Do you mean caller's of
q6v5_xfer_mem_ownership()should pass two additional inputs i.e.*next_permand*next_vmidYou have two cases; assign to HLOS and assign to MSS, so I imagine that you pass a single indicator of which you want to assign. I.e. rather than looking at what the current state is and flipping you pass the conditional of that if statement as a parameter.
OK
Quoted 8 lines
Quoted 4 lines
and that based on successful return of
qcom_scm_assign() they should be
treated as*current_permand*current_vmidInstead of your index, you take a "int
*curr_perms", which you use as the current vmid list and you assign at the end of the function (like you do today).Quoted 7 lines
So to transfer the ownership to the MSS you would make a function call like:
ret =
q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true);
mpss_ownerwould have to be initialize to HLOS before calling this, but will always be holding the current value.
Quoted 9 lines
i am not finding compelling enough region to carry an input pointer to hold current ownership
specially when i am carrying a boolean flag to check whethernext->vmidwill be MSS or HLOS
I mean where am i going to use this current owner info in mss rproc driver, i am yet not getting enough reason.
while the local array did job of maintaining and flipping the ownership based on info if which image ownership transfer is being called.
As far as I can see your patch works fine, every code path will end up calling xfer_mem() an even number of times, meaning that when we're done the ownership is on the HLOS side.
But the reason I don't like this flip-flop mechanism is that it forces us to _always_ exit every code path with an even number of calls. Meaning that if we ever refactor any of this code and accidentally add another flip, we will start seeing "random" crashes. This is the reason why I want the code to be explicit in "transfer permission to X".
The reason for not using the "destination owner" for figuring out the current owner is that in the even that you call "transfer permission to HLOS" twice in a row, you will call TZ saying that the current ownership is MSS and the call will fail. In this case the calling code has no chance to know if we failed because we have called xfer_mem() an odd number of times or something else and although we are good (HLOS is owner) we have to treat this as a fatal error.
So, it's all about future maintainability - not about it currently working.
Regards,
Bjorn
PATCH v5 4/4 remoteproc: qcom: Add support for mss remoteproc on msm8996
In reply to Avaneesh Kumar DwivediThis patch add support for mss boot on msm8996. Major changes include initializing mss rproc for msm8996, making appropriate change for executing mss reset sequence etc.
--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 163 ++++++++++++++++++--- 2 files changed, 140 insertions(+), 24 deletions(-)
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1 -0
@@ -11,6 +11,7 @@ on the Qualcomm Hexagon core. "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 +139 -24
238 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: [PATCH v5 4/4] remoteproc: qcom: Add support for mss remoteproc on msm8996
Quoted 15 lines · Avaneesh Kumar Dwivedi
This patch add support for mss boot on msm8996. Major changes include initializing mss rproc for msm8996, making appropriate change for executing mss reset sequence etc.
Signed-off-by Avaneesh Kumar Dwivedi <[email protected]>--- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 163 ++++++++++++++++++--- 2 files changed, 140 insertions(+), 24 deletions(-)Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0
-0
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 12
Quoted 2 lines
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +0
-0@@ -11,6 +11,7 @@ on the Qualcomm Hexagon core. "qcom,msm8916-mss-pil", "qcom,msm8974-mss-pil"
This blank line should be moved one step down.
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt on the Qualcomm Hexagon core. · line 17
Quoted 4 lines
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +1
-0+ "qcom,msm8996-mss-pil" - reg: Usage: required Value type: <prop-encoded-array>
Apart from that I think this looks good.
Regards,
Bjorn