| Message ID | 1488996202-1546-2-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 03/08, Avaneesh Kumar Dwivedi wrote: > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c > index 4a0f5ea..187fc00 100644 > --- a/drivers/firmware/qcom_scm-64.c > +++ b/drivers/firmware/qcom_scm-64.c > @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) > > return ret ? : res.a1; > } > + > +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) Why are we passing a structure copy? > +{ > + int ret; > + struct qcom_scm_desc desc = {0}; > + struct arm_smccc_res res; > + > + desc.args[0] = vmid.fw_phy; > + desc.args[1] = vmid.size_fw; > + desc.args[2] = vmid.from_phy; > + desc.args[3] = vmid.size_from; > + desc.args[4] = vmid.to_phy; > + desc.args[5] = vmid.size_to; These should all cause sparse warnings because of incorrect assignments. Given that these are all registers, I'm not sure why the vmid_detail structure has __le32 in it. > + 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; > +} > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 893f953ea..f137f34 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -42,6 +42,18 @@ struct qcom_scm { > > static struct qcom_scm *__scm; > > +struct dest_vm_and_perm_info { > + __le32 vm; > + __le32 perm; > + __le32 *ctx; Drop the pointer? Just treat it like another number? Pointer is really odd because it doesn't really make any sense what the address of the pointer would be. > + __le32 ctx_size; > +}; > + > +struct fw_region_info { > + __le64 addr; > + __le64 size; > +}; > + > static int qcom_scm_clk_enable(void) > { > int ret; > @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) > } > EXPORT_SYMBOL(qcom_scm_pas_shutdown); > > +/** > + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old > + * new owners of memory region for fw and metadata etc, Which is > + * further passed to hypervisor, which does translate intermediate > + * physical address used by subsystems. Maybe this can be the long description, but the short description should be shorter. > + * @vmid: structure with pointers and size detail of old and new > + * owners vmid detail. > + * Return 0 on success. There's a standard syntax for return too. Look at kernel doc howto please. > + */ > +int qcom_scm_assign_mem(struct vmid_detail vmid) Please no structure copy. > +{ > + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; > + struct dest_vm_and_perm_info *to; > + struct fw_region_info *fw_info; > + __le64 fw_phy; > + __le32 *from; > + int ret = -ENOMEM; Not sure why we assign it. It gets overwritten with the first use. > + int i; > + > + from = dma_alloc_attrs(__scm->dev, vmid.size_from, > + &vmid.from_phy, GFP_KERNEL, dma_attrs); > + if (!from) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass source vmid detail\n"); > + return -ENOMEM; > + } > + to = dma_alloc_attrs(__scm->dev, vmid.size_to, > + &vmid.to_phy, GFP_KERNEL, dma_attrs); > + if (!to) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass destination vmid detail\n"); > + goto free_src_buff; > + } > + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info), > + &fw_phy, GFP_KERNEL, dma_attrs); Can we consolidate this into one allocation with the appropriate size and then offset the different structures in it? > + if (!fw_info) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass firmware detail\n"); > + goto free_dest_buff; > + } > + > + /* copy detail of original owner of ddr region */ > + /* in physically contigious buffer */ > + memcpy(from, vmid.from, vmid.size_from); > + > + /* fill details of new owners of ddr region*/ > + /* in physically contigious buffer */ > + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { > + to[i].vm = vmid.to[i]; > + to[i].perm = vmid.permission[i]; Here you want the cpu_to_le32() stuff. Please run sparse. > + to[i].ctx = NULL; > + to[i].ctx_size = 0; > + } > + > + /* copy detail of firmware region whose mapping need to be done */ > + /* in physically contigious buffer */ /* * Multi-line comments are like so. */ > + fw_info->addr = vmid.fw_phy; > + fw_info->size = vmid.size_fw; > + > + /* reuse fw_phy and size_fw members to fill address and size of */ > + /* fw_info buffer */ > + vmid.fw_phy = fw_phy; > + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32)); > + vmid.size_fw = sizeof(*fw_info); > + ret = __qcom_scm_assign_mem(__scm->dev, vmid); > + if (!ret) > + goto free_fw_buff; > + return ret; We don't free dma on failure? > +free_fw_buff: > + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info, > + fw_phy, dma_attrs); > +free_dest_buff: > + dma_free_attrs(__scm->dev, vmid.size_to, to, > + vmid.to_phy, dma_attrs); > +free_src_buff: > + dma_free_attrs(__scm->dev, vmid.size_from, from, > + vmid.from_phy, dma_attrs); > + return ret; > +} > +EXPORT_SYMBOL(qcom_scm_assign_mem); > + > static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev, > unsigned long idx) > { > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > index 3584b00..4665a11 100644 > --- a/drivers/firmware/qcom_scm.h > +++ b/drivers/firmware/qcom_scm.h > @@ -55,6 +55,9 @@ 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); > +#define QCOM_SCM_SVC_MP 0xc This is already defined upstream? > +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 > +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid); > > /* common error codes */ > #define QCOM_SCM_V2_EBUSY -12 > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > index 8fd697a..62ad976 100644 > --- a/drivers/remoteproc/qcom_q6v5_pil.c > +++ b/drivers/remoteproc/qcom_q6v5_pil.c > @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, > return &table; > } > > +int hyp_mem_access(int id, phys_addr_t addr, size_t size) static? > > static const struct of_device_id q6v5_of_match[] = { > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h > index cc32ab8..cb0b7ee 100644 > --- a/include/linux/qcom_scm.h > +++ b/include/linux/qcom_scm.h > @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req { > u32 val; > }; > > +struct vmid_detail { > + __le32 *from; > + __le32 *to; > + __le32 *permission; > + __le32 size_from; > + __le32 size_to; > + __le32 size_fw; > + __le64 fw_phy; > + __le64 from_phy; > + __le64 to_phy; should the *_phy be phys_addr_t types? Leave these all as u32/u64. Perhaps also move size_from/size_to next to the arrays they're for. Also add some documentation so we know what they're all about.
On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote: > This patch add scm call support to make hypervisor call to enable access > of fw regions in ddr to mss subsystem on arm-v8 arch soc's. > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > --- > drivers/firmware/qcom_scm-64.c | 25 +++++++ > drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++ > drivers/firmware/qcom_scm.h | 3 + > drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++- > include/linux/qcom_scm.h | 14 ++++ > 5 files changed, 262 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c > index 4a0f5ea..187fc00 100644 > --- a/drivers/firmware/qcom_scm-64.c > +++ b/drivers/firmware/qcom_scm-64.c > @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) > > return ret ? : res.a1; > } > + > +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) Rather than packing these parameters up in a struct I think it's cleaner to just pass them directly. > +{ > + int ret; > + struct qcom_scm_desc desc = {0}; > + struct arm_smccc_res res; > + > + desc.args[0] = vmid.fw_phy; > + desc.args[1] = vmid.size_fw; > + desc.args[2] = vmid.from_phy; > + desc.args[3] = vmid.size_from; > + desc.args[4] = vmid.to_phy; > + desc.args[5] = vmid.size_to; > + 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; If I understand the downstream code we only care about "ret" here; being zero on success or negative on error. > +} > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 893f953ea..f137f34 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -42,6 +42,18 @@ struct qcom_scm { > > static struct qcom_scm *__scm; > > +struct dest_vm_and_perm_info { > + __le32 vm; > + __le32 perm; > + __le32 *ctx; Please be explicit about the fact that this is 64 bit. > + __le32 ctx_size; > +}; This should be __packed > + > +struct fw_region_info { > + __le64 addr; > + __le64 size; > +}; > + > static int qcom_scm_clk_enable(void) > { > int ret; > @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) > } > EXPORT_SYMBOL(qcom_scm_pas_shutdown); > > +/** > + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old > + * new owners of memory region for fw and metadata etc, Which is > + * further passed to hypervisor, which does translate intermediate > + * physical address used by subsystems. > + * @vmid: structure with pointers and size detail of old and new > + * owners vmid detail. > + * Return 0 on success. > + */ > +int qcom_scm_assign_mem(struct vmid_detail vmid) After a long discussion with Stephen I now think that I understand what's actually going on here. So this function will request TZ to remove all permissions for the memory region in the tables specified in "from" and then for each vmid in "to" it will set up the specified "permission". So the "to" and "permissions" are actually a tuple, rather than independent lists of values. So I think this should be exposed in the prototype, as a list of <vmid, permission> entries. To make the function prototype more convenient I think you should turn "from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)). If you then make the function, on success, return "to" as a bitmap the caller can simply store that in a state variable and pass it as "from" in the next call. So you would have: struct qcom_scm_mem_perm new_perms[] = { { VMID_HLOS, PERM_READ }, { VMID_MSS_MSA, PREM_READ | PERM_WRITE }, }; current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2); And I believe something like "curr_perm" and "new_perm" are even better names than "from" and "to" > +{ > + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; > + struct dest_vm_and_perm_info *to; > + struct fw_region_info *fw_info; > + __le64 fw_phy; > + __le32 *from; > + int ret = -ENOMEM; > + int i; > + > + from = dma_alloc_attrs(__scm->dev, vmid.size_from, > + &vmid.from_phy, GFP_KERNEL, dma_attrs); > + if (!from) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass source vmid detail\n"); > + return -ENOMEM; > + } > + to = dma_alloc_attrs(__scm->dev, vmid.size_to, > + &vmid.to_phy, GFP_KERNEL, dma_attrs); > + if (!to) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass destination vmid detail\n"); > + goto free_src_buff; > + } > + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info), > + &fw_phy, GFP_KERNEL, dma_attrs); > + if (!fw_info) { > + dev_err(__scm->dev, > + "failed to allocate buffer to pass firmware detail\n"); > + goto free_dest_buff; > + } You should be able to allocate these in one chunk and you should be able to just use dma_alloc_coherent(). > + > + /* copy detail of original owner of ddr region */ > + /* in physically contigious buffer */ > + memcpy(from, vmid.from, vmid.size_from); With "from" as a bitmap see hweight_long() to figure out the size of "from". > + > + /* fill details of new owners of ddr region*/ > + /* in physically contigious buffer */ > + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { Better pass "number of entries in 'to'" and use standard types (such as int or unsigned int) for "to" and "permission" until this point. > + to[i].vm = vmid.to[i]; > + to[i].perm = vmid.permission[i]; > + to[i].ctx = NULL; > + to[i].ctx_size = 0; > + } > + > + /* copy detail of firmware region whose mapping need to be done */ > + /* in physically contigious buffer */ > + fw_info->addr = vmid.fw_phy; > + fw_info->size = vmid.size_fw; fw_info is a confusing name for "a list of memory regions". And you need a cpu_to_le32() somewhere, better keep the in-kernel API pass standard types (such as phys_addr_t) and then convert to the "hardware specific" type here. > + > + /* reuse fw_phy and size_fw members to fill address and size of */ > + /* fw_info buffer */ Please don't try to be "clever" and reuse things when writing kernel-code, non-obvious reuse of variables or struct members often end up causing someone else to introduce bugs in your code later. > + vmid.fw_phy = fw_phy; > + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32)); > + vmid.size_fw = sizeof(*fw_info); > + ret = __qcom_scm_assign_mem(__scm->dev, vmid); > + if (!ret) > + goto free_fw_buff; > + return ret; > +free_fw_buff: > + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info, > + fw_phy, dma_attrs); > +free_dest_buff: > + dma_free_attrs(__scm->dev, vmid.size_to, to, > + vmid.to_phy, dma_attrs); > +free_src_buff: > + dma_free_attrs(__scm->dev, vmid.size_from, from, > + vmid.from_phy, dma_attrs); > + return ret; > +} > +EXPORT_SYMBOL(qcom_scm_assign_mem); > + > static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev, > unsigned long idx) > { > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > index 3584b00..4665a11 100644 > --- a/drivers/firmware/qcom_scm.h > +++ b/drivers/firmware/qcom_scm.h > @@ -55,6 +55,9 @@ 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); > +#define QCOM_SCM_SVC_MP 0xc > +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 > +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid); > > /* common error codes */ > #define QCOM_SCM_V2_EBUSY -12 > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > index 8fd697a..62ad976 100644 > --- a/drivers/remoteproc/qcom_q6v5_pil.c > +++ b/drivers/remoteproc/qcom_q6v5_pil.c Please split the patch in a scm-patch and a q6v5 patch. > @@ -110,6 +110,7 @@ struct rproc_hexagon_res { > struct qcom_mss_reg_res *active_supply; > char **proxy_clk_names; > char **active_clk_names; > + int version; This will result in multi-line conditionals checking if we should do memory protection for platform x, y, z or not. Add a "bool need_mem_protect;" instead. > }; > > struct q6v5 { > @@ -153,8 +154,28 @@ struct q6v5 { > size_t mpss_size; > > struct qcom_rproc_subdev smd_subdev; > + int version; > }; > > +enum { > + MSS_MSM8916, > + MSS_MSM8974, > + MSS_MSM8996, > +}; > + > +enum { > + ASSIGN_ACCESS_MSA, > + REMOVE_ACCESS_MSA, > + ASSIGN_SHARED_ACCESS_MSA, > + REMOVE_SHARED_ACCESS_MSA, > +}; > + > +#define VMID_HLOS 0x3 > +#define VMID_MSS_MSA 0xF > +#define PERM_READ 0x4 > +#define PERM_WRITE 0x2 > +#define PERM_EXEC 0x1 > +#define PERM_RW (PERM_READ | PERM_WRITE) These belongs in the SCM API instead. (Prefix them appropriately) > static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, > const struct qcom_mss_reg_res *reg_res) > { > @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, > return &table; > } > > +int hyp_mem_access(int id, phys_addr_t addr, size_t size) I much prefer the downstream split and naming of this function, rather than using a switch to implement 4 different functions in one please split it up. > +{ > + struct vmid_detail vmid; > + int ret; > + Pass the struct q6v5 here and return successfully here if we're not supposed to do memory protection. It does clean up the calling code. > + switch (id) { > + case ASSIGN_ACCESS_MSA: > + vmid.from = (int[]) { VMID_HLOS }; > + vmid.to = (int[]) { VMID_MSS_MSA }; > + vmid.permission = (int[]) { PERM_READ | PERM_WRITE }; > + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); > + break; > + case REMOVE_ACCESS_MSA: > + vmid.from = (int[]) { VMID_MSS_MSA }; > + vmid.to = (int[]) { VMID_HLOS }; > + vmid.permission = > + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; > + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); > + break; > + case ASSIGN_SHARED_ACCESS_MSA: > + vmid.from = (int[]) { VMID_HLOS }; > + vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA }; > + vmid.permission = (int[]) { PERM_RW, PERM_RW }; > + vmid.size_from = 1 * sizeof(__le32); > + vmid.size_to = 2 * sizeof(__le32); > + break; > + case REMOVE_SHARED_ACCESS_MSA: > + vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA }; > + vmid.to = (int[]) { VMID_HLOS }; > + vmid.permission = > + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; > + vmid.size_from = 2 * sizeof(__le32); > + vmid.size_to = 1 * sizeof(__le32); > + break; > + default: > + break; > + } > + > + vmid.fw_phy = addr; > + vmid.size_fw = size; > + ret = qcom_scm_assign_mem(vmid); > + if (ret) > + pr_err("%s: Failed to assign memory protection, ret = %d\n", > + __func__, ret); > + > + return ret; > +} > + > static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > { > struct q6v5 *qproc = rproc->priv; > @@ -461,6 +530,15 @@ 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 = qproc->version == MSS_MSM8996 ? To prove the point above, imagine just in the next year when this becomes: ret = (qproc->version == MSS_MSM8996 || qproc->version == MSS_MSM8998 || qproc->version == MSS_NEXT_THING) ? hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) : 0; Or in 5 years... > + hyp_mem_access(ASSIGN_ACCESS_MSA, phys, > + ALIGN(fw->size, SZ_4K)) : 0; > + if (ret) { > + dev_err(qproc->dev, > + "Failed to assign metadata memory, ret - %d\n", ret); Rather than having this print after each call to hyp_mem_access() move the message into that function. > + return -ENOMEM; And don't forget to clean up the allocation before returning. > + } > 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 +548,13 @@ 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 */ > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(REMOVE_ACCESS_MSA, phys, > + ALIGN(fw->size, SZ_4K)) : 0; You use ALIGN(fw->size, SZ_4K) numerous times in this function, use a variable and make the allocation take this as well to make it explicit that we're dealing with the same memory chunk size all over. > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim metadata memory, ret - %d\n", ret); > dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); > > return ret < 0 ? ret : 0; > @@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > if (!size) { > boot_addr = relocate ? qproc->mpss_phys : min_addr; > + /* Hypervisor mapping of modem fw */ > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA, > + boot_addr, qproc->mpss_size) : 0; The mpss is loaded somewhere in qproc->mpss_region, potentially at some offset (which would be reflected in boot_addr). But if boot_addr > qproc->mpss_region the region to hand out is no longer mpss_size large, it's "mpss_size - (boot_addr - mpss_phys)", so you might give the mpss permission to trash the memory after the mpss_region. Better hand out qproc->mpss_phys of size qproc->mpss_size. And as far as I can tell the downstream driver load the segments and then make the MSS sole owner of the memory region. Do note that it's perfectly fine to refactor code to make things better fit a natural and easy to follow flow of ownership here! > + if (ret) { > + dev_err(qproc->dev, > + "Failed to assign fw memory access, ret - %d\n", > + ret); > + return -ENOMEM; > + } > writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); > writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); > } > @@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc) > goto assert_reset; > } > > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys, > + qproc->mba_size) : 0; > + if (ret) { > + dev_err(qproc->dev, > + "Failed to assign mba memory access, ret - %d\n", ret); > + goto assert_reset; > + } > writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); > > ret = q6v5proc_reset(qproc); > @@ -657,16 +760,22 @@ 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; > } > > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, > + qproc->mba_size) : 0; > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim mba memory, ret - %d\n", ret); > qproc->running = true; > > q6v5_clk_disable(qproc->dev, qproc->proxy_clks, > @@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc) > On success q6v5_start() will leave Linus as sole owner of mba_phys and mpss_phys (or boot_addr) will have shared ownership. rproc_stop() should make sure to reclaim the sole ownership of mpss_phys again, so that subsequent calls to rproc_start() will find the memory protection in an appropriate state. > return 0; > > +reclaim_mem: > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(REMOVE_SHARED_ACCESS_MSA, > + qproc->mpss_phys, qproc->mpss_size) : 0; > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim fw memory, ret - %d\n", ret); > halt_axi_ports: > + ret = qproc->version == MSS_MSM8996 ? > + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, > + qproc->mba_size) : 0; > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim mba memory, ret - %d\n", ret); Isn't it possible that the remote processor is still executing the MBA firmware up until we halt the axi ports and assert the reset? We don't want to remove permission until we know the CPU is stopped and it wont fault on us. > 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); > @@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev) > if (ret) > goto free_rproc; > > + qproc->version = desc->version; > ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); > if (ret < 0) > goto free_rproc; > @@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev) > "mem", > NULL > }, > + .version = MSS_MSM8916, > }; > > static const struct rproc_hexagon_res msm8974_mss = { > @@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev) > "mem", > NULL > }, > + .version = MSS_MSM8974, > }; > > static const struct of_device_id q6v5_of_match[] = { > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h > index cc32ab8..cb0b7ee 100644 > --- a/include/linux/qcom_scm.h > +++ b/include/linux/qcom_scm.h > @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req { > u32 val; > }; > > +struct vmid_detail { Whenever you add structs to a public API, make sure to add some kerneldoc. And as this is a kernel-API I would like to see standard kernel-types for things. > + __le32 *from; > + __le32 *to; > + __le32 *permission; > + __le32 size_from; > + __le32 size_to; > + __le32 size_fw; > + __le64 fw_phy; > + __le64 from_phy; > + __le64 to_phy; > + > +}; > + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn/Boyd, thanks for comments and suggestion, beg apology for delayed reply as i got busy with internal issues. please see inline response. On 4/6/2017 10:13 AM, Bjorn Andersson wrote: > On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote: > >> This patch add scm call support to make hypervisor call to enable access >> of fw regions in ddr to mss subsystem on arm-v8 arch soc's. >> >> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> >> --- >> drivers/firmware/qcom_scm-64.c | 25 +++++++ >> drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++ >> drivers/firmware/qcom_scm.h | 3 + >> drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++- >> include/linux/qcom_scm.h | 14 ++++ >> 5 files changed, 262 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c >> index 4a0f5ea..187fc00 100644 >> --- a/drivers/firmware/qcom_scm-64.c >> +++ b/drivers/firmware/qcom_scm-64.c >> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) >> >> return ret ? : res.a1; >> } >> + >> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) > Rather than packing these parameters up in a struct I think it's cleaner > to just pass them directly. passing so many parameters directly is not seen good practice specially when number of parameters to pass are many. that is why i used struct copy, i did not use reference of struct since values passed were not going to be modified. but will surely look if i can work with direct passing of params. > >> +{ >> + int ret; >> + struct qcom_scm_desc desc = {0}; >> + struct arm_smccc_res res; >> + >> + desc.args[0] = vmid.fw_phy; >> + desc.args[1] = vmid.size_fw; >> + desc.args[2] = vmid.from_phy; >> + desc.args[3] = vmid.size_from; >> + desc.args[4] = vmid.to_phy; >> + desc.args[5] = vmid.size_to; >> + 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; > If I understand the downstream code we only care about "ret" here; being > zero on success or negative on error. sure, will check. >> +} >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 893f953ea..f137f34 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -42,6 +42,18 @@ struct qcom_scm { >> >> static struct qcom_scm *__scm; >> >> +struct dest_vm_and_perm_info { >> + __le32 vm; >> + __le32 perm; >> + __le32 *ctx; > Please be explicit about the fact that this is 64 bit. sorry i am not sure why they need to be 64 bit variable? > >> + __le32 ctx_size; >> +}; > This should be __packed Ok. > >> + >> +struct fw_region_info { >> + __le64 addr; >> + __le64 size; >> +}; >> + >> static int qcom_scm_clk_enable(void) >> { >> int ret; >> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) >> } >> EXPORT_SYMBOL(qcom_scm_pas_shutdown); >> >> +/** >> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old >> + * new owners of memory region for fw and metadata etc, Which is >> + * further passed to hypervisor, which does translate intermediate >> + * physical address used by subsystems. >> + * @vmid: structure with pointers and size detail of old and new >> + * owners vmid detail. >> + * Return 0 on success. >> + */ >> +int qcom_scm_assign_mem(struct vmid_detail vmid) > After a long discussion with Stephen I now think that I understand > what's actually going on here. > > So this function will request TZ to remove all permissions for the > memory region in the tables specified in "from" and then for each vmid > in "to" it will set up the specified "permission". > > So the "to" and "permissions" are actually a tuple, rather than > independent lists of values. So I think this should be exposed in the > prototype, as a list of <vmid, permission> entries. > > To make the function prototype more convenient I think you should turn > "from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)). i am not able to fully comprehend advantage of turning "from" into bitmap. moreover when i read your below suggestion, which suggest to use "struct qcom_scm_mem_perm" as below then i get further confused what i will do with bitmap? > > If you then make the function, on success, return "to" as a bitmap the > caller can simply store that in a state variable and pass it as "from" > in the next call. > > So you would have: > > struct qcom_scm_mem_perm new_perms[] = { > { VMID_HLOS, PERM_READ }, > { VMID_MSS_MSA, PREM_READ | PERM_WRITE }, > }; > > current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2); This looks OK, will try to incorporate this idea. > > > And I believe something like "curr_perm" and "new_perm" are even better > names than "from" and "to" ok. > >> +{ >> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; >> + struct dest_vm_and_perm_info *to; >> + struct fw_region_info *fw_info; >> + __le64 fw_phy; >> + __le32 *from; >> + int ret = -ENOMEM; >> + int i; >> + >> + from = dma_alloc_attrs(__scm->dev, vmid.size_from, >> + &vmid.from_phy, GFP_KERNEL, dma_attrs); >> + if (!from) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass source vmid detail\n"); >> + return -ENOMEM; >> + } >> + to = dma_alloc_attrs(__scm->dev, vmid.size_to, >> + &vmid.to_phy, GFP_KERNEL, dma_attrs); >> + if (!to) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass destination vmid detail\n"); >> + goto free_src_buff; >> + } >> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info), >> + &fw_phy, GFP_KERNEL, dma_attrs); >> + if (!fw_info) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass firmware detail\n"); >> + goto free_dest_buff; >> + } > You should be able to allocate these in one chunk and you should be able > to just use dma_alloc_coherent(). OK. > >> + >> + /* copy detail of original owner of ddr region */ >> + /* in physically contigious buffer */ >> + memcpy(from, vmid.from, vmid.size_from); > With "from" as a bitmap see hweight_long() to figure out the size of > "from". hweight_long() returns number of bits set in bitmap? is this correct understanding? > >> + >> + /* fill details of new owners of ddr region*/ >> + /* in physically contigious buffer */ >> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { > Better pass "number of entries in 'to'" and use standard types (such as > int or unsigned int) for "to" and "permission" until this point. OK. > >> + to[i].vm = vmid.to[i]; >> + to[i].perm = vmid.permission[i]; >> + to[i].ctx = NULL; >> + to[i].ctx_size = 0; >> + } >> + >> + /* copy detail of firmware region whose mapping need to be done */ >> + /* in physically contigious buffer */ >> + fw_info->addr = vmid.fw_phy; >> + fw_info->size = vmid.size_fw; > fw_info is a confusing name for "a list of memory regions". ok will try to use appropriate name. > > And you need a cpu_to_le32() somewhere, better keep the in-kernel API > pass standard types (such as phys_addr_t) and then convert to the > "hardware specific" type here. ok. > >> + >> + /* reuse fw_phy and size_fw members to fill address and size of */ >> + /* fw_info buffer */ > Please don't try to be "clever" and reuse things when writing > kernel-code, non-obvious reuse of variables or struct members often end > up causing someone else to introduce bugs in your code later. ok. > >> + vmid.fw_phy = fw_phy; >> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32)); >> + vmid.size_fw = sizeof(*fw_info); >> + ret = __qcom_scm_assign_mem(__scm->dev, vmid); >> + if (!ret) >> + goto free_fw_buff; >> + return ret; >> +free_fw_buff: >> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info, >> + fw_phy, dma_attrs); >> +free_dest_buff: >> + dma_free_attrs(__scm->dev, vmid.size_to, to, >> + vmid.to_phy, dma_attrs); >> +free_src_buff: >> + dma_free_attrs(__scm->dev, vmid.size_from, from, >> + vmid.from_phy, dma_attrs); >> + return ret; >> +} >> +EXPORT_SYMBOL(qcom_scm_assign_mem); >> + >> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev, >> unsigned long idx) >> { >> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h >> index 3584b00..4665a11 100644 >> --- a/drivers/firmware/qcom_scm.h >> +++ b/drivers/firmware/qcom_scm.h >> @@ -55,6 +55,9 @@ 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); >> +#define QCOM_SCM_SVC_MP 0xc >> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 >> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid); >> >> /* common error codes */ >> #define QCOM_SCM_V2_EBUSY -12 >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index 8fd697a..62ad976 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c > Please split the patch in a scm-patch and a q6v5 patch. ok. > >> @@ -110,6 +110,7 @@ struct rproc_hexagon_res { >> struct qcom_mss_reg_res *active_supply; >> char **proxy_clk_names; >> char **active_clk_names; >> + int version; > This will result in multi-line conditionals checking if we should do > memory protection for platform x, y, z or not. Add a "bool > need_mem_protect;" instead. ok. > >> }; >> >> struct q6v5 { >> @@ -153,8 +154,28 @@ struct q6v5 { >> size_t mpss_size; >> >> struct qcom_rproc_subdev smd_subdev; >> + int version; >> }; >> >> +enum { >> + MSS_MSM8916, >> + MSS_MSM8974, >> + MSS_MSM8996, >> +}; >> + >> +enum { >> + ASSIGN_ACCESS_MSA, >> + REMOVE_ACCESS_MSA, >> + ASSIGN_SHARED_ACCESS_MSA, >> + REMOVE_SHARED_ACCESS_MSA, >> +}; >> + >> +#define VMID_HLOS 0x3 >> +#define VMID_MSS_MSA 0xF >> +#define PERM_READ 0x4 >> +#define PERM_WRITE 0x2 >> +#define PERM_EXEC 0x1 >> +#define PERM_RW (PERM_READ | PERM_WRITE) > These belongs in the SCM API instead. (Prefix them appropriately) Do you mean something like SCM_VMID_HLOS? > >> static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, >> const struct qcom_mss_reg_res *reg_res) >> { >> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, >> return &table; >> } >> >> +int hyp_mem_access(int id, phys_addr_t addr, size_t size) > I much prefer the downstream split and naming of this function, rather > than using a switch to implement 4 different functions in one please > split it up. so you mean i need to have seprate function for each "int id"? in switch case? just for sake of minimum code churn i tried to work with single function. > >> +{ >> + struct vmid_detail vmid; >> + int ret; >> + > Pass the struct q6v5 here and return successfully here if we're not > supposed to do memory protection. It does clean up the calling code. OK. > >> + switch (id) { >> + case ASSIGN_ACCESS_MSA: >> + vmid.from = (int[]) { VMID_HLOS }; >> + vmid.to = (int[]) { VMID_MSS_MSA }; >> + vmid.permission = (int[]) { PERM_READ | PERM_WRITE }; >> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); >> + break; >> + case REMOVE_ACCESS_MSA: >> + vmid.from = (int[]) { VMID_MSS_MSA }; >> + vmid.to = (int[]) { VMID_HLOS }; >> + vmid.permission = >> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; >> + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); >> + break; >> + case ASSIGN_SHARED_ACCESS_MSA: >> + vmid.from = (int[]) { VMID_HLOS }; >> + vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA }; >> + vmid.permission = (int[]) { PERM_RW, PERM_RW }; >> + vmid.size_from = 1 * sizeof(__le32); >> + vmid.size_to = 2 * sizeof(__le32); >> + break; >> + case REMOVE_SHARED_ACCESS_MSA: >> + vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA }; >> + vmid.to = (int[]) { VMID_HLOS }; >> + vmid.permission = >> + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; >> + vmid.size_from = 2 * sizeof(__le32); >> + vmid.size_to = 1 * sizeof(__le32); >> + break; >> + default: >> + break; >> + } >> + >> + vmid.fw_phy = addr; >> + vmid.size_fw = size; >> + ret = qcom_scm_assign_mem(vmid); >> + if (ret) >> + pr_err("%s: Failed to assign memory protection, ret = %d\n", >> + __func__, ret); >> + >> + return ret; >> +} >> + >> static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> { >> struct q6v5 *qproc = rproc->priv; >> @@ -461,6 +530,15 @@ 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 = qproc->version == MSS_MSM8996 ? > To prove the point above, imagine just in the next year when this I agree. > becomes: > > ret = (qproc->version == MSS_MSM8996 || > qproc->version == MSS_MSM8998 || > qproc->version == MSS_NEXT_THING) ? > hyp_mem_access(ASSIGN_ACCESS_MSA, phys, ALIGN(fw->size, SZ_4K)) : > 0; > > Or in 5 years... > >> + hyp_mem_access(ASSIGN_ACCESS_MSA, phys, >> + ALIGN(fw->size, SZ_4K)) : 0; >> + if (ret) { >> + dev_err(qproc->dev, >> + "Failed to assign metadata memory, ret - %d\n", ret); > Rather than having this print after each call to hyp_mem_access() move > the message into that function. Ok. > >> + return -ENOMEM; > And don't forget to clean up the allocation before returning. Ok. > >> + } >> 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 +548,13 @@ 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 */ >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(REMOVE_ACCESS_MSA, phys, >> + ALIGN(fw->size, SZ_4K)) : 0; > You use ALIGN(fw->size, SZ_4K) numerous times in this function, use a > variable and make the allocation take this as well to make it explicit > that we're dealing with the same memory chunk size all over. OK. > >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim metadata memory, ret - %d\n", ret); >> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); >> >> return ret < 0 ? ret : 0; >> @@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc) >> size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); >> if (!size) { >> boot_addr = relocate ? qproc->mpss_phys : min_addr; >> + /* Hypervisor mapping of modem fw */ >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA, >> + boot_addr, qproc->mpss_size) : 0; > The mpss is loaded somewhere in qproc->mpss_region, potentially at some > offset (which would be reflected in boot_addr). > > But if boot_addr > qproc->mpss_region the region to hand out is no > longer mpss_size large, it's "mpss_size - (boot_addr - mpss_phys)", so > you might give the mpss permission to trash the memory after the > mpss_region. > > Better hand out qproc->mpss_phys of size qproc->mpss_size. OK. > > > And as far as I can tell the downstream driver load the segments and > then make the MSS sole owner of the memory region. Do note that it's > perfectly fine to refactor code to make things better fit a natural and > easy to follow flow of ownership here! Not very clear what to do? > >> + if (ret) { >> + dev_err(qproc->dev, >> + "Failed to assign fw memory access, ret - %d\n", >> + ret); >> + return -ENOMEM; >> + } >> writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); >> writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); >> } >> @@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc) >> goto assert_reset; >> } >> >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys, >> + qproc->mba_size) : 0; >> + if (ret) { >> + dev_err(qproc->dev, >> + "Failed to assign mba memory access, ret - %d\n", ret); >> + goto assert_reset; >> + } >> writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); >> >> ret = q6v5proc_reset(qproc); >> @@ -657,16 +760,22 @@ 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; >> } >> >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, >> + qproc->mba_size) : 0; >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim mba memory, ret - %d\n", ret); >> qproc->running = true; >> >> q6v5_clk_disable(qproc->dev, qproc->proxy_clks, >> @@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc) >> > On success q6v5_start() will leave Linus as sole owner of mba_phys and > mpss_phys (or boot_addr) will have shared ownership. rproc_stop() should > make sure to reclaim the sole ownership of mpss_phys again, so that > subsequent calls to rproc_start() will find the memory protection in an > appropriate state. Sure! > >> return 0; >> >> +reclaim_mem: >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(REMOVE_SHARED_ACCESS_MSA, >> + qproc->mpss_phys, qproc->mpss_size) : 0; >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim fw memory, ret - %d\n", ret); >> halt_axi_ports: >> + ret = qproc->version == MSS_MSM8996 ? >> + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, >> + qproc->mba_size) : 0; >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim mba memory, ret - %d\n", ret); > Isn't it possible that the remote processor is still executing the MBA > firmware up until we halt the axi ports and assert the reset? We don't > want to remove permission until we know the CPU is stopped and it wont > fault on us. Will check. > >> 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); >> @@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev) >> if (ret) >> goto free_rproc; >> >> + qproc->version = desc->version; >> ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); >> if (ret < 0) >> goto free_rproc; >> @@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev) >> "mem", >> NULL >> }, >> + .version = MSS_MSM8916, >> }; >> >> static const struct rproc_hexagon_res msm8974_mss = { >> @@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev) >> "mem", >> NULL >> }, >> + .version = MSS_MSM8974, >> }; >> >> static const struct of_device_id q6v5_of_match[] = { >> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h >> index cc32ab8..cb0b7ee 100644 >> --- a/include/linux/qcom_scm.h >> +++ b/include/linux/qcom_scm.h >> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req { >> u32 val; >> }; >> >> +struct vmid_detail { > Whenever you add structs to a public API, make sure to add some > kerneldoc. Will check if where is it appropriate to provide documentation. > > And as this is a kernel-API I would like to see standard kernel-types > for things. OK. > >> + __le32 *from; >> + __le32 *to; >> + __le32 *permission; >> + __le32 size_from; >> + __le32 size_to; >> + __le32 size_fw; >> + __le64 fw_phy; >> + __le64 from_phy; >> + __le64 to_phy; >> + >> +}; >> + > Regards, > Bjorn
On 4/6/2017 5:14 AM, Stephen Boyd wrote: > On 03/08, Avaneesh Kumar Dwivedi wrote: >> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c >> index 4a0f5ea..187fc00 100644 >> --- a/drivers/firmware/qcom_scm-64.c >> +++ b/drivers/firmware/qcom_scm-64.c >> @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) >> >> return ret ? : res.a1; >> } >> + >> +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) > Why are we passing a structure copy? I did not use struct pointer cause i am not modifying any of the passed value, do you think i should use struct pointer than pass by value? > >> +{ >> + int ret; >> + struct qcom_scm_desc desc = {0}; >> + struct arm_smccc_res res; >> + >> + desc.args[0] = vmid.fw_phy; >> + desc.args[1] = vmid.size_fw; >> + desc.args[2] = vmid.from_phy; >> + desc.args[3] = vmid.size_from; >> + desc.args[4] = vmid.to_phy; >> + desc.args[5] = vmid.size_to; > These should all cause sparse warnings because of incorrect > assignments. Given that these are all registers, I'm not sure why > the vmid_detail structure has __le32 in it. will fix. > >> + 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; >> +} >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 893f953ea..f137f34 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -42,6 +42,18 @@ struct qcom_scm { >> >> static struct qcom_scm *__scm; >> >> +struct dest_vm_and_perm_info { >> + __le32 vm; >> + __le32 perm; >> + __le32 *ctx; > Drop the pointer? Just treat it like another number? Pointer is > really odd because it doesn't really make any sense what the > address of the pointer would be. Downstream this is pointer though unused, that is why kept same will check and change. > >> + __le32 ctx_size; >> +}; >> + >> +struct fw_region_info { >> + __le64 addr; >> + __le64 size; >> +}; >> + >> static int qcom_scm_clk_enable(void) >> { >> int ret; >> @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) >> } >> EXPORT_SYMBOL(qcom_scm_pas_shutdown); >> >> +/** >> + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old >> + * new owners of memory region for fw and metadata etc, Which is >> + * further passed to hypervisor, which does translate intermediate >> + * physical address used by subsystems. > Maybe this can be the long description, but the short description > should be shorter. OK :( > >> + * @vmid: structure with pointers and size detail of old and new >> + * owners vmid detail. >> + * Return 0 on success. > There's a standard syntax for return too. Look at kernel doc > howto please. OK. > >> + */ >> +int qcom_scm_assign_mem(struct vmid_detail vmid) > Please no structure copy. should struct pointer be OK, or direct argument passing? > >> +{ >> + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; >> + struct dest_vm_and_perm_info *to; >> + struct fw_region_info *fw_info; >> + __le64 fw_phy; >> + __le32 *from; >> + int ret = -ENOMEM; > Not sure why we assign it. It gets overwritten with the first use. Yes will correct. > >> + int i; >> + >> + from = dma_alloc_attrs(__scm->dev, vmid.size_from, >> + &vmid.from_phy, GFP_KERNEL, dma_attrs); >> + if (!from) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass source vmid detail\n"); >> + return -ENOMEM; >> + } >> + to = dma_alloc_attrs(__scm->dev, vmid.size_to, >> + &vmid.to_phy, GFP_KERNEL, dma_attrs); >> + if (!to) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass destination vmid detail\n"); >> + goto free_src_buff; >> + } >> + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info), >> + &fw_phy, GFP_KERNEL, dma_attrs); > Can we consolidate this into one allocation with the appropriate > size and then offset the different structures in it? OK. >> + if (!fw_info) { >> + dev_err(__scm->dev, >> + "failed to allocate buffer to pass firmware detail\n"); >> + goto free_dest_buff; >> + } >> + >> + /* copy detail of original owner of ddr region */ >> + /* in physically contigious buffer */ >> + memcpy(from, vmid.from, vmid.size_from); >> + >> + /* fill details of new owners of ddr region*/ >> + /* in physically contigious buffer */ >> + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { >> + to[i].vm = vmid.to[i]; >> + to[i].perm = vmid.permission[i]; > Here you want the cpu_to_le32() stuff. Please run sparse. OK, last time i tried running sparse but seems some environment problem, it did not gave any warning. > >> + to[i].ctx = NULL; >> + to[i].ctx_size = 0; >> + } >> + >> + /* copy detail of firmware region whose mapping need to be done */ >> + /* in physically contigious buffer */ > /* > * Multi-line comments are like so. > */ Will change. > >> + fw_info->addr = vmid.fw_phy; >> + fw_info->size = vmid.size_fw; >> + >> + /* reuse fw_phy and size_fw members to fill address and size of */ >> + /* fw_info buffer */ >> + vmid.fw_phy = fw_phy; >> + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32)); >> + vmid.size_fw = sizeof(*fw_info); >> + ret = __qcom_scm_assign_mem(__scm->dev, vmid); >> + if (!ret) >> + goto free_fw_buff; >> + return ret; > We don't free dma on failure? Did not get, isnt i am freeing all dma allocs on failure? > >> +free_fw_buff: >> + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info, >> + fw_phy, dma_attrs); >> +free_dest_buff: >> + dma_free_attrs(__scm->dev, vmid.size_to, to, >> + vmid.to_phy, dma_attrs); >> +free_src_buff: >> + dma_free_attrs(__scm->dev, vmid.size_from, from, >> + vmid.from_phy, dma_attrs); >> + return ret; >> +} >> +EXPORT_SYMBOL(qcom_scm_assign_mem); >> + >> static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev, >> unsigned long idx) >> { >> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h >> index 3584b00..4665a11 100644 >> --- a/drivers/firmware/qcom_scm.h >> +++ b/drivers/firmware/qcom_scm.h >> @@ -55,6 +55,9 @@ 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); >> +#define QCOM_SCM_SVC_MP 0xc > This is already defined upstream? Will check, but i thought its not there. > >> +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 >> +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid); >> >> /* common error codes */ >> #define QCOM_SCM_V2_EBUSY -12 >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index 8fd697a..62ad976 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, >> return &table; >> } >> >> +int hyp_mem_access(int id, phys_addr_t addr, size_t size) > static? Yes, will correct. > >> >> static const struct of_device_id q6v5_of_match[] = { >> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h >> index cc32ab8..cb0b7ee 100644 >> --- a/include/linux/qcom_scm.h >> +++ b/include/linux/qcom_scm.h >> @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req { >> u32 val; >> }; >> >> +struct vmid_detail { >> + __le32 *from; >> + __le32 *to; >> + __le32 *permission; >> + __le32 size_from; >> + __le32 size_to; >> + __le32 size_fw; >> + __le64 fw_phy; >> + __le64 from_phy; >> + __le64 to_phy; > should the *_phy be phys_addr_t types? > > Leave these all as u32/u64. Perhaps also move size_from/size_to > next to the arrays they're for. Also add some documentation so we > know what they're all about. OK. >
On Fri 14 Apr 06:54 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > On 4/6/2017 10:13 AM, Bjorn Andersson wrote: > > On Wed 08 Mar 10:03 PST 2017, Avaneesh Kumar Dwivedi wrote: [..] > > > +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) > > Rather than packing these parameters up in a struct I think it's cleaner > > to just pass them directly. > passing so many parameters directly is not seen good practice specially when > number of parameters to pass are many. > that is why i used struct copy, i did not use reference of struct since > values passed were not going to be modified. > but will surely look if i can work with direct passing of params. It's a good rule of thumb, but in this case you have 1 caller and 2 implementations (don't forget to stub the 32-bit one), so the overhead of having a struct just to pass these parameters probably is not worth it. And the fact that some of the members have slightly different meanings here compared to the other API is a bigger issue. > > > > > +{ > > > + int ret; > > > + struct qcom_scm_desc desc = {0}; > > > + struct arm_smccc_res res; > > > + > > > + desc.args[0] = vmid.fw_phy; > > > + desc.args[1] = vmid.size_fw; > > > + desc.args[2] = vmid.from_phy; > > > + desc.args[3] = vmid.size_from; > > > + desc.args[4] = vmid.to_phy; > > > + desc.args[5] = vmid.size_to; > > > + desc.args[6] = 0; > > > + [..] > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > > index 893f953ea..f137f34 100644 > > > --- a/drivers/firmware/qcom_scm.c > > > +++ b/drivers/firmware/qcom_scm.c > > > @@ -42,6 +42,18 @@ struct qcom_scm { > > > static struct qcom_scm *__scm; > > > +struct dest_vm_and_perm_info { > > > + __le32 vm; > > > + __le32 perm; > > > + __le32 *ctx; > > Please be explicit about the fact that this is 64 bit. > sorry i am not sure why they need to be 64 bit variable? __le32 says that elements are 32-bits, * says this is a pointer to such elements and as we are on a 64-bit system this member is 64-bit. This is a binary interface between the Linux kernel and the TrustZone code, so the struct must be encoded in exactly the same way on both sides. As Stephen suggested, making this __le64 would make this way more obvious - and endian safe. > > > > > + __le32 ctx_size; > > > +}; [..] > > > +int qcom_scm_assign_mem(struct vmid_detail vmid) > > After a long discussion with Stephen I now think that I understand > > what's actually going on here. > > > > So this function will request TZ to remove all permissions for the > > memory region in the tables specified in "from" and then for each vmid > > in "to" it will set up the specified "permission". > > > > So the "to" and "permissions" are actually a tuple, rather than > > independent lists of values. So I think this should be exposed in the > > prototype, as a list of <vmid, permission> entries. > > > > To make the function prototype more convenient I think you should turn > > "from" into a bitmap (e.g. BIT(VMID_HLOS) | BIT(VMID_MSS_MSA)). > i am not able to fully comprehend advantage of turning "from" into bitmap. > moreover when i read your below suggestion, which suggest to use "struct > qcom_scm_mem_perm" as below then i get further confused what i will do with > bitmap? Every time you call this function you have to pass a list of "from" that corresponds to the "to" of the last (successful) call to this function. In your code this means that the "from" is either 1 or 2 elements and as you can see in your own code there is no convenient way of doing this in C - having an array of 2 elements and claiming that it is either 1 or 2 entries long is not very clean. Further more, in the remoteproc driver you currently rely on the fact that you "know", in each code path, what the last "to" was and just hard code "from". But if you make this call return the next "from" as a bitmap (much more convenient than a variable length array) we can store this in the driver for each memory region and I believe the calling code will be cleaner. > > > > If you then make the function, on success, return "to" as a bitmap the > > caller can simply store that in a state variable and pass it as "from" > > in the next call. > > > > So you would have: > > > > struct qcom_scm_mem_perm new_perms[] = { > > { VMID_HLOS, PERM_READ }, > > { VMID_MSS_MSA, PREM_READ | PERM_WRITE }, > > }; > > current_perms = qcom_scm_assign_mem(ptr, size, current_perms, new_perms, 2); > This looks OK, will try to incorporate this idea. > > > > > > And I believe something like "curr_perm" and "new_perm" are even better > > names than "from" and "to" > ok. > > [..] > > > + > > > + /* copy detail of original owner of ddr region */ > > > + /* in physically contigious buffer */ > > > + memcpy(from, vmid.from, vmid.size_from); > > With "from" as a bitmap see hweight_long() to figure out the size of > > "from". > > hweight_long() returns number of bits set in bitmap? is this correct understanding? > Correct. [..] > > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h [..] > > > + > > > +#define VMID_HLOS 0x3 > > > +#define VMID_MSS_MSA 0xF > > > +#define PERM_READ 0x4 > > > +#define PERM_WRITE 0x2 > > > +#define PERM_EXEC 0x1 > > > +#define PERM_RW (PERM_READ | PERM_WRITE) > > These belongs in the SCM API instead. (Prefix them appropriately) > Do you mean something like SCM_VMID_HLOS? This is part of the qcom-scm API, other defines there are of the form QCOM_SCM_* so QCOM_SCM_VMID_HLOS should be ok. > > > > > static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, > > > const struct qcom_mss_reg_res *reg_res) > > > { > > > @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, > > > return &table; > > > } > > > +int hyp_mem_access(int id, phys_addr_t addr, size_t size) > > I much prefer the downstream split and naming of this function, rather > > than using a switch to implement 4 different functions in one please > > split it up. > so you mean i need to have seprate function for each "int id"? in switch > case? Yes. > just for sake of minimum code churn i tried to work with single function. > In terms of C-code you have 1 function, but conceptually you have 4 - so any reasoning about what this function does fully depends on "id" as well. [..] > > > > > > And as far as I can tell the downstream driver load the segments and > > then make the MSS sole owner of the memory region. Do note that it's > > perfectly fine to refactor code to make things better fit a natural and > > easy to follow flow of ownership here! > Not very clear what to do? In order to hand over ownership completely we can no longer verify the segments as we load them - we will have to do this in two steps. So I believe this code has to be refactored to make this in two separate steps - with the latter done after we transmit the ownership of the memory to the remote. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 14 Apr 07:01 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > On 4/6/2017 5:14 AM, Stephen Boyd wrote: > > On 03/08, Avaneesh Kumar Dwivedi wrote: [..] > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > > index 893f953ea..f137f34 100644 > > > --- a/drivers/firmware/qcom_scm.c > > > +++ b/drivers/firmware/qcom_scm.c > > > @@ -42,6 +42,18 @@ struct qcom_scm { > > > static struct qcom_scm *__scm; > > > +struct dest_vm_and_perm_info { > > > + __le32 vm; > > > + __le32 perm; > > > + __le32 *ctx; > > Drop the pointer? Just treat it like another number? Pointer is > > really odd because it doesn't really make any sense what the > > address of the pointer would be. > Downstream this is pointer though unused, that is why kept same will check > and change. The problem is that the size of a pointer depends on which platform you're on. Spelling out __le32 here is then just deceiving. > > > > > + __le32 ctx_size; > > > +}; [..] > > > +int qcom_scm_assign_mem(struct vmid_detail vmid) [..] > > > + ret = __qcom_scm_assign_mem(__scm->dev, vmid); > > > + if (!ret) > > > + goto free_fw_buff; > > > + return ret; > > We don't free dma on failure? > Did not get, isnt i am freeing all dma allocs on failure? In the event that __qcom_scm_assign_mem() returns non-zero your not jumping to free_fw_buff and just returning "ret" without freeing the memory. [..] > > > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > > > index 3584b00..4665a11 100644 > > > --- a/drivers/firmware/qcom_scm.h > > > +++ b/drivers/firmware/qcom_scm.h > > > @@ -55,6 +55,9 @@ 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); > > > +#define QCOM_SCM_SVC_MP 0xc > > This is already defined upstream? > Will check, but i thought its not there. We use the qcom_scm-api to abstract these facts, so this is an internal constant - as such it goes in drivers/firmware/qcom_scm.h - and it's already defined. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c index 4a0f5ea..187fc00 100644 --- a/drivers/firmware/qcom_scm-64.c +++ b/drivers/firmware/qcom_scm-64.c @@ -358,3 +358,28 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) return ret ? : res.a1; } + +int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid) +{ + int ret; + struct qcom_scm_desc desc = {0}; + struct arm_smccc_res res; + + desc.args[0] = vmid.fw_phy; + desc.args[1] = vmid.size_fw; + desc.args[2] = vmid.from_phy; + desc.args[3] = vmid.size_from; + desc.args[4] = vmid.to_phy; + desc.args[5] = vmid.size_to; + 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; +} diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index 893f953ea..f137f34 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -42,6 +42,18 @@ struct qcom_scm { static struct qcom_scm *__scm; +struct dest_vm_and_perm_info { + __le32 vm; + __le32 perm; + __le32 *ctx; + __le32 ctx_size; +}; + +struct fw_region_info { + __le64 addr; + __le64 size; +}; + static int qcom_scm_clk_enable(void) { int ret; @@ -292,6 +304,87 @@ int qcom_scm_pas_shutdown(u32 peripheral) } EXPORT_SYMBOL(qcom_scm_pas_shutdown); +/** + * qcom_scm_assign_mem() - Allocate and fill vmid detail of old + * new owners of memory region for fw and metadata etc, Which is + * further passed to hypervisor, which does translate intermediate + * physical address used by subsystems. + * @vmid: structure with pointers and size detail of old and new + * owners vmid detail. + * Return 0 on success. + */ +int qcom_scm_assign_mem(struct vmid_detail vmid) +{ + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; + struct dest_vm_and_perm_info *to; + struct fw_region_info *fw_info; + __le64 fw_phy; + __le32 *from; + int ret = -ENOMEM; + int i; + + from = dma_alloc_attrs(__scm->dev, vmid.size_from, + &vmid.from_phy, GFP_KERNEL, dma_attrs); + if (!from) { + dev_err(__scm->dev, + "failed to allocate buffer to pass source vmid detail\n"); + return -ENOMEM; + } + to = dma_alloc_attrs(__scm->dev, vmid.size_to, + &vmid.to_phy, GFP_KERNEL, dma_attrs); + if (!to) { + dev_err(__scm->dev, + "failed to allocate buffer to pass destination vmid detail\n"); + goto free_src_buff; + } + fw_info = dma_alloc_attrs(__scm->dev, sizeof(*fw_info), + &fw_phy, GFP_KERNEL, dma_attrs); + if (!fw_info) { + dev_err(__scm->dev, + "failed to allocate buffer to pass firmware detail\n"); + goto free_dest_buff; + } + + /* copy detail of original owner of ddr region */ + /* in physically contigious buffer */ + memcpy(from, vmid.from, vmid.size_from); + + /* fill details of new owners of ddr region*/ + /* in physically contigious buffer */ + for (i = 0; i < (vmid.size_to / sizeof(__le32)); i++) { + to[i].vm = vmid.to[i]; + to[i].perm = vmid.permission[i]; + to[i].ctx = NULL; + to[i].ctx_size = 0; + } + + /* copy detail of firmware region whose mapping need to be done */ + /* in physically contigious buffer */ + fw_info->addr = vmid.fw_phy; + fw_info->size = vmid.size_fw; + + /* reuse fw_phy and size_fw members to fill address and size of */ + /* fw_info buffer */ + vmid.fw_phy = fw_phy; + vmid.size_to = sizeof(*to) * (vmid.size_to / sizeof(__le32)); + vmid.size_fw = sizeof(*fw_info); + ret = __qcom_scm_assign_mem(__scm->dev, vmid); + if (!ret) + goto free_fw_buff; + return ret; +free_fw_buff: + dma_free_attrs(__scm->dev, sizeof(*fw_info), fw_info, + fw_phy, dma_attrs); +free_dest_buff: + dma_free_attrs(__scm->dev, vmid.size_to, to, + vmid.to_phy, dma_attrs); +free_src_buff: + dma_free_attrs(__scm->dev, vmid.size_from, from, + vmid.from_phy, dma_attrs); + return ret; +} +EXPORT_SYMBOL(qcom_scm_assign_mem); + static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev, unsigned long idx) { diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index 3584b00..4665a11 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -55,6 +55,9 @@ 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); +#define QCOM_SCM_SVC_MP 0xc +#define QCOM_MEM_PROT_ASSIGN_ID 0x16 +extern int __qcom_scm_assign_mem(struct device *dev, struct vmid_detail vmid); /* common error codes */ #define QCOM_SCM_V2_EBUSY -12 diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index 8fd697a..62ad976 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -110,6 +110,7 @@ struct rproc_hexagon_res { struct qcom_mss_reg_res *active_supply; char **proxy_clk_names; char **active_clk_names; + int version; }; struct q6v5 { @@ -153,8 +154,28 @@ struct q6v5 { size_t mpss_size; struct qcom_rproc_subdev smd_subdev; + int version; }; +enum { + MSS_MSM8916, + MSS_MSM8974, + MSS_MSM8996, +}; + +enum { + ASSIGN_ACCESS_MSA, + REMOVE_ACCESS_MSA, + ASSIGN_SHARED_ACCESS_MSA, + REMOVE_SHARED_ACCESS_MSA, +}; + +#define VMID_HLOS 0x3 +#define VMID_MSS_MSA 0xF +#define PERM_READ 0x4 +#define PERM_WRITE 0x2 +#define PERM_EXEC 0x1 +#define PERM_RW (PERM_READ | PERM_WRITE) static int q6v5_regulator_init(struct device *dev, struct reg_info *regs, const struct qcom_mss_reg_res *reg_res) { @@ -288,6 +309,54 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, return &table; } +int hyp_mem_access(int id, phys_addr_t addr, size_t size) +{ + struct vmid_detail vmid; + int ret; + + switch (id) { + case ASSIGN_ACCESS_MSA: + vmid.from = (int[]) { VMID_HLOS }; + vmid.to = (int[]) { VMID_MSS_MSA }; + vmid.permission = (int[]) { PERM_READ | PERM_WRITE }; + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); + break; + case REMOVE_ACCESS_MSA: + vmid.from = (int[]) { VMID_MSS_MSA }; + vmid.to = (int[]) { VMID_HLOS }; + vmid.permission = + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; + vmid.size_from = vmid.size_to = 1 * sizeof(__le32); + break; + case ASSIGN_SHARED_ACCESS_MSA: + vmid.from = (int[]) { VMID_HLOS }; + vmid.to = (int[]) { VMID_HLOS, VMID_MSS_MSA }; + vmid.permission = (int[]) { PERM_RW, PERM_RW }; + vmid.size_from = 1 * sizeof(__le32); + vmid.size_to = 2 * sizeof(__le32); + break; + case REMOVE_SHARED_ACCESS_MSA: + vmid.from = (int[]) { VMID_HLOS, VMID_MSS_MSA }; + vmid.to = (int[]) { VMID_HLOS }; + vmid.permission = + (int[]) { PERM_READ | PERM_WRITE | PERM_EXEC }; + vmid.size_from = 2 * sizeof(__le32); + vmid.size_to = 1 * sizeof(__le32); + break; + default: + break; + } + + vmid.fw_phy = addr; + vmid.size_fw = size; + ret = qcom_scm_assign_mem(vmid); + if (ret) + pr_err("%s: Failed to assign memory protection, ret = %d\n", + __func__, ret); + + return ret; +} + static int q6v5_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5 *qproc = rproc->priv; @@ -461,6 +530,15 @@ 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 = qproc->version == MSS_MSM8996 ? + hyp_mem_access(ASSIGN_ACCESS_MSA, phys, + ALIGN(fw->size, SZ_4K)) : 0; + if (ret) { + dev_err(qproc->dev, + "Failed to assign metadata memory, ret - %d\n", ret); + return -ENOMEM; + } 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 +548,13 @@ 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 */ + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(REMOVE_ACCESS_MSA, phys, + ALIGN(fw->size, SZ_4K)) : 0; + if (ret) + dev_err(qproc->dev, + "Failed to reclaim metadata memory, ret - %d\n", ret); dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); return ret < 0 ? ret : 0; @@ -578,6 +663,16 @@ static int q6v5_mpss_load(struct q6v5 *qproc) size = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); if (!size) { boot_addr = relocate ? qproc->mpss_phys : min_addr; + /* Hypervisor mapping of modem fw */ + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(ASSIGN_SHARED_ACCESS_MSA, + boot_addr, qproc->mpss_size) : 0; + if (ret) { + dev_err(qproc->dev, + "Failed to assign fw memory access, ret - %d\n", + ret); + return -ENOMEM; + } writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); } @@ -636,6 +731,14 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(ASSIGN_ACCESS_MSA, qproc->mba_phys, + qproc->mba_size) : 0; + if (ret) { + dev_err(qproc->dev, + "Failed to assign mba memory access, ret - %d\n", ret); + goto assert_reset; + } writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); ret = q6v5proc_reset(qproc); @@ -657,16 +760,22 @@ 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; } + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, + qproc->mba_size) : 0; + if (ret) + dev_err(qproc->dev, + "Failed to reclaim mba memory, ret - %d\n", ret); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks, @@ -676,7 +785,20 @@ static int q6v5_start(struct rproc *rproc) return 0; +reclaim_mem: + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(REMOVE_SHARED_ACCESS_MSA, + qproc->mpss_phys, qproc->mpss_size) : 0; + if (ret) + dev_err(qproc->dev, + "Failed to reclaim fw memory, ret - %d\n", ret); halt_axi_ports: + ret = qproc->version == MSS_MSM8996 ? + hyp_mem_access(REMOVE_ACCESS_MSA, qproc->mba_phys, + qproc->mba_size) : 0; + if (ret) + dev_err(qproc->dev, + "Failed to reclaim mba memory, ret - %d\n", ret); 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); @@ -1015,6 +1137,7 @@ static int q6v5_probe(struct platform_device *pdev) if (ret) goto free_rproc; + qproc->version = desc->version; ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); if (ret < 0) goto free_rproc; @@ -1090,6 +1213,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .version = MSS_MSM8916, }; static const struct rproc_hexagon_res msm8974_mss = { @@ -1127,6 +1251,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .version = MSS_MSM8974, }; static const struct of_device_id q6v5_of_match[] = { diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h index cc32ab8..cb0b7ee 100644 --- a/include/linux/qcom_scm.h +++ b/include/linux/qcom_scm.h @@ -23,6 +23,19 @@ struct qcom_scm_hdcp_req { u32 val; }; +struct vmid_detail { + __le32 *from; + __le32 *to; + __le32 *permission; + __le32 size_from; + __le32 size_to; + __le32 size_fw; + __le64 fw_phy; + __le64 from_phy; + __le64 to_phy; + +}; + extern bool qcom_scm_is_available(void); extern bool qcom_scm_hdcp_available(void); @@ -36,6 +49,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(struct vmid_detail vmid); #define QCOM_SCM_CPU_PWR_DOWN_L2_ON 0x0 #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF 0x1
This patch add scm call support to make hypervisor call to enable access of fw regions in ddr to mss subsystem on arm-v8 arch soc's. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- drivers/firmware/qcom_scm-64.c | 25 +++++++ drivers/firmware/qcom_scm.c | 93 ++++++++++++++++++++++++++ drivers/firmware/qcom_scm.h | 3 + drivers/remoteproc/qcom_q6v5_pil.c | 129 ++++++++++++++++++++++++++++++++++++- include/linux/qcom_scm.h | 14 ++++ 5 files changed, 262 insertions(+), 2 deletions(-)