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. 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.
> >> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
