linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-07-10 12:59 ` [PATCH v2 0/2] " Pali Rohár
@ 2013-07-10 12:59   ` Pali Rohár
  2013-07-10 17:45     ` Dave Martin
  0 siblings, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2013-07-10 12:59 UTC (permalink / raw)
  To: linux-kernel, Tony Lindgren, Aaro Koskinen
  Cc: linux-omap, linux, linux-arm-kernel, Nishanth Menon, Pavel Machek,
	Peter De Schrijver, Santosh Shilimkar, Ivaylo Dimitrov,
	Pali Rohár

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 <freemangordon@abv.bg>
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 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 <santosh.shilimkar@ti.com>
  *
+ * Copyright (C) 2012 Ivaylo Dimitrov <freemangordon@abv.bg>
+ * Copyright (C) 2013 Pali Rohár <pali.rohar@gmail.com>
  *
  * 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}
+	mov	r12, r0		@ Copy the secure service ID
+	mov	r6, #0xff	@ Indicate new Task call
+	dsb
+	dmb
+	smc	#1		@ call PPA service
+	ldmfd	sp!, {r4-r12, pc}
+ENDPROC(omap_smc3)
+
 ENTRY(omap_modify_auxcoreboot0)
 	stmfd   sp!, {r1-r12, lr}
 	ldr	r12, =0x104
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-07-10 12:59   ` [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Pali Rohár
@ 2013-07-10 17:45     ` Dave Martin
  0 siblings, 0 replies; 7+ messages in thread
From: Dave Martin @ 2013-07-10 17:45 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Nishanth Menon, linux, Aaro Koskinen, Tony Lindgren,
	Peter De Schrijver, linux-kernel, Ivaylo Dimitrov,
	Santosh Shilimkar, Pavel Machek, linux-omap, linux-arm-kernel

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 <freemangordon@abv.bg>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  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 <santosh.shilimkar@ti.com>
>   *
> + * Copyright (C) 2012 Ivaylo Dimitrov <freemangordon@abv.bg>
> + * Copyright (C) 2013 Pali Rohár <pali.rohar@gmail.com>
>   *
>   * 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
@ 2013-07-11 19:43 Ивайло Димитров
  2013-07-11 19:54 ` Santosh Shilimkar
  0 siblings, 1 reply; 7+ messages in thread
From: Ивайло Димитров @ 2013-07-11 19:43 UTC (permalink / raw)
  To: Dave Martin
  Cc: pali.rohar, linux-kernel, tony, aaro.koskinen, nm, linux,
	pdeschrijver, santosh.shilimkar, pavel, linux-omap,
	linux-arm-kernel

 




 >-------- Оригинално писмо --------
 >От:  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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-07-11 19:43 [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Ивайло Димитров
@ 2013-07-11 19:54 ` Santosh Shilimkar
  2013-07-12 10:24   ` Dave Martin
  0 siblings, 1 reply; 7+ messages in thread
From: Santosh Shilimkar @ 2013-07-11 19:54 UTC (permalink / raw)
  To: Ивайло Димитров
  Cc: Dave Martin, pali.rohar, linux-kernel, tony, aaro.koskinen, nm,
	linux, pdeschrijver, pavel, linux-omap, linux-arm-kernel

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.

Regards
Santosh
P.S: Please sensibly wrap your message while replying. You reply apears like
one single line and I needed to keep scrolling to read it.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-07-11 19:54 ` Santosh Shilimkar
@ 2013-07-12 10:24   ` Dave Martin
  2013-08-04  8:45     ` [PATCH v3 " Pali Rohár
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Martin @ 2013-07-12 10:24 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: nm, linux, aaro.koskinen, tony, pdeschrijver, linux-kernel,
	Ивайло Димитров,
	pavel, pali.rohar, linux-omap, linux-arm-kernel

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-07-12 10:24   ` Dave Martin
@ 2013-08-04  8:45     ` Pali Rohár
  2013-08-05 13:29       ` Dave Martin
  0 siblings, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2013-08-04  8:45 UTC (permalink / raw)
  To: tony
  Cc: Dave Martin, Santosh Shilimkar,
	Ивайло Димитров,
	nm, linux, aaro.koskinen, pdeschrijver, linux-kernel, pavel,
	linux-omap, linux-arm-kernel

[-- Attachment #1: Type: Text/Plain, Size: 2341 bytes --]

Here is new version (v3) of omap secure part patch:

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 <freemangordon@abv.bg>
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
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..7bbc043 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 <santosh.shilimkar@ti.com>
  *
+ * Copyright (C) 2012 Ivaylo Dimitrov <freemangordon@abv.bg>
+ * Copyright (C) 2013 Pali Rohár <pali.rohar@gmail.com>
  *
  * 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,23 @@ 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-r11, lr}
+	mov	r12, r0		@ Copy the secure service ID
+	mov	r6, #0xff	@ Indicate new Task call
+	dsb			@ Memory Barrier
+	smc	#1		@ Call PPA service
+	ldmfd	sp!, {r4-r11, pc}
+ENDPROC(omap_smc3)
+
 ENTRY(omap_modify_auxcoreboot0)
 	stmfd   sp!, {r1-r12, lr}
 	ldr	r12, =0x104

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1
  2013-08-04  8:45     ` [PATCH v3 " Pali Rohár
@ 2013-08-05 13:29       ` Dave Martin
  0 siblings, 0 replies; 7+ messages in thread
From: Dave Martin @ 2013-08-05 13:29 UTC (permalink / raw)
  To: Pali Rohár
  Cc: nm, linux, aaro.koskinen, tony, pdeschrijver, linux-kernel,
	Ивайло Димитров,
	Santosh Shilimkar, pavel, linux-omap, linux-arm-kernel

On Sun, Aug 04, 2013 at 10:45:00AM +0200, Pali Rohár wrote:
> Here is new version (v3) of omap secure part patch:
> 
> 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 <freemangordon@abv.bg>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> 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..7bbc043 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 <santosh.shilimkar@ti.com>
>   *
> + * Copyright (C) 2012 Ivaylo Dimitrov <freemangordon@abv.bg>
> + * Copyright (C) 2013 Pali Rohár <pali.rohar@gmail.com>
>   *
>   * 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,23 @@ 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-r11, lr}
> +	mov	r12, r0		@ Copy the secure service ID
> +	mov	r6, #0xff	@ Indicate new Task call
> +	dsb			@ Memory Barrier

Can you explain _why_ the barrier is there?  The reader doesn't need to
be told that a barrier instruction is a barrier instruction.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-08-05 13:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-11 19:43 [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Ивайло Димитров
2013-07-11 19:54 ` Santosh Shilimkar
2013-07-12 10:24   ` Dave Martin
2013-08-04  8:45     ` [PATCH v3 " Pali Rohár
2013-08-05 13:29       ` Dave Martin
  -- strict thread matches above, loose matches on Subject: below --
2013-02-28  9:42 [PATCH] arm: omap: RX-51: ARM errata 430973 workaround Pali Rohár
2013-07-10 12:59 ` [PATCH v2 0/2] " Pali Rohár
2013-07-10 12:59   ` [PATCH v2 1/2] ARM: OMAP: Add secure function omap_smc3() which calling instruction smc #1 Pali Rohár
2013-07-10 17:45     ` Dave Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).