On Wed, Mar 25, 2015 at 04:44:27PM +0100, Joakim Bech wrote:
Hi,
On Wed, Mar 25, 2015 at 12:01:40PM +0100, Jens Wiklander wrote:
+static long tee_ioctl_shm_alloc(struct tee_filp *teefilp,
struct tee_ioctl_shm_alloc_data __user *udata)+{
- long ret;
- struct tee_ioctl_shm_alloc_data data;
- struct tee_shm *shm;
- if (copy_from_user(&data, udata, sizeof(data)))
return -EFAULT;- /* Currently no input flags are supported */
- if (data.flags)
return -EINVAL;- shm = tee_shm_alloc(teefilp->teedev, teefilp, data.size,
TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);- if (IS_ERR(shm))
return PTR_ERR(shm);- ret = teefilp->teedev->desc->ops->shm_share(shm);
I was thinking a bit when it comes to this function. Now we start by allocating memory (tee_shm_alloc) and then we let the specific driver get a chance to do something with the shared memory we just allocated.
Wouldn't it be nicer to turn things around? I.e, first ask the specific driver if it wants to do something and then if needed call the tee_shm_alloc function? I.e, let the specific driver have a chance to override the default implementation?
For example, lets say that a certain specific driver needs to deal with some shared memory on it's own. So use cases could be, for example: a) The specific driver handles shared memory entirely on its own. b) The specific driver give hints (by settings some flags) to the generic driver. I.e, something like this.
Pseudo... /* This call updates "some_flag" in shm */ ret = teefilp->teedev->desc->ops->shm_share(shm);
u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF;
switch (shm.some_flag) { case HANDLED_BY_SPECIFIC: /* * Don't call tee_shm_alloc(), since the specific driver * already has allocated shared memory and don't want the * generic driver to do the same. */ break;
case FEATURE_X_REQUIRED: flags |= DO_SOME_MORE_MAGIC; /* Fall through ... */
case NO_FLAG_SET: default: /* Simply call the generic shm alloc function */ shm = tee_shm_alloc(teefilp->teedev, teefilp, data.size, flags); }
Comments? Thoughts?
The original purpose of the "shm_share" callback was to give the specific driver a chance to notify the Secure OS that there's another block of shared memory.
If we want to override the default shared memory allocation in the generic driver I see two other options: 1. Use CMA and assign a specific CMA context to the specific device with dma_declare_contiguous(). We'll need to update the current shm alloc code in the TEE subsystem to use the DMA-API instead (as far as I can tell, that's how CMA is used), but that has more or less been in the plan. 2. We extend to api towards the specific driver allowing it to register its own shared memory allocator.
I would prefer option 1 as it uses services already available in the kernel.
- if (ret)
goto err;- data.flags = shm->flags;
- data.size = shm->size;
- data.fd = tee_shm_fd(shm);
- if (data.fd < 0) {
ret = data.fd;goto err;- }
- if (copy_to_user(udata, &data, sizeof(data))) {
ret = -EFAULT;goto err;- }
- return 0;
+err:
- tee_shm_free(shm);
- return ret;
+}
-- Regards, Joakim