Date:   Wed, 7 Jun 2017 21:57:25 +0530
From:   "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     sboyd@...eaurora.org, agross@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem
 ownership switch



On 6/2/2017 11:25 PM, Bjorn Andersson wrote:
> On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
>
>> 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
>>
> 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.
OK
>
>>      and that based on successful return of qcom_scm_assign () they should be
>> treated as *current_perm and *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.
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.

>
>>     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.
>
>>     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.
>
>>     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.
>
> Regards,
> Bjorn
>
>> -Avaneesh
>>
>>
>> On 6/2/2017 2:12 AM, Bjorn Andersson wrote:
>>> On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>>>
>>>> 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.
>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>>>> @@ -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.
>>>
>>>> +				   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.
>>>
>>>> +	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.
>>>
>>>> +		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;
>>>> +}
>>>> +
>>> Regards,
>>> Bjorn
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> -- 
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>

-- 
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