[PATCH 1/2] ARM: smccc-call: Use r12 to route secure monitor calls on TI platforms

[PATCH 1/2] ARM: smccc-call: Use r12 to route secure monitor calls on TI platforms

[Mark Rutland]

On Mon, Sep 18, 2017 at 03:50:04PM -0500, Andrew F. Davis wrote:
> Our ROM Secure Monitor(SM) uses the value in r12 to determine which
> service is being requested by an SMC call. This inline with the ARM
> recommended SMC Calling Convention(SMCCC), which partitions the values
> in R0 for this task, OP-TEE's SM follows the ARM recommended convention.

I can't parse this last sentence. What exactly is inline with the SMCCC?

AFAICT, the SMCCC says r12 is "The Intra-Procedure-call scratch
register", which is not a paramter, service ID, etc.

> We need a way to signal that a call is for the OP-TEE SM and not for
> the ROM SM in a way that is safe for the ROM SM, in case it is still
> present. We do this by putting a value of 0x200 in r12 when the call
> is for OP-TEE or any other ARM by modifying the SMCCC caller function.

So IIUC, you have a secure monitor which is not SMCCC compliant, but you
want to use it at the same time as OP-TEE, which is SMCCC compliant.

It would be much better to have a new version of that monitor that was
SMCCC compliant, and have to update the code for that, than to have to
move OP-TEE away from standards compliance.

> There are four combinations of events:
> 
> If the ROM SM is present and we make a legacy style SMC call, as we
> do in early boot, the call will not have r12 set to 0x200 as these
> calls go through existing mach-omap2/ SMC handlers, so all is well.
> 
> If the ROM SM is present and we make an SMCCC style call, r12 will be
> set to 0x200 and ROM SM will see this as an invalid service call and
> safely return to the normal world.

So you're going to describe OP-TEE as present even when it's not!?

That's simply wrong.

> If OP-TEE is present and we make a legacy style SMC call, r12 will
> not be set to 0x200, and OP-TEE will emulate the functionality that
> the call is requesting.

AFAICT this means that OP-TEE completely replaces your existing monitor.

Please fix this properly. Implement SMCCC-compliant versions of those
calls you wish to retain, and come up with a new binding for those.
Describe that in the DT for systems which have OP-TEE providing these
services.

Systems with the old monitor should have that descrbied in their DT, and
not OP-TEE.

That completely removes the need to come up with a new OP-TEE binding
extension, and aligns your FW for forward compatibility with future
services.

Thanks,
Mark.

[PATCH 1/2] ARM: smccc-call: Use r12 to route secure monitor calls on TI platforms

[Andrew F. Davis]

On 09/19/2017 09:19 AM, Mark Rutland wrote:
> On Mon, Sep 18, 2017 at 03:50:04PM -0500, Andrew F. Davis wrote:
>> Our ROM Secure Monitor(SM) uses the value in r12 to determine which
>> service is being requested by an SMC call. This inline with the ARM
>> recommended SMC Calling Convention(SMCCC), which partitions the values
>> in R0 for this task, OP-TEE's SM follows the ARM recommended convention.
> 
> I can't parse this last sentence. What exactly is inline with the SMCCC?
> 

My bad, I meant "This 'is not' inline with the ARM..."

> AFAICT, the SMCCC says r12 is "The Intra-Procedure-call scratch
> register", which is not a paramter, service ID, etc.
> 

Right, which is why our old secure monitor is not inline with the SMCCC,
it uses r12 and not r0.

>> We need a way to signal that a call is for the OP-TEE SM and not for
>> the ROM SM in a way that is safe for the ROM SM, in case it is still
>> present. We do this by putting a value of 0x200 in r12 when the call
>> is for OP-TEE or any other ARM by modifying the SMCCC caller function.
> 
> So IIUC, you have a secure monitor which is not SMCCC compliant, but you
> want to use it at the same time as OP-TEE, which is SMCCC compliant.
> 

Not at the same time, our old secure monitor is in ROM, it will be
present anytime OP-TEE is not installed. OP-TEE is not mandatory, and so
any call to the secure side must be safe for both secure monitors.

