On Thu, Sep 01, 2016 at 01:06:04PM -0500, Andrew F. Davis wrote:

On 09/01/2016 04:22 AM, Jens Wiklander wrote:

quoted

On Wed, Aug 31, 2016 at 11:40:20AM -0500, Andrew F. Davis wrote:

quoted

On 08/31/2016 08:50 AM, Jens Wiklander wrote:

quoted

On Tue, Aug 30, 2016 at 03:23:24PM -0500, Andrew F. Davis wrote:

quoted

On 08/22/2016 08:00 AM, Jens Wiklander wrote:

quoted

+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.

quoted

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