From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Subject: Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Date: Fri, 12 Jul 2013 11:24:37 +0100 Message-ID: <20130712102430.GA3249@localhost.localdomain> References: <531356997.66774.1373571839852.JavaMail.apache@mail81.abv.bg> <51DF0D7A.4090008@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <51DF0D7A.4090008@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: Santosh Shilimkar Cc: nm@ti.com, linux@arm.linux.org.uk, aaro.koskinen@iki.fi, tony@atomide.com, pdeschrijver@nvidia.com, linux-kernel@vger.kernel.org, =?utf-8?B?0JjQstCw0LnQu9C+INCU0LjQvNC40YLRgNC+0LI=?= , pavel@ucw.cz, pali.rohar@gmail.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: linux-omap@vger.kernel.org T24gVGh1LCBKdWwgMTEsIDIwMTMgYXQgMDM6NTQ6MzRQTSAtMDQwMCwgU2FudG9zaCBTaGlsaW1r YXIgd3JvdGU6Cj4gT24gVGh1cnNkYXkgMTEgSnVseSAyMDEzIDAzOjQzIFBNLCDQmNCy0LDQudC7 0L4g0JTQuNC80LjRgtGA0L7QsiB3cm90ZToKPiA+ICAKPiA+IAo+ID4gCj4gPiAKPiA+IAo+ID4g ID4tLS0tLS0tLSDQntGA0LjQs9C40L3QsNC70L3QviDQv9C40YHQvNC+IC0tLS0tLS0tCj4gPiAg PtCe0YI6ICBEYXZlIE1hcnRpbiAKPiA+ICA+0J7RgtC90L7RgdC90L46IFJlOiBbUEFUQ0ggdjIg MS8yXSBBUk06IE9NQVA6IEFkZCBzZWN1cmUgZnVuY3Rpb24gb21hcF9zbWMzKCkgd2hpY2gKPiA+ ICBjYWxsaW5nIGluc3RydWN0aW9uIHNtYyAjMQo+ID4gID7QlNC+OiBQYWxpIFJvaMOhciAKPiA+ ICA+0JjQt9C/0YDQsNGC0LXQvdC+INC90LA6INCh0YDRj9C00LAsIDIwMTMsINCu0LvQuCAxMCAy MDo0NToyNiBFRVNUCj4gPiAgPgo+ID4gID4KPiA+ICA+T24gV2VkLCBKdWwgMTAsIDIwMTMgYXQg MDI6NTk6MDRQTSArMDIwMCwgUGFsaSBSb2jDoXIgd3JvdGU6Cj4gPiAgPj4gT3RoZXIgc2VjdXJl IGZ1bmN0aW9ucyBvbWFwX3NtYzEoKSBhbmQgb21hcF9zbWMyKCkgY2FsbGluZyBpbnN0cnVjdGlv biBzbWMgIzAKPiA+ICA+PiBidXQgTm9raWEgUlgtNTEgYm9hcmQgbmVlZHMgdG8gY2FsbCBzbWMg IzEgZm9yIFBQQSBhY2Nlc3MuCj4gPiAgPj4gCj4gPiAgPj4gU2lnbmVkLW9mZi1ieTogSXZheWxv IERpbWl0cm92IAo+ID4gID4+IFNpZ25lZC1vZmYtYnk6IFBhbGkgUm9ow6FyIAo+ID4gID4+IC0t LQo+ID4gID4+ICBhcmNoL2FybS9tYWNoLW9tYXAyL29tYXAtc2VjdXJlLmggfCAgICAxICsKPiA+ ICA+PiAgYXJjaC9hcm0vbWFjaC1vbWFwMi9vbWFwLXNtYy5TICAgIHwgICAyMiArKysrKysrKysr KysrKysrKysrKystCj4gPiAgPj4gIDIgZmlsZXMgY2hhbmdlZCwgMjIgaW5zZXJ0aW9ucygrKSwg MSBkZWxldGlvbigtKQo+ID4gID4+IAo+ID4gID4+IGRpZmYgLS1naXQgYS9hcmNoL2FybS9tYWNo LW9tYXAyL29tYXAtc2VjdXJlLmggYi9hcmNoL2FybS9tYWNoLW9tYXAyL29tYXAtc2VjdXJlLmgK PiA+ICA+PiBpbmRleCAwZTcyOTE3Li5jNDU4NmY0IDEwMDY0NAo+ID4gID4+IC0tLSBhL2FyY2gv YXJtL21hY2gtb21hcDIvb21hcC1zZWN1cmUuaAo+ID4gID4+ICsrKyBiL2FyY2gvYXJtL21hY2gt b21hcDIvb21hcC1zZWN1cmUuaAo+ID4gID4+IEBAIC01MSw2ICs1MSw3IEBACj4gPiAgPj4gIGV4 dGVybiB1MzIgb21hcF9zZWN1cmVfZGlzcGF0Y2hlcih1MzIgaWR4LCB1MzIgZmxhZywgdTMyIG5h cmdzLAo+ID4gID4+ICAJCQkJdTMyIGFyZzEsIHUzMiBhcmcyLCB1MzIgYXJnMywgdTMyIGFyZzQp Owo+ID4gID4+ICBleHRlcm4gdTMyIG9tYXBfc21jMih1MzIgaWQsIHUzMiBmYWxnLCB1MzIgcGFy Z3MpOwo+ID4gID4+ICtleHRlcm4gdTMyIG9tYXBfc21jMyh1MzIgaWQsIHUzMiBwcm9jZXNzLCB1 MzIgZmxhZywgdTMyIHBhcmdzKTsKPiA+ICA+PiAgZXh0ZXJuIHBoeXNfYWRkcl90IG9tYXBfc2Vj dXJlX3JhbV9tZW1wb29sX2Jhc2Uodm9pZCk7Cj4gPiAgPj4gIGV4dGVybiBpbnQgb21hcF9zZWN1 cmVfcmFtX3Jlc2VydmVfbWVtYmxvY2sodm9pZCk7Cj4gPiAgPj4gIAo+ID4gID4+IGRpZmYgLS1n aXQgYS9hcmNoL2FybS9tYWNoLW9tYXAyL29tYXAtc21jLlMgYi9hcmNoL2FybS9tYWNoLW9tYXAy L29tYXAtc21jLlMKPiA+ICA+PiBpbmRleCBmNjQ0MWMxLi41YzAyYjhkIDEwMDY0NAo+ID4gID4+ IC0tLSBhL2FyY2gvYXJtL21hY2gtb21hcDIvb21hcC1zbWMuUwo+ID4gID4+ICsrKyBiL2FyY2gv YXJtL21hY2gtb21hcDIvb21hcC1zbWMuUwo+ID4gID4+IEBAIC0xLDkgKzEsMTEgQEAKPiA+ICA+ PiAgLyoKPiA+ICA+PiAtICogT01BUDQ0eHggc2VjdXJlIEFQSXMgZmlsZS4KPiA+ICA+PiArICog T01BUDM0eHggYW5kIE9NQVA0NHh4IHNlY3VyZSBBUElzIGZpbGUuCj4gPiAgPj4gICAqCj4gPiAg Pj4gICAqIENvcHlyaWdodCAoQykgMjAxMCBUZXhhcyBJbnN0cnVtZW50cywgSW5jLgo+ID4gID4+ ICAgKiBXcml0dGVuIGJ5IFNhbnRvc2ggU2hpbGlta2FyIAo+ID4gID4+ICAgKgo+ID4gID4+ICsg KiBDb3B5cmlnaHQgKEMpIDIwMTIgSXZheWxvIERpbWl0cm92IAo+ID4gID4+ICsgKiBDb3B5cmln aHQgKEMpIDIwMTMgUGFsaSBSb2jDoXIgCj4gPiAgPj4gICAqCj4gPiAgPj4gICAqIFRoaXMgcHJv Z3JhbSBpcyBmcmVlIHNvZnR3YXJlLHlvdSBjYW4gcmVkaXN0cmlidXRlIGl0IGFuZC9vciBtb2Rp ZnkKPiA+ICA+PiAgICogaXQgdW5kZXIgdGhlIHRlcm1zIG9mIHRoZSBHTlUgR2VuZXJhbCBQdWJs aWMgTGljZW5zZSB2ZXJzaW9uIDIgYXMKPiA+ICA+PiBAQCAtNTQsNiArNTYsMjQgQEAgRU5UUlko b21hcF9zbWMyKQo+ID4gID4+ICAJbGRtZmQgICBzcCEsIHtyNC1yMTIsIHBjfQo+ID4gID4+ICBF TkRQUk9DKG9tYXBfc21jMikKPiA+ICA+PiAgCj4gPiAgPj4gKy8qKgo+ID4gID4+ICsgKiB1MzIg b21hcF9zbWMzKHUzMiBzZXJ2aWNlX2lkLCB1MzIgcHJvY2Vzc19pZCwgdTMyIGZsYWcsIHUzMiBw YXJncykKPiA+ICA+PiArICogTG93IGxldmVsIGNvbW1vbiByb3V0aW5lIGZvciBzZWN1cmUgSEFM IGFuZCBQUEEgQVBJcyB2aWEgc21jICMxCj4gPiAgPj4gKyAqIHIwIC0gQHNlcnZpY2VfaWQ6IFNl Y3VyZSBTZXJ2aWNlIElECj4gPiAgPj4gKyAqIHIxIC0gQHByb2Nlc3NfaWQ6IFByb2Nlc3MgSUQK PiA+ICA+PiArICogcjIgLSBAZmxhZzogRmxhZyB0byBpbmRpY2F0ZSB0aGUgY3JpdGljYWxpdHkg b2Ygb3BlcmF0aW9uCj4gPiAgPj4gKyAqIHIzIC0gQHBhcmdzOiBQaHlzaWNhbCBhZGRyZXNzIG9m IHBhcmFtZXRlciBsaXN0Cj4gPiAgPj4gKyAqLwo+ID4gID4+ICtFTlRSWShvbWFwX3NtYzMpCj4g PiAgPj4gKwlzdG1mZAlzcCEsIHtyNC1yMTIsIGxyfQo+ID4gID4KPiA+ICA+WW91IGRvbid0IG5l ZWQgdG8gc2F2ZS9yZXN0b3JlIHIxMi4gIFRoZSBBQkkgYWxsb3dzIGl0IHRvIGJlIGNsb2JiZXJl ZAo+ID4gID5hY3Jvc3MgZnVuY3Rpb24gY2FsbHMuCj4gPiAgPgo+ID4gID4+ICsJbW92CXIxMiwg cjAJCUAgQ29weSB0aGUgc2VjdXJlIHNlcnZpY2UgSUQKPiA+ICA+PiArCW1vdglyNiwgIzB4ZmYJ QCBJbmRpY2F0ZSBuZXcgVGFzayBjYWxsCj4gPiAgPj4gKwlkc2IKPiA+ICA+PiArCWRtYgo+ID4g ID4KPiA+ICA+ZHNiIHN5bmNocm9uaXNlcyBhIHN1cGVyc2V0IG9mIHdoYXQgZG1iIHN5bmNocm9u aXNlcywgc28gdGhlIGRtYiBoZXJlIGlzCj4gPiAgPm5vdCB1c2VmdWwuCj4gPiAgPgo+ID4gID5J biBhbnkgY2FzZSwgYW55IGNvZGUgY2FsbGluZyB0aGlzIG11c3QgZmx1c2ggdGhlIHJlZ2lvbiBh ZGRyZXNzZWQgYnkKPiA+ICA+cjMgYmVmb3JlaGFuZCBhbnl3YXksIHdoaWNoIHdpbGwgaW5jbHVk ZSBhIGRzYiBhcyBwYXJ0IG9mIGl0cyBzZW1hbnRpY3MKPiA+ICA+LS0gdGhpcyBpcyBob3cgeW91 IGNhbGwgaXQgZnJvbSByeDUxX3NlY3VyZV9kaXNwYXRjaGVyKCkuCj4gPiAgPgo+ID4gID5TbyBJ IHRoaW5rIHRoZSBkc2IgbWF5IG5vdCBiZSBuZWVkZWQgaGVyZSAoPykKPiA+ICA+Cj4gPiAgPkNo ZWVycwo+ID4gID4tLS1EYXZlCj4gPiAgPgo+ID4gID4KPiA+IAo+ID4gQ291bGQgYmUsIGJ1dCBJ IHdvbmRlciB3aHkgYWxtb3N0IGFsbCB0aGUga2VybmVsIGNvZGUoSSBhbSBhd2FyZSBvZikgdGhh dCB1c2VzIFNNQyBhbmQgaXMgd3JpdHRlbiBieSBUSSwgaXMgc3RvcmluZyByMTIgYW5kIGlzIHVz aW5nIGJvdGggRFNCIGFuZCBETUIuIFNlZSBhcmNoL2FybS9tYWNoLW9tYXAyL3NsZWVwMzR4eC5T IG9yIGFyY2gvYXJtL21hY2gtb21hcDIvb21hcC1zbWMuUyBmb3IgZXhhbXBsZXMuIEknZCByYXRo ZXIgcGxheSBpdCBzYWZlIGFuZCBsZWF2ZSBpdCB0aGF0IHdheSwgdW5sZXNzIHNvbWVvbmUgY29u ZmlybXMgdGhlIG90aGVyIGNvZGUgdXNpbmcgU01DIGhhcyBleHRyYSBEU0IvRE1CIGluc3RydWN0 aW9ucyB0b28uIEkgd291bGRuJ3QgcmlzayBwYXNzaW5nIGludmFsaWQvc3RhbGUgZGF0YSB0byB0 aGUgU2VjdXJlIE1vbml0b3IgdG8ganVzdCBzYXZlIDggYnl0ZXMgYW5kIGJhcnJpZXJzIGluIGEg cGVyZm9ybWFuY2Ugbm9uLWNyaXRpY2FsIGNvZGUgd2hpY2ggd2lsbCBiZSBjYWxsZWQgb25seSBh IGNvdXBsZSBvZiB0aW1lcyBkdXJpbmcgdGhlIGJvb3QtdXAgcHJvY2Vzcy4gcjEyIHNhdmUvcmVz dG9yZSBpcyBhIGxlZ2FjeSBmcm9tIG9tYXBfc21jMiBpbiBvbWFwLXNtYy5TLCBzbyBJIGd1ZXNz IGl0IGNhbiBnbyBhd2F5IHdpdGhvdXQgbXVjaCBvZiBhIHRyb3VibGUuCj4gPiAKPiBEYXZlIHBv aW50ZWQgb3V0IGFib3V0IHRoZSBkc2IgYW5kIHIxMiB0byBtZSBpbiBhbm90aGVyIHRocmVhZC4g UjEyIGNhbiBiZSBlYXNpbHkgcmVtb3ZlZAo+IGJ1dCB0aGUgRFNCJ3Mgd2VyZSBuZWVkZWQgb24g T01BUCBmb3IgcG93ZXIgc2VxdWVuY2luZy4gV2l0aG91dCB0aG9zZSwgd2UgaGF2ZSBzZWVuCj4g bWFueSBpc3N1ZXMuIFNvIHlvdSBjYW4gbGVhdmUgdGhlIGRzYidzIHRvIGJlIGNvbnNpc3RlbnQg d2l0aCByZXN0IG9mIHRoZSBjb2RlLgoKQ29uc2lzdGVuY3kgaXMgYSBwZXJmZWN0bHkgZ29vZCBy ZWFzb24sIGVzcGVjaWFsbHkgaW4gY29kZSBsaWtlIHRoaXMKd2hlcmUgYSBjZXJ0YWluIGNvZGUg c2VxdWVuY2UgaGFzIGJlZW4gcHJvdmVuLCBidXQgaXQncyBwcmVmZXJhYmxlIHRvCmluY2x1ZGUg YnJpZWYgY29tbWVudHMgdG8gZXhwbGFpbiBub25ldGhlbGVzcy4KCkRpZmZpY3VsdHkgaW4gZXhw bGFpbmluZyBwcmVjaXNlbHkgd2h5IHNvbWV0aGluZyBpcyBuZWVkZWQgc2hvdWxkIGJlIGEKd2Fy bmluZyBmbGFnIHRoYXQgYSBjb21tZW50IGlzIG5lZWRlZC4KCkNoZWVycwotLS1EYXZlCgoKX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtl cm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0 dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5l bAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave.Martin@arm.com (Dave Martin) Date: Fri, 12 Jul 2013 11:24:37 +0100 Subject: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 In-Reply-To: <51DF0D7A.4090008@ti.com> References: <531356997.66774.1373571839852.JavaMail.apache@mail81.abv.bg> <51DF0D7A.4090008@ti.com> Message-ID: <20130712102430.GA3249@localhost.localdomain> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Jul 11, 2013 at 03:54:34PM -0400, Santosh Shilimkar wrote: > On Thursday 11 July 2013 03:43 PM, ?????? ???????? wrote: > > > > > > > > > > > > >-------- ?????????? ????? -------- > > >??: Dave Martin > > >???????: Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which > > calling instruction smc #1 > > >??: Pali Roh?r > > >????????? ??: ?????, 2013, ??? 10 20:45:26 EEST > > > > > > > > >On Wed, Jul 10, 2013 at 02:59:04PM +0200, Pali Roh?r wrote: > > >> Other secure functions omap_smc1() and omap_smc2() calling instruction smc #0 > > >> but Nokia RX-51 board needs to call smc #1 for PPA access. > > >> > > >> Signed-off-by: Ivaylo Dimitrov > > >> Signed-off-by: Pali Roh?r > > >> --- > > >> arch/arm/mach-omap2/omap-secure.h | 1 + > > >> arch/arm/mach-omap2/omap-smc.S | 22 +++++++++++++++++++++- > > >> 2 files changed, 22 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h > > >> index 0e72917..c4586f4 100644 > > >> --- a/arch/arm/mach-omap2/omap-secure.h > > >> +++ b/arch/arm/mach-omap2/omap-secure.h > > >> @@ -51,6 +51,7 @@ > > >> extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, > > >> u32 arg1, u32 arg2, u32 arg3, u32 arg4); > > >> extern u32 omap_smc2(u32 id, u32 falg, u32 pargs); > > >> +extern u32 omap_smc3(u32 id, u32 process, u32 flag, u32 pargs); > > >> extern phys_addr_t omap_secure_ram_mempool_base(void); > > >> extern int omap_secure_ram_reserve_memblock(void); > > >> > > >> diff --git a/arch/arm/mach-omap2/omap-smc.S b/arch/arm/mach-omap2/omap-smc.S > > >> index f6441c1..5c02b8d 100644 > > >> --- a/arch/arm/mach-omap2/omap-smc.S > > >> +++ b/arch/arm/mach-omap2/omap-smc.S > > >> @@ -1,9 +1,11 @@ > > >> /* > > >> - * OMAP44xx secure APIs file. > > >> + * OMAP34xx and OMAP44xx secure APIs file. > > >> * > > >> * Copyright (C) 2010 Texas Instruments, Inc. > > >> * Written by Santosh Shilimkar > > >> * > > >> + * Copyright (C) 2012 Ivaylo Dimitrov > > >> + * Copyright (C) 2013 Pali Roh?r > > >> * > > >> * This program is free software,you can redistribute it and/or modify > > >> * it under the terms of the GNU General Public License version 2 as > > >> @@ -54,6 +56,24 @@ ENTRY(omap_smc2) > > >> ldmfd sp!, {r4-r12, pc} > > >> ENDPROC(omap_smc2) > > >> > > >> +/** > > >> + * u32 omap_smc3(u32 service_id, u32 process_id, u32 flag, u32 pargs) > > >> + * Low level common routine for secure HAL and PPA APIs via smc #1 > > >> + * r0 - @service_id: Secure Service ID > > >> + * r1 - @process_id: Process ID > > >> + * r2 - @flag: Flag to indicate the criticality of operation > > >> + * r3 - @pargs: Physical address of parameter list > > >> + */ > > >> +ENTRY(omap_smc3) > > >> + stmfd sp!, {r4-r12, lr} > > > > > >You don't need to save/restore r12. The ABI allows it to be clobbered > > >across function calls. > > > > > >> + mov r12, r0 @ Copy the secure service ID > > >> + mov r6, #0xff @ Indicate new Task call > > >> + dsb > > >> + dmb > > > > > >dsb synchronises a superset of what dmb synchronises, so the dmb here is > > >not useful. > > > > > >In any case, any code calling this must flush the region addressed by > > >r3 beforehand anyway, which will include a dsb as part of its semantics > > >-- this is how you call it from rx51_secure_dispatcher(). > > > > > >So I think the dsb may not be needed here (?) > > > > > >Cheers > > >---Dave > > > > > > > > > > Could be, but I wonder why almost all the kernel code(I am aware of) that uses SMC and is written by TI, is storing r12 and is using both DSB and DMB. See arch/arm/mach-omap2/sleep34xx.S or arch/arm/mach-omap2/omap-smc.S for examples. I'd rather play it safe and leave it that way, unless someone confirms the other code using SMC has extra DSB/DMB instructions too. I wouldn't risk passing invalid/stale data to the Secure Monitor to just save 8 bytes and barriers in a performance non-critical code which will be called only a couple of times during the boot-up process. r12 save/restore is a legacy from omap_smc2 in omap-smc.S, so I guess it can go away without much of a trouble. > > > Dave pointed out about the dsb and r12 to me in another thread. R12 can be easily removed > but the DSB's were needed on OMAP for power sequencing. Without those, we have seen > many issues. So you can leave the dsb's to be consistent with rest of the code. Consistency is a perfectly good reason, especially in code like this where a certain code sequence has been proven, but it's preferable to include brief comments to explain nonetheless. Difficulty in explaining precisely why something is needed should be a warning flag that a comment is needed. Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757458Ab3GLK0E (ORCPT ); Fri, 12 Jul 2013 06:26:04 -0400 Received: from fw-tnat.cambridge.arm.com ([217.140.96.21]:54122 "EHLO cam-smtp0.cambridge.arm.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757244Ab3GLK0B (ORCPT ); Fri, 12 Jul 2013 06:26:01 -0400 Date: Fri, 12 Jul 2013 11:24:37 +0100 From: Dave Martin To: Santosh Shilimkar Cc: =?utf-8?B?0JjQstCw0LnQu9C+INCU0LjQvNC40YLRgNC+0LI=?= , nm@ti.com, linux@arm.linux.org.uk, aaro.koskinen@iki.fi, tony@atomide.com, pdeschrijver@nvidia.com, linux-kernel@vger.kernel.org, pavel@ucw.cz, pali.rohar@gmail.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Message-ID: <20130712102430.GA3249@localhost.localdomain> References: <531356997.66774.1373571839852.JavaMail.apache@mail81.abv.bg> <51DF0D7A.4090008@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <51DF0D7A.4090008@ti.com> 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 Thu, Jul 11, 2013 at 03:54:34PM -0400, Santosh Shilimkar wrote: > On Thursday 11 July 2013 03:43 PM, Ивайло Димитров wrote: > > > > > > > > > > > > >-------- Оригинално писмо -------- > > >От: Dave Martin > > >Относно: Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which > > calling instruction smc #1 > > >До: Pali Rohár > > >Изпратено на: Сряда, 2013, Юли 10 20:45:26 EEST > > > > > > > > >On Wed, Jul 10, 2013 at 02:59:04PM +0200, Pali Rohár wrote: > > >> Other secure functions omap_smc1() and omap_smc2() calling instruction smc #0 > > >> but Nokia RX-51 board needs to call smc #1 for PPA access. > > >> > > >> Signed-off-by: Ivaylo Dimitrov > > >> Signed-off-by: Pali Rohár > > >> --- > > >> arch/arm/mach-omap2/omap-secure.h | 1 + > > >> arch/arm/mach-omap2/omap-smc.S | 22 +++++++++++++++++++++- > > >> 2 files changed, 22 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h > > >> index 0e72917..c4586f4 100644 > > >> --- a/arch/arm/mach-omap2/omap-secure.h > > >> +++ b/arch/arm/mach-omap2/omap-secure.h > > >> @@ -51,6 +51,7 @@ > > >> extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, > > >> u32 arg1, u32 arg2, u32 arg3, u32 arg4); > > >> extern u32 omap_smc2(u32 id, u32 falg, u32 pargs); > > >> +extern u32 omap_smc3(u32 id, u32 process, u32 flag, u32 pargs); > > >> extern phys_addr_t omap_secure_ram_mempool_base(void); > > >> extern int omap_secure_ram_reserve_memblock(void); > > >> > > >> diff --git a/arch/arm/mach-omap2/omap-smc.S b/arch/arm/mach-omap2/omap-smc.S > > >> index f6441c1..5c02b8d 100644 > > >> --- a/arch/arm/mach-omap2/omap-smc.S > > >> +++ b/arch/arm/mach-omap2/omap-smc.S > > >> @@ -1,9 +1,11 @@ > > >> /* > > >> - * OMAP44xx secure APIs file. > > >> + * OMAP34xx and OMAP44xx secure APIs file. > > >> * > > >> * Copyright (C) 2010 Texas Instruments, Inc. > > >> * Written by Santosh Shilimkar > > >> * > > >> + * Copyright (C) 2012 Ivaylo Dimitrov > > >> + * Copyright (C) 2013 Pali Rohár > > >> * > > >> * This program is free software,you can redistribute it and/or modify > > >> * it under the terms of the GNU General Public License version 2 as > > >> @@ -54,6 +56,24 @@ ENTRY(omap_smc2) > > >> ldmfd sp!, {r4-r12, pc} > > >> ENDPROC(omap_smc2) > > >> > > >> +/** > > >> + * u32 omap_smc3(u32 service_id, u32 process_id, u32 flag, u32 pargs) > > >> + * Low level common routine for secure HAL and PPA APIs via smc #1 > > >> + * r0 - @service_id: Secure Service ID > > >> + * r1 - @process_id: Process ID > > >> + * r2 - @flag: Flag to indicate the criticality of operation > > >> + * r3 - @pargs: Physical address of parameter list > > >> + */ > > >> +ENTRY(omap_smc3) > > >> + stmfd sp!, {r4-r12, lr} > > > > > >You don't need to save/restore r12. The ABI allows it to be clobbered > > >across function calls. > > > > > >> + mov r12, r0 @ Copy the secure service ID > > >> + mov r6, #0xff @ Indicate new Task call > > >> + dsb > > >> + dmb > > > > > >dsb synchronises a superset of what dmb synchronises, so the dmb here is > > >not useful. > > > > > >In any case, any code calling this must flush the region addressed by > > >r3 beforehand anyway, which will include a dsb as part of its semantics > > >-- this is how you call it from rx51_secure_dispatcher(). > > > > > >So I think the dsb may not be needed here (?) > > > > > >Cheers > > >---Dave > > > > > > > > > > Could be, but I wonder why almost all the kernel code(I am aware of) that uses SMC and is written by TI, is storing r12 and is using both DSB and DMB. See arch/arm/mach-omap2/sleep34xx.S or arch/arm/mach-omap2/omap-smc.S for examples. I'd rather play it safe and leave it that way, unless someone confirms the other code using SMC has extra DSB/DMB instructions too. I wouldn't risk passing invalid/stale data to the Secure Monitor to just save 8 bytes and barriers in a performance non-critical code which will be called only a couple of times during the boot-up process. r12 save/restore is a legacy from omap_smc2 in omap-smc.S, so I guess it can go away without much of a trouble. > > > Dave pointed out about the dsb and r12 to me in another thread. R12 can be easily removed > but the DSB's were needed on OMAP for power sequencing. Without those, we have seen > many issues. So you can leave the dsb's to be consistent with rest of the code. Consistency is a perfectly good reason, especially in code like this where a certain code sequence has been proven, but it's preferable to include brief comments to explain nonetheless. Difficulty in explaining precisely why something is needed should be a warning flag that a comment is needed. Cheers ---Dave