> It would be much better to have a new version of that monitor that was
> SMCCC compliant, and have to update the code for that, than to have to
> move OP-TEE away from standards compliance.
> 

The current monitor is part of ROM and dates back to OMAP3 days, so
looks to pre-date ARM standardizing the SMCCC by several years, so I
hope our lack of compliance here can be forgiven here..

Luckily, OP-TEE does not have to move away from compliance to support
our platforms, it only needs to check for this sentinel value in r12,
which it already has support upstream for:

https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a15.c#L34

>> There are four combinations of events:
>>
>> If the ROM SM is present and we make a legacy style SMC call, as we
>> do in early boot, the call will not have r12 set to 0x200 as these
>> calls go through existing mach-omap2/ SMC handlers, so all is well.
>>
>> If the ROM SM is present and we make an SMCCC style call, r12 will be
>> set to 0x200 and ROM SM will see this as an invalid service call and
>> safely return to the normal world.
> 
> So you're going to describe OP-TEE as present even when it's not!?
> 
> That's simply wrong.
> 

This particular case should never happen as we only add the OP-TEE DT
node after having installed OP-TEE during boot. It is listed here only
for completeness and to show that it is safe even if it did occur.

>> If OP-TEE is present and we make a legacy style SMC call, r12 will
>> not be set to 0x200, and OP-TEE will emulate the functionality that
>> the call is requesting.
> 
> AFAICT this means that OP-TEE completely replaces your existing monitor.
> 

Correct.

> Please fix this properly. Implement SMCCC-compliant versions of those
> calls you wish to retain, and come up with a new binding for those.
> Describe that in the DT for systems which have OP-TEE providing these
> services.
> 

This might not be possible, these old SMCs are made very early and are
used all over our existing platform code:

git grep "smc" -- arch/arm/mach-omap2

> Systems with the old monitor should have that descrbied in their DT, and
> not OP-TEE.
> 

All systems have the old monitor by default, OP-TEE is the new and
optional case, I think its node should be the entity modified when
adding something OP-TEE specific like this.

> That completely removes the need to come up with a new OP-TEE binding
> extension, and aligns your FW for forward compatibility with future
> services.
> 

I agree this would be nice, but due to the above this may simply not be
reasonably possible.

Thanks,
Andrew

> Thanks,
> Mark

[PATCH 1/2] ARM: smccc-call: Use r12 to route secure monitor calls on TI platforms

[Mark Rutland]

On Tue, Sep 19, 2017 at 10:07:39AM -0500, Andrew F. Davis wrote:
> On 09/19/2017 09:19 AM, Mark Rutland wrote:
> > On Mon, Sep 18, 2017 at 03:50:04PM -0500, Andrew F. Davis wrote:
> >> We need a way to signal that a call is for the OP-TEE SM and not for
> >> the ROM SM in a way that is safe for the ROM SM, in case it is still
> >> present. We do this by putting a value of 0x200 in r12 when the call
> >> is for OP-TEE or any other ARM by modifying the SMCCC caller function.
> > 
> > So IIUC, you have a secure monitor which is not SMCCC compliant, but you
> > want to use it at the same time as OP-TEE, which is SMCCC compliant.
> 
> Not at the same time, our old secure monitor is in ROM, it will be
> present anytime OP-TEE is not installed. OP-TEE is not mandatory, and so
> any call to the secure side must be safe for both secure monitors.
> 
> > It would be much better to have a new version of that monitor that was
> > SMCCC compliant, and have to update the code for that, than to have to
> > move OP-TEE away from standards compliance.
> 
> The current monitor is part of ROM and dates back to OMAP3 days, so
> looks to pre-date ARM standardizing the SMCCC by several years, so I
> hope our lack of compliance here can be forgiven here..

For the existing ROM, as that stands, certainly. I appreciate it's older
than SMCCC.

> Luckily, OP-TEE does not have to move away from compliance to support
> our platforms, it only needs to check for this sentinel value in r12,
> which it already has support upstream for:
> 
> https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a15.c#L34

