Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/12/2022 11:33, Sibi Sankar wrote:
> 
> 
> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>> The memory region allocated using dma_alloc_attr with no kernel mapping
>>> attribute set would still be a part of the linear kernel map. Any access
>>> to this region by the application processor after assigning it to the
>>> remote Q6 will result in a XPU violation. Fix this by replacing the
>>> dynamically allocated memory region with a no-map carveout and unmap the
>>> modem metadata memory region before passing control to the remote Q6.
>>>
>>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>>   
>>>   	return ret < 0 ? ret : 0;
>>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>>   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>>   	qproc->mpss_size = resource_size(&r);
>>>   
>>> +	if (!child) {
>>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>>> +	} else {
>>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>>
>> Bindings do not allow to have child "metadata", do they?
> 
> memory-region property was used to specify mba/mpss region in a phandle
> array only from SC7180 SoC. All the older dtbs in the wild/upstream
> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
> as to not break bindings when newer kernel uses a older dtb.

This does not explain why you extend the driver without extending the
bindings. You do not do it for legacy stuff but for SC7180. But even for
legacy devices you cannot add new properties without having it in some
legacy bindings.


Best regards,
Krzysztof