From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jens Wiklander Subject: Re: [PATCH v11 3/4] tee: add OP-TEE driver Date: Fri, 2 Sep 2016 12:49:52 +0200 Message-ID: <20160902104950.GA24088@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> <20160901092213.GA3968@ermac> <0f9a528e-5288-ad4b-256e-f7a0b5952c3c@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <0f9a528e-5288-ad4b-256e-f7a0b5952c3c@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: "Andrew F. Davis" Cc: valentin.manea@huawei.com, devicetree@vger.kernel.org, javier@javigon.com, emmanuel.michel@st.com, Arnd Bergmann , Nishanth Menon , Greg Kroah-Hartman , Will Deacon , linux-kernel@vger.kernel.org, jean-michel.delorme@st.com, Jason Gunthorpe , Rob Herring , Al Viro , Mark Rutland , Michal Simek , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.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