That *is* a move away from compliance, as an SMCCC-compliant TEE should
not look at r12, and an SMCCC-compliant client can (and will) leave
arbitrary values in r12.

[...]

> >> If OP-TEE is present and we make a legacy style SMC call, r12 will
> >> not be set to 0x200, and OP-TEE will emulate the functionality that
> >> the call is requesting.
> > 
> > AFAICT this means that OP-TEE completely replaces your existing monitor.
> 
> Correct.
> 
> > Please fix this properly. Implement SMCCC-compliant versions of those
> > calls you wish to retain, and come up with a new binding for those.
> > Describe that in the DT for systems which have OP-TEE providing these
> > services.
> 
> This might not be possible, these old SMCs are made very early and are
> used all over our existing platform code:
> 
> git grep "smc" -- arch/arm/mach-omap2

Given that one can build a multi-platform kernel, parts of the DT have
already been parsed (in order to determine that the machine is an OMAP2)
before any of this is executed.

These are not too early to preclude runtime switching based on the DT.

I can appreciate that it's a pain to have to alter these, but it's
certainly possible, and it would eliminate the problem rather than
spreading it further.

> > Systems with the old monitor should have that descrbied in their DT, and
> > not OP-TEE.
> 
> All systems have the old monitor by default, OP-TEE is the new and
> optional case, I think its node should be the entity modified when
> adding something OP-TEE specific like this.

... and so the DT can also be altered to describe a new SMCCC-compliant
set of OMAP2 SMCs in this case.

Thanks,
Mark.

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[Rob Herring]

On Mon, Sep 18, 2017 at 3:50 PM, Andrew F. Davis <afd@ti.com> wrote:
> On TI platforms OP-TEE must be called using a modified SMC call,
> allow the selection of this though DT.
>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt | 2 ++
>  drivers/tee/optee/core.c                                           | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> index d38834c67dff..a3275ecdf186 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> @@ -20,6 +20,8 @@ the reference implementation maintained by Linaro.
>                     "hvc" : HVC #0, with the register assignments specified
>                            in drivers/tee/optee/optee_smc.h
>
> +                   "ti-smc" : Similar to "smc" with TI specific register
> +                             adjustments

Sigh, really? IMO, this should be determined from the compatible
string. Then the next TI (or any vendor) specific thing can be handled
without a DT change.

Maybe some day we'll figure out that not just h/w needs to be
probe-able, but all the firmware pieces do too.

Rob

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[Andrew F. Davis]

On 09/19/2017 08:36 AM, Rob Herring wrote:
> On Mon, Sep 18, 2017 at 3:50 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On TI platforms OP-TEE must be called using a modified SMC call,
>> allow the selection of this though DT.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>  Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt | 2 ++
>>  drivers/tee/optee/core.c                                           | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>> index d38834c67dff..a3275ecdf186 100644
>> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>> @@ -20,6 +20,8 @@ the reference implementation maintained by Linaro.
>>                     "hvc" : HVC #0, with the register assignments specified
>>                            in drivers/tee/optee/optee_smc.h
>>
>> +                   "ti-smc" : Similar to "smc" with TI specific register
>> +                             adjustments
> 
> Sigh, really? IMO, this should be determined from the compatible
> string. Then the next TI (or any vendor) specific thing can be handled
> without a DT change.
> 

Which compatible string, do you mean the OP-TEE driver check the top
level platform compatible string?

> Maybe some day we'll figure out that not just h/w needs to be
> probe-able, but all the firmware pieces do too.
> 
> Rob
> 

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[Rob Herring]

