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

[Jens Wiklander <jens.wiklander@linaro.org>]

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.

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?

Thanks,
Jens