From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754017AbcIAJWa (ORCPT ); Thu, 1 Sep 2016 05:22:30 -0400 Received: from mail-lf0-f45.google.com ([209.85.215.45]:32987 "EHLO mail-lf0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751134AbcIAJWX (ORCPT ); Thu, 1 Sep 2016 05:22:23 -0400 Date: Thu, 1 Sep 2016 11:22:18 +0200 From: Jens Wiklander To: "Andrew F. Davis" Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Greg Kroah-Hartman , Al Viro , valentin.manea@huawei.com, jean-michel.delorme@st.com, emmanuel.michel@st.com, javier@javigon.com, Jason Gunthorpe , Mark Rutland , Michal Simek , Rob Herring , Will Deacon , Arnd Bergmann , Nishanth Menon Subject: Re: [PATCH v11 3/4] tee: add OP-TEE driver Message-ID: <20160901092213.GA3968@ermac> References: <1471870856-18684-1-git-send-email-jens.wiklander@linaro.org> <1471870856-18684-4-git-send-email-jens.wiklander@linaro.org> <050a2ce4-e175-d09e-ddff-3b0cbe766d88@ti.com> <20160831135015.GB25472@ermac> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: jens.wiklander@linaro.org (Jens Wiklander) Date: Thu, 1 Sep 2016 11:22:18 +0200 Subject: [PATCH v11 3/4] tee: add OP-TEE driver In-Reply-To: References: <1471870856-18684-1-git-send-email-jens.wiklander@linaro.org> <1471870856-18684-4-git-send-email-jens.wiklander@linaro.org> <050a2ce4-e175-d09e-ddff-3b0cbe766d88@ti.com> <20160831135015.GB25472@ermac> Message-ID: <20160901092213.GA3968@ermac> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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