On Tue, Sep 19, 2017 at 10:54 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 09/19/2017 08:36 AM, Rob Herring wrote:
>> On Mon, Sep 18, 2017 at 3:50 PM, Andrew F. Davis <afd@ti.com> wrote:
>>> On TI platforms OP-TEE must be called using a modified SMC call,
>>> allow the selection of this though DT.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt | 2 ++
>>>  drivers/tee/optee/core.c                                           | 2 ++
>>>  2 files changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>>> index d38834c67dff..a3275ecdf186 100644
>>> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>>> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
>>> @@ -20,6 +20,8 @@ the reference implementation maintained by Linaro.
>>>                     "hvc" : HVC #0, with the register assignments specified
>>>                            in drivers/tee/optee/optee_smc.h
>>>
>>> +                   "ti-smc" : Similar to "smc" with TI specific register
>>> +                             adjustments
>>
>> Sigh, really? IMO, this should be determined from the compatible
>> string. Then the next TI (or any vendor) specific thing can be handled
>> without a DT change.
>>
>
> Which compatible string, do you mean the OP-TEE driver check the top
> level platform compatible string?

No, you need to have something like "ti,optee-tz" for the driver to
match on because your implementation is different.

Rob

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[Mark Rutland]

On Mon, Sep 18, 2017 at 03:50:05PM -0500, Andrew F. Davis wrote:
> On TI platforms OP-TEE must be called using a modified SMC call,
> allow the selection of this though DT.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt | 2 ++
>  drivers/tee/optee/core.c                                           | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> index d38834c67dff..a3275ecdf186 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.txt
> @@ -20,6 +20,8 @@ the reference implementation maintained by Linaro.
>                     "hvc" : HVC #0, with the register assignments specified
>  		           in drivers/tee/optee/optee_smc.h
>  
> +                   "ti-smc" : Similar to "smc" with TI specific register
> +		              adjustments

... which are what, exactly? Specified where?

As Rob said, this should really have a new compat string.

Thanks,
Mark.

>  
>  
>  Example:
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 7952357df9c8..dfa9de590d98 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -441,6 +441,8 @@ static optee_invoke_fn *get_invoke_func(struct device_node *np)
>  		return optee_smccc_hvc;
>  	else if (!strcmp("smc", method))
>  		return optee_smccc_smc;
> +	else if (!strcmp("ti-smc", method))
> +		return arm_ti_smccc_smc;
>  
>  	pr_warn("invalid \"method\" property: %s\n", method);
>  	return ERR_PTR(-EINVAL);
> -- 
> 2.14.1
> 

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[kbuild test robot]

Hi Andrew,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc1 next-20170919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrew-F-Davis/ARM-smccc-call-Use-r12-to-route-secure-monitor-calls-on-TI-platforms/20170920-052543
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> ERROR: "arm_ti_smccc_smc" [drivers/tee/optee/optee.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 57549 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170920/bc8005a8/attachment-0001.gz>

[PATCH 2/2] tee: optee: allow selection of ti-smc as a calling method

[kbuild test robot]

Hi Andrew,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc1 next-20170919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrew-F-Davis/ARM-smccc-call-Use-r12-to-route-secure-monitor-calls-on-TI-platforms/20170920-052543
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/tee/optee/core.o: In function `get_invoke_func':
>> drivers/tee/optee/core.c:445: undefined reference to `arm_ti_smccc_smc'
   drivers/tee/optee/core.c:445:(.init.text+0xc0): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `arm_ti_smccc_smc'
>> drivers/tee/optee/core.c:445: undefined reference to `arm_ti_smccc_smc'

vim +445 drivers/tee/optee/core.c

   428	
   429	static optee_invoke_fn *get_invoke_func(struct device_node *np)
   430	{
   431		const char *method;
   432	
   433		pr_info("probing for conduit method from DT.\n");
   434	
   435		if (of_property_read_string(np, "method", &method)) {
   436			pr_warn("missing \"method\" property\n");
   437			return ERR_PTR(-ENXIO);
   438		}
   439	
   440		if (!strcmp("hvc", method))
   441			return optee_smccc_hvc;
   442		else if (!strcmp("smc", method))
   443			return optee_smccc_smc;
   444		else if (!strcmp("ti-smc", method))
 > 445			return arm_ti_smccc_smc;
   446	
   447		pr_warn("invalid \"method\" property: %s\n", method);
   448		return ERR_PTR(-EINVAL);
   449	}
   450	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 36756 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170920/95051a8b/attachment-0001.gz>