Re: [PATCH v11 3/4] tee: add OP-TEE driver

[Andrew F. Davis]

On 09/02/2016 05:49 AM, Jens Wiklander wrote:
> On Thu, Sep 01, 2016 at 01:06:04PM -0500, Andrew F. Davis wrote:
>> On 09/01/2016 04:22 AM, Jens Wiklander wrote:
>>> On Wed, Aug 31, 2016 at 11:40:20AM -0500, Andrew F. Davis wrote:
>>>> On 08/31/2016 08:50 AM, Jens Wiklander wrote:
>>>>> On Tue, Aug 30, 2016 at 03:23:24PM -0500, Andrew F. Davis wrote:
>>>>>> On 08/22/2016 08:00 AM, Jens Wiklander wrote:
>>>>>>> +static struct tee_shm_pool *
>>>>>>> +optee_config_shm_ioremap(struct device *dev, optee_invoke_fn *invoke_fn,
>>>>>>> +			 void __iomem **ioremaped_shm)
>>>>>>> +{
>>>>>>> +	struct arm_smccc_res res;
>>>>>>> +	struct tee_shm_pool *pool;
>>>>>>> +	unsigned long vaddr;
>>>>>>> +	phys_addr_t paddr;
>>>>>>> +	size_t size;
>>>>>>> +	phys_addr_t begin;
>>>>>>> +	phys_addr_t end;
>>>>>>> +	void __iomem *va;
>>>>>>> +	struct tee_shm_pool_mem_info priv_info;
>>>>>>> +	struct tee_shm_pool_mem_info dmabuf_info;
>>>>>>> +
>>>>>>> +	invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res);
>>>>>>> +	if (res.a0 != OPTEE_SMC_RETURN_OK) {
>>>>>>> +		dev_info(dev, "shm service not available\n");
>>>>>>> +		return ERR_PTR(-ENOENT);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (res.a3 != OPTEE_SMC_SHM_CACHED) {
>>>>>>> +		dev_err(dev, "only normal cached shared memory supported\n");
>>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	begin = roundup(res.a1, PAGE_SIZE);
>>>>>>> +	end = rounddown(res.a1 + res.a2, PAGE_SIZE);
>>>>>>
>>>>>> res.a1/2/3 is really hard to review and understand, would it work better
>>>>>> to use a union or cast for the output of invoke_fn based on the function
>>>>>> type?
>>>>>>
>>>>>> In the header that defines what the returned info from these calls means
>>>>>> add:
>>>>>>
>>>>>> struct OPTEE_SMC_GET_SHM_CONFIG_RESULT {
>>>>>> 	unsigned long status;
>>>>>> 	unsigned long start;
>>>>>> 	unsigned long size;
>>>>>> 	unsigned long settings;
>>>>>> };
>>>>>>
>>>>>> then:
>>>>>>
>>>>>> union something result;
>>>>>>
>>>>>> begin = roundup(result.ret.start, PAGE_SIZE);
>>>>>> end = rounddown(result.ret.start + result.ret.size, PAGE_SIZE);
>>>>>>
>>>>>> or similar with just casting to the better named struct type.
>>>>>
>>>>> optee_smc.h describes what's passed in the registers during an SMC I'd
>>>>> rather not clutter it with structs that doesn't add any information
>>>>> there. I'm not that happy with casting or using unions to alias struct
>>>>> arm_smccc_res either. How about a simple wrapper function for this call
>>>>> to deal with the details instead?
>>>>>
>>>>
>>>> I think that would be a good idea anyway, for instance, someday if the
>>>> interface changes slightly then you will be able to contain the
>>>> compatibility fixes in the wrapper and not out here in the main driver.
>>>
>>> That interface is supposed to be a stable ABI, incompatible changes in
>>> the ABI should be discouraged. If there's an incompatible change it has
>>> to be dealt with in the main driver.
>>
>> Why? This driver is for "OPTEE" not "OPTEE v2.0.01.02", any minor ABI
>> changes should be abstracted away as much as possible to keep the main
>> driver logically simple, agnostic to any OPTEE version ABI quirks/handling.
> 
> Call me naive, but I don't expect any quirks. The ABI should only be
> extended with new functions and old may be deprecated.
> 

If only it was always this simple Linus would have nothing to yell about
:), but sometimes ABI quirks happen. I can't really think of a good
example right now, and I guess we can always add that level of
indirection when/if we ever need it, so I'll drop this concern for now.

> Take the function optee_config_shm_ioremap() as an example. That
> function will not be used if OP-TEE doesn't use a specific shared memory
> pool but instead allocate shared memory via vmalloc() or from user
> space.
> 
> This kind of changes/extensions are expected, but that's probably things
> the driver need to deal with directly since if change doesn't add
> something significant it wouldn't be needed.
> 
>>
>>> A small wrapper function in a
>>> standalone header file has no chance here as it probably involves using
>>> information gathered while probing secure world.
>>>
>>> What I meant was a small wrapper function just above 
>>> optee_config_shm_ioremap() to deal with only this call.
>>>
>>
>> This wouldn't do anything that a cast couldn't do. Why not put the
>> wrapper function in the header as part of that OPTEE version's ABI
>> definition?
> 
> Choosing between wrapper functions or structs in optee_smc.h I'd choose
> structs. I'll add structs for the ABI functions where it helps, skipping
> for instance the OPTEE_SMC_*UID and OPTEE_SMC_CALL_WITH_ARG functions.
> Would that be OK?
> 

That's fine, it wouldn't add anything to name those return values.

Thanks,
Andrew