| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a8123857-e4e8-e365-f982-65bd7b65fa02@codeaurora.org>
Date: Fri, 14 Apr 2017 19:31:31 +0530
From: "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
To: Stephen Boyd <sboyd@...eaurora.org>
Cc: bjorn.andersson@...aro.org, agross@...eaurora.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v3 1/2] soc: qcom: Add support of scm call for mss rproc
to share access of ddr
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.
>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Powered by blists - more mailing lists