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

[Andrew F. Davis]

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.

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

>>>> [snip]
>>>>
>>>>> +#define OPTEE_MSG_ATTR_CACHE_SHIFT		16
>>>>> +#define OPTEE_MSG_ATTR_CACHE_MASK		0x7
>>>>
>>>> GENMASK
>>>
>>> Do you mean that OPTEE_MSG_ATTR_CACHE_MASK should be defined using the
>>> GENMASK() macro? If so I guess I should update OPTEE_MSG_ATTR_TYPE_MASK
>>> also.
>>>
>>
>> Right, I can tell what bits 0x7 mask, but sometimes it's not so easy
>> (7F8, etc) so I find it's best to use GENMASKS for all masks so I don't
>> have to ever think about it at all.
>>
>> Also I like to mask then shift, so it would be:
>>
>> #define OPTEE_MSG_ATTR_CACHE_SHIFT	16
>> #define OPTEE_MSG_ATTR_CACHE_MASK	GENMASK(18, 16)
>>
>> Then we can see at a glance the actual bits we are looking for (18-16)
>> and verify the shift is the right amount.
> 
> I'm wired the other way around, I usually shift then mask, I suppose
> it's a matter of taste. I'm happy to use GENMASK instead to define
> OPTEE_MSG_ATTR_CACHE_MASK, but I'd like to keep it unshifted if you
> don't mind.
> 
> Thanks,
> Jens
>