Date:	Wed, 15 Jul 2015 17:55:21 -0700
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Bjorn Andersson <bjorn.andersson@...ymobile.com>
CC:	Andy Gross <agross@...eaurora.org>, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org
Subject: Re: [PATCH v2] firmware: qcom: scm: Peripheral Authentication Service

On 07/15/2015 05:35 PM, Bjorn Andersson wrote:
> On Wed 15 Jul 16:43 PDT 2015, Stephen Boyd wrote:
>
>> On 07/15, Bjorn Andersson wrote:
>>> This adds the Peripheral Authentication Service (PAS) interface to the
>>> Qualcomm SCM interface. The API is used to authenticate and boot a range
>>> of external processors in various Qualcomm platforms.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@...ymobile.com>
>>> ---
>>>
>>> Changes since v1:
>>> - Big endian compatibility
>> Did you try out a big endian kernel?
>>
> No, you're still the only one.

:)

>>> +			    &ret_val, sizeof(ret_val));
>>> +
>>> +	return ret ? false : !!ret_val;
>>> +}
>>> +
>>> +int __qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>>> +{
>>> +	dma_addr_t mdata_phys;
>>> +	void *mdata_buf;
>>> +	__le32 scm_ret;
>>> +	int ret;
>>> +	struct pas_init_image_req {
>>> +		__le32 proc;
>>> +		__le32 image_addr;
>>> +	} request;
>>> +
>>> +	/*
>>> +	 * During the scm call memory protection will be enabled for the meta
>>> +	 * data blob, so make sure it's physically contiguous, 4K aligned and
>>> +	 * non-cachable to avoid XPU violations.
>>> +	 */
>>> +	mdata_buf = dma_alloc_coherent(NULL, size, &mdata_phys, GFP_KERNEL);
>> This should pass a device pointer instead of NULL here. Please
>> make struct device an argument of this function and pass
>> something there besides NULL in the calling driver. Or we should
>> make SCM into a platform device driver with a node in DT (named
>> firmware?). Then if we need to do anything special for DMA to the
>> firmware, etc. we have a struct device that can describe that.
>>
> I think making scm into a platform driver seems very much overkill,
> passing the callers device * sounds reasonable. My only concern would be
> if associating this dma allocation with the client has any further
> implications, but I'll have to read up a little bit on how that works.
>
>> Also, dma_alloc_coherent() doesn't do enough to prevent XPU
>> violations because memory returned from that function on ARM is
>> not guaranteed to be device memory and so we could speculatively
>> access the locked down metadata region. This is why we added the
>> strongly ordered mapping property and pass that to
>> dma_alloc_attrs in the downstream code so we can change the page
>> table attributes of the mapping to be device memory. Not doing
>> this can lead to random crashes when some read speculates on the
>> metadata and the secure world intercepts it and shuts the system
>> down.
>>
> The code is taken verbatim from msm-3.4 and the comment is picked from
> the git log, sorry to hear that this is not enough.

Please move up to msm-3.14 or msm-3.10. Try to find the newest stuff if 
it's code like this that isn't specific for a particular SoC. Otherwise 
we're going to miss random bug fixes that haven't trickled down to trees 
for chips that are two to three years old.

>
>> I was going to say we could try to use the carveout/reserved
>> memory code but that doesn't look fool proof. From what I can
>> tell CMA doesn't use the same page table attributes for the
>> mapping that dma-coherent does, so if we use dma-coherent it will
>> use ioremap and work but if we use CMA it won't (at least it
>> looks like bufferable memory there). Can we add a way to request
>> memory doesn't allow speculatioan through the DMA APIs?
>>
> I haven't looked enough at dma allocations, but this is what worries me
> when using the clients dev pointer (I'm under the impression that these
> choices follow the dev*).

Yes it does. If the device is cache coherent (e.g. the video processor 
may be cache coherent) or even if we want to have two different regions 
of memory carved out for the device then using the client's dev pointer 
won't work well.

I think for this sort of allocation it makes sense to make SCM into a 
platform driver/device so that we can assign the right attributes to a 
memory carveout associated with it. It will also help when we need to 
max out crypto clocks and bus bandwidth or other things that are 
strictly related to what the firmware needs and not the remote 
processor. The trouble is probe defer, so we may need to have some sort 
of get/put API that returns EPROBE_DEFER so that client drivers can 
figure out when they need to wait for SCM to be ready.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists