[v3,2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()
diff mbox series

Message ID 20191230185004.32279-3-afd@ti.com
State New
Headers show
Series
  • Use ARM SMC Calling Convention when OP-TEE is available
Related show

Commit Message

Andrew F. Davis Dec. 30, 2019, 6:50 p.m. UTC
This check and associated flag can be used to signal the presence
of OP-TEE on the platform. This can be used to determine which
SMC calls to make to perform secure operations.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
 arch/arm/mach-omap2/omap-secure.h |  3 +++
 2 files changed, 17 insertions(+)

Comments

Lokesh Vutla Dec. 31, 2019, 6:32 a.m. UTC | #1
On 31/12/19 12:20 AM, Andrew F. Davis wrote:
> This check and associated flag can be used to signal the presence
> of OP-TEE on the platform. This can be used to determine which
> SMC calls to make to perform secure operations.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
> index e936732cdc4f..39d8070aede6 100644
> --- a/arch/arm/mach-omap2/omap-secure.c
> +++ b/arch/arm/mach-omap2/omap-secure.c
> @@ -12,6 +12,7 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/memblock.h>
> +#include <linux/of.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/memblock.h>
> @@ -20,6 +21,18 @@
>  
>  static phys_addr_t omap_secure_memblock_base;
>  
> +bool optee_available;
> +
> +static void __init omap_optee_init_check(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_node_by_path("/firmware/optee");
> +	if (np && of_device_is_available(np))

This doesn't guarantee that optee driver is probed successfully or firmware
installed correctly. Isn't there a better way to detect? Doesn't tee core layer
exposes anything?

Thanks and regards,
Lokesh

> +		optee_available = true;
> +	of_node_put(np);
> +}
> +
>  /**
>   * omap_sec_dispatcher: Routine to dispatch low power secure
>   * service routines
> @@ -166,4 +179,5 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
>  
>  void __init omap_secure_init(void)
>  {
> +	omap_optee_init_check();
>  }
> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
> index 9aeeb236a224..78a1c4f04bbe 100644
> --- a/arch/arm/mach-omap2/omap-secure.h
> +++ b/arch/arm/mach-omap2/omap-secure.h
> @@ -10,6 +10,8 @@
>  #ifndef OMAP_ARCH_OMAP_SECURE_H
>  #define OMAP_ARCH_OMAP_SECURE_H
>  
> +#include <linux/types.h>
> +
>  /* Monitor error code */
>  #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
>  #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
> @@ -72,6 +74,7 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
>  extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
>  extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
>  
> +extern bool optee_available;
>  void omap_secure_init(void);
>  
>  #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
>
Andrew F. Davis Dec. 31, 2019, 2:15 p.m. UTC | #2
On 12/31/19 1:32 AM, Lokesh Vutla wrote:
> 
> 
> On 31/12/19 12:20 AM, Andrew F. Davis wrote:
>> This check and associated flag can be used to signal the presence
>> of OP-TEE on the platform. This can be used to determine which
>> SMC calls to make to perform secure operations.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>  arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
>>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
>> index e936732cdc4f..39d8070aede6 100644
>> --- a/arch/arm/mach-omap2/omap-secure.c
>> +++ b/arch/arm/mach-omap2/omap-secure.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/memblock.h>
>> +#include <linux/of.h>
>>  
>>  #include <asm/cacheflush.h>
>>  #include <asm/memblock.h>
>> @@ -20,6 +21,18 @@
>>  
>>  static phys_addr_t omap_secure_memblock_base;
>>  
>> +bool optee_available;
>> +
>> +static void __init omap_optee_init_check(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	np = of_find_node_by_path("/firmware/optee");
>> +	if (np && of_device_is_available(np))
> 
> This doesn't guarantee that optee driver is probed successfully or firmware
> installed correctly. Isn't there a better way to detect? Doesn't tee core layer
> exposes anything?


We don't actually need the kernel-side OP-TEE driver at all here, we are
making raw SMCCC calls which get handled by OP-TEE using platform
specific code then emulates the function previously handled by ROM[0]
and execution is returned. No driver involved for these types of calls.

U-Boot will not add this node to the DT unless OP-TEE is installed
correctly, but you are right that is no perfect guarantee. OP-TEE's
kernel driver does do a handshake to verify it is working but this is
not exposed outside of that driver and happens *way* too late for our
uses here. Plus as above, we don't need the OP-TEE driver at all and we
should boot the same without it even enabled.

So my opinion is that if DT says OP-TEE is installed, but it is not,
then that is a misconfiguration and we usually just have to trust DT for
most things. If DT is wrong here then the only thing that happens is
this call safely fails, a message is printed informing the user of the
problem, and kernel keeps booting (although probably not stable given we
need these calls for important system configuration).

Andrew

[0]
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a9.c
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a15.c


> 
> Thanks and regards,
> Lokesh
> 
>> +		optee_available = true;
>> +	of_node_put(np);
>> +}
>> +
>>  /**
>>   * omap_sec_dispatcher: Routine to dispatch low power secure
>>   * service routines
>> @@ -166,4 +179,5 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
>>  
>>  void __init omap_secure_init(void)
>>  {
>> +	omap_optee_init_check();
>>  }
>> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
>> index 9aeeb236a224..78a1c4f04bbe 100644
>> --- a/arch/arm/mach-omap2/omap-secure.h
>> +++ b/arch/arm/mach-omap2/omap-secure.h
>> @@ -10,6 +10,8 @@
>>  #ifndef OMAP_ARCH_OMAP_SECURE_H
>>  #define OMAP_ARCH_OMAP_SECURE_H
>>  
>> +#include <linux/types.h>
>> +
>>  /* Monitor error code */
>>  #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
>>  #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
>> @@ -72,6 +74,7 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
>>  extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
>>  extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
>>  
>> +extern bool optee_available;
>>  void omap_secure_init(void);
>>  
>>  #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
>>
Tony Lindgren Jan. 2, 2020, 5:14 p.m. UTC | #3
* Andrew F. Davis <afd@ti.com> [191231 14:16]:
> On 12/31/19 1:32 AM, Lokesh Vutla wrote:
> > This doesn't guarantee that optee driver is probed successfully or firmware
> > installed correctly. Isn't there a better way to detect? Doesn't tee core layer
> > exposes anything?
> 
> We don't actually need the kernel-side OP-TEE driver at all here, we are
> making raw SMCCC calls which get handled by OP-TEE using platform
> specific code then emulates the function previously handled by ROM[0]
> and execution is returned. No driver involved for these types of calls.
> 
> U-Boot will not add this node to the DT unless OP-TEE is installed
> correctly, but you are right that is no perfect guarantee. OP-TEE's
> kernel driver does do a handshake to verify it is working but this is
> not exposed outside of that driver and happens *way* too late for our
> uses here. Plus as above, we don't need the OP-TEE driver at all and we
> should boot the same without it even enabled.
> 
> So my opinion is that if DT says OP-TEE is installed, but it is not,
> then that is a misconfiguration and we usually just have to trust DT for
> most things. If DT is wrong here then the only thing that happens is
> this call safely fails, a message is printed informing the user of the
> problem, and kernel keeps booting (although probably not stable given we
> need these calls for important system configuration).

OK, please add comments to omap_optee_init_check(), it's not obvious
to anybody not dealing with optee directly.

Regards,

Tony
Andrew F. Davis Jan. 2, 2020, 5:24 p.m. UTC | #4
On 1/2/20 12:14 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191231 14:16]:
>> On 12/31/19 1:32 AM, Lokesh Vutla wrote:
>>> This doesn't guarantee that optee driver is probed successfully or firmware
>>> installed correctly. Isn't there a better way to detect? Doesn't tee core layer
>>> exposes anything?
>>
>> We don't actually need the kernel-side OP-TEE driver at all here, we are
>> making raw SMCCC calls which get handled by OP-TEE using platform
>> specific code then emulates the function previously handled by ROM[0]
>> and execution is returned. No driver involved for these types of calls.
>>
>> U-Boot will not add this node to the DT unless OP-TEE is installed
>> correctly, but you are right that is no perfect guarantee. OP-TEE's
>> kernel driver does do a handshake to verify it is working but this is
>> not exposed outside of that driver and happens *way* too late for our
>> uses here. Plus as above, we don't need the OP-TEE driver at all and we
>> should boot the same without it even enabled.
>>
>> So my opinion is that if DT says OP-TEE is installed, but it is not,
>> then that is a misconfiguration and we usually just have to trust DT for
>> most things. If DT is wrong here then the only thing that happens is
>> this call safely fails, a message is printed informing the user of the
>> problem, and kernel keeps booting (although probably not stable given we
>> need these calls for important system configuration).
> 
> OK, please add comments to omap_optee_init_check(), it's not obvious
> to anybody not dealing with optee directly.
> 


Okay, will add this comment and the one suggested by Lokesh for v4.

Andrew


> Regards,
> 
> Tony
>

Patch
diff mbox series

diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index e936732cdc4f..39d8070aede6 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -12,6 +12,7 @@ 
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/memblock.h>
+#include <linux/of.h>
 
 #include <asm/cacheflush.h>
 #include <asm/memblock.h>
@@ -20,6 +21,18 @@ 
 
 static phys_addr_t omap_secure_memblock_base;
 
+bool optee_available;
+
+static void __init omap_optee_init_check(void)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/firmware/optee");
+	if (np && of_device_is_available(np))
+		optee_available = true;
+	of_node_put(np);
+}
+
 /**
  * omap_sec_dispatcher: Routine to dispatch low power secure
  * service routines
@@ -166,4 +179,5 @@  u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
 
 void __init omap_secure_init(void)
 {
+	omap_optee_init_check();
 }
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 9aeeb236a224..78a1c4f04bbe 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -10,6 +10,8 @@ 
 #ifndef OMAP_ARCH_OMAP_SECURE_H
 #define OMAP_ARCH_OMAP_SECURE_H
 
+#include <linux/types.h>
+
 /* Monitor error code */
 #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
 #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
@@ -72,6 +74,7 @@  extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
 extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
 extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
 
+extern bool optee_available;
 void omap_secure_init(void);
 
 #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER