diff mbox series

ARM: OMAP: Use ARM SMC Calling Convention when OP-TEE is available

Message ID 20191118165236.22136-1-afd@ti.com (mailing list archive)
State New, archived
Headers show
Series ARM: OMAP: Use ARM SMC Calling Convention when OP-TEE is available | expand

Commit Message

Andrew Davis Nov. 18, 2019, 4:52 p.m. UTC
On High-Security(HS) OMAP2+ class devices a couple actions must be performed
from the ARM TrustZone during boot. These traditionally can be performed by
calling into the secure ROM code resident in this secure world using legacy
SMC calls. Optionally OP-TEE can replace this secure world functionality by
replacing the ROM after boot. ARM recommends a standard calling convention
is used for this interaction (SMC Calling Convention). We check DT for the
presence of OP-TEE and use this type of call to perform the needed actions,
falling back to the legacy OMAP ROM call if OP-TEE is not available.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 arch/arm/mach-omap2/common.h      |  2 +-
 arch/arm/mach-omap2/omap-secure.c | 27 ++++++++++++++++++++++++++-
 arch/arm/mach-omap2/omap-secure.h |  1 +
 arch/arm/mach-omap2/omap-smc.S    |  6 +++---
 4 files changed, 31 insertions(+), 5 deletions(-)

Comments

Tony Lindgren Nov. 18, 2019, 9:57 p.m. UTC | #1
Hi,

* Andrew F. Davis <afd@ti.com> [191118 08:53]:
> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
> +	ARM_SMCCC_OWNER_SIP, (func_num))
> +
> +void omap_smc1(u32 fn, u32 arg)
> +{
> +	struct device_node *optee;
> +	struct arm_smccc_res res;
> +
> +	/*
> +	 * If this platform has OP-TEE installed we use ARM SMC calls
> +	 * otherwise fall back to the OMAP ROM style calls.
> +	 */
> +	optee = of_find_node_by_path("/firmware/optee");
> +	if (optee) {
> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
> +			      0, 0, 0, 0, 0, 0, &res);
> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
> +	} else {
> +		_omap_smc1(fn, arg);
> +	}
> +}

I think we're better off just making arm_smccc_smc() work properly.
See cat arch/arm*/kernel/smccc-call.S.

If quirk handling is needed, looks like ARM_SMCCC_QUIRK_STATE_OFFS
can be used.

AFAIK this should work both for optee and the current use cases.

Regards,

Tony
Andrew Davis Nov. 18, 2019, 10:13 p.m. UTC | #2
On 11/18/19 4:57 PM, Tony Lindgren wrote:
> Hi,
> 
> * Andrew F. Davis <afd@ti.com> [191118 08:53]:
>> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
>> +	ARM_SMCCC_OWNER_SIP, (func_num))
>> +
>> +void omap_smc1(u32 fn, u32 arg)
>> +{
>> +	struct device_node *optee;
>> +	struct arm_smccc_res res;
>> +
>> +	/*
>> +	 * If this platform has OP-TEE installed we use ARM SMC calls
>> +	 * otherwise fall back to the OMAP ROM style calls.
>> +	 */
>> +	optee = of_find_node_by_path("/firmware/optee");
>> +	if (optee) {
>> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
>> +			      0, 0, 0, 0, 0, 0, &res);
>> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
>> +	} else {
>> +		_omap_smc1(fn, arg);
>> +	}
>> +}
> 
> I think we're better off just making arm_smccc_smc() work properly.
> See cat arch/arm*/kernel/smccc-call.S.
> 


arm_smccc_smc() does work properly already, I'm using it here.


> If quirk handling is needed, looks like ARM_SMCCC_QUIRK_STATE_OFFS
> can be used.
> 


Tried that [0], was NAKd. Making quirk-free SMCCC calls if OP-TEE is
detected seems to be the suggested path forward, QCOM got a pass,
doesn't look like we will get the same.

+Mark, in case you want to comment if this patch matches what you had in
mind.

[0] https://www.spinics.net/lists/arm-kernel/msg607263.html

Andrew


> AFAIK this should work both for optee and the current use cases.
> 
> Regards,
> 
> Tony
>
Tony Lindgren Nov. 18, 2019, 10:31 p.m. UTC | #3
* Andrew F. Davis <afd@ti.com> [191118 22:14]:
> On 11/18/19 4:57 PM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Andrew F. Davis <afd@ti.com> [191118 08:53]:
> >> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
> >> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
> >> +	ARM_SMCCC_OWNER_SIP, (func_num))
> >> +
> >> +void omap_smc1(u32 fn, u32 arg)
> >> +{
> >> +	struct device_node *optee;
> >> +	struct arm_smccc_res res;
> >> +
> >> +	/*
> >> +	 * If this platform has OP-TEE installed we use ARM SMC calls
> >> +	 * otherwise fall back to the OMAP ROM style calls.
> >> +	 */
> >> +	optee = of_find_node_by_path("/firmware/optee");
> >> +	if (optee) {
> >> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
> >> +			      0, 0, 0, 0, 0, 0, &res);
> >> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
> >> +	} else {
> >> +		_omap_smc1(fn, arg);
> >> +	}
> >> +}
> > 
> > I think we're better off just making arm_smccc_smc() work properly.
> > See cat arch/arm*/kernel/smccc-call.S.
> > 
> 
> 
> arm_smccc_smc() does work properly already, I'm using it here.

OK. I guess I don't follow then why we can't use arm_smccc_smc()
for old code.

> > If quirk handling is needed, looks like ARM_SMCCC_QUIRK_STATE_OFFS
> > can be used.
> > 
> 
> 
> Tried that [0], was NAKd. Making quirk-free SMCCC calls if OP-TEE is
> detected seems to be the suggested path forward, QCOM got a pass,
> doesn't look like we will get the same.
> 
> +Mark, in case you want to comment if this patch matches what you had in
> mind.
> 
> [0] https://www.spinics.net/lists/arm-kernel/msg607263.html

Yeah I might be missing some parts here..

> > AFAIK this should work both for optee and the current use cases.

.. as I'd like to have a solution that works for both cases using
arm_smccc_smc().

If r12 is the only issue, souds like we can just use a wrapper
for the legacy calls to call arm_smccc_smc()?

Regards,

Tony
Andrew Davis Nov. 19, 2019, 1:13 a.m. UTC | #4
On 11/18/19 5:31 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191118 22:14]:
>> On 11/18/19 4:57 PM, Tony Lindgren wrote:
>>> Hi,
>>>
>>> * Andrew F. Davis <afd@ti.com> [191118 08:53]:
>>>> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
>>>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
>>>> +	ARM_SMCCC_OWNER_SIP, (func_num))
>>>> +
>>>> +void omap_smc1(u32 fn, u32 arg)
>>>> +{
>>>> +	struct device_node *optee;
>>>> +	struct arm_smccc_res res;
>>>> +
>>>> +	/*
>>>> +	 * If this platform has OP-TEE installed we use ARM SMC calls
>>>> +	 * otherwise fall back to the OMAP ROM style calls.
>>>> +	 */
>>>> +	optee = of_find_node_by_path("/firmware/optee");
>>>> +	if (optee) {
>>>> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
>>>> +			      0, 0, 0, 0, 0, 0, &res);
>>>> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
>>>> +	} else {
>>>> +		_omap_smc1(fn, arg);
>>>> +	}
>>>> +}
>>>
>>> I think we're better off just making arm_smccc_smc() work properly.
>>> See cat arch/arm*/kernel/smccc-call.S.
>>>
>>
>>
>> arm_smccc_smc() does work properly already, I'm using it here.
> 
> OK. I guess I don't follow then why we can't use arm_smccc_smc()
> for old code.
> 


Our ROM code needs r12 to have the function code in it, where as the ARM
SMC calling convention standard requires that (plus some other
information) stored in r0. Our ROM doesn't know anything about the that
standard that came out years after we shipped these devices. And as such
is not complaint.

A generic smc() call would be nice, but arm_smccc_smc() is specifically
for SMCCC.


>>> If quirk handling is needed, looks like ARM_SMCCC_QUIRK_STATE_OFFS
>>> can be used.
>>>
>>
>>
>> Tried that [0], was NAKd. Making quirk-free SMCCC calls if OP-TEE is
>> detected seems to be the suggested path forward, QCOM got a pass,
>> doesn't look like we will get the same.
>>
>> +Mark, in case you want to comment if this patch matches what you had in
>> mind.
>>
>> [0] https://www.spinics.net/lists/arm-kernel/msg607263.html
> 
> Yeah I might be missing some parts here..
> 
>>> AFAIK this should work both for optee and the current use cases.
> 
> .. as I'd like to have a solution that works for both cases using
> arm_smccc_smc().
> 
> If r12 is the only issue, souds like we can just use a wrapper
> for the legacy calls to call arm_smccc_smc()?
> 


The standard does not define r12, to be compliant to the standard r12
must not be used. arm_smccc_smc() doesn't allow us to populate it even
if we wanted, only r0-r7.

In the above linked patch I made a quirk version that allowed for r12
use but got NAKd, I'm not sure of any other way outside of something
like this patch.

Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 4:21 p.m. UTC | #5
* Andrew F. Davis <afd@ti.com> [191119 01:14]:
> On 11/18/19 5:31 PM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [191118 22:14]:
> >> On 11/18/19 4:57 PM, Tony Lindgren wrote:
> >>> Hi,
> >>>
> >>> * Andrew F. Davis <afd@ti.com> [191118 08:53]:
> >>>> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
> >>>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
> >>>> +	ARM_SMCCC_OWNER_SIP, (func_num))
> >>>> +
> >>>> +void omap_smc1(u32 fn, u32 arg)
> >>>> +{
> >>>> +	struct device_node *optee;
> >>>> +	struct arm_smccc_res res;
> >>>> +
> >>>> +	/*
> >>>> +	 * If this platform has OP-TEE installed we use ARM SMC calls
> >>>> +	 * otherwise fall back to the OMAP ROM style calls.
> >>>> +	 */
> >>>> +	optee = of_find_node_by_path("/firmware/optee");
> >>>> +	if (optee) {
> >>>> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
> >>>> +			      0, 0, 0, 0, 0, 0, &res);
> >>>> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
> >>>> +	} else {
> >>>> +		_omap_smc1(fn, arg);
> >>>> +	}
> >>>> +}
> >>>
> >>> I think we're better off just making arm_smccc_smc() work properly.
> >>> See cat arch/arm*/kernel/smccc-call.S.
> >>>
> >>
> >>
> >> arm_smccc_smc() does work properly already, I'm using it here.
> > 
> > OK. I guess I don't follow then why we can't use arm_smccc_smc()
> > for old code.
> > 
> 
> 
> Our ROM code needs r12 to have the function code in it, where as the ARM
> SMC calling convention standard requires that (plus some other
> information) stored in r0. Our ROM doesn't know anything about the that
> standard that came out years after we shipped these devices. And as such
> is not complaint.

Right.

> A generic smc() call would be nice, but arm_smccc_smc() is specifically
> for SMCCC.

To me it seeems that HAVE_ARM_SMCCC is a generic feature though.
It's not limited to OPTEE. We have select HAVE_ARM_SMCCC if CPU_V7
in arch/arm/Kconfig, and OPTEE depends on HAVE_ARM_SMCCC.

From that point of view it seems that we could have HAVE_ARM_SMCCC
enabled also for v6 and use it for all mach-omap2 with a wrapper.

So I'd like to have our smc callers eventually just call generic
generic arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn)...) rather
than the custom calls. And we want to update to using the generic
functions one case at a time as the features get tested :)

In any case, you should do the necessary checks for HAVE_ARM_SMCCC
only once during init. I'm not sure how much checking for
"/firmware/optee" helps here, sounds like we have a broken system
if the firmware is not there while the arm_smccc_smc() should
still work just fine :)

Regards,

Tony
Tony Lindgren Nov. 19, 2019, 4:30 p.m. UTC | #6
* Tony Lindgren <tony@atomide.com> [191119 16:22]:
> * Andrew F. Davis <afd@ti.com> [191119 01:14]:
> > A generic smc() call would be nice, but arm_smccc_smc() is specifically
> > for SMCCC.
> 
> To me it seeems that HAVE_ARM_SMCCC is a generic feature though.
> It's not limited to OPTEE. We have select HAVE_ARM_SMCCC if CPU_V7
> in arch/arm/Kconfig, and OPTEE depends on HAVE_ARM_SMCCC.
> 
> From that point of view it seems that we could have HAVE_ARM_SMCCC
> enabled also for v6 and use it for all mach-omap2 with a wrapper.

In the omap_smc1 case it seems that we can just unconditionally
change the callers to use arm_smccc_smc() instead of omap_smc1
and get rid of it. It's only used by v7 SMP related stuff based
on grepping for it.

> So I'd like to have our smc callers eventually just call generic
> generic arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn)...) rather
> than the custom calls. And we want to update to using the generic
> functions one case at a time as the features get tested :)

Sounds like the others can be then done one at a time as
needed :)

Regards,

Tony
Andrew Davis Nov. 19, 2019, 4:30 p.m. UTC | #7
On 11/19/19 11:21 AM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191119 01:14]:
>> On 11/18/19 5:31 PM, Tony Lindgren wrote:
>>> * Andrew F. Davis <afd@ti.com> [191118 22:14]:
>>>> On 11/18/19 4:57 PM, Tony Lindgren wrote:
>>>>> Hi,
>>>>>
>>>>> * Andrew F. Davis <afd@ti.com> [191118 08:53]:
>>>>>> +#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
>>>>>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
>>>>>> +	ARM_SMCCC_OWNER_SIP, (func_num))
>>>>>> +
>>>>>> +void omap_smc1(u32 fn, u32 arg)
>>>>>> +{
>>>>>> +	struct device_node *optee;
>>>>>> +	struct arm_smccc_res res;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If this platform has OP-TEE installed we use ARM SMC calls
>>>>>> +	 * otherwise fall back to the OMAP ROM style calls.
>>>>>> +	 */
>>>>>> +	optee = of_find_node_by_path("/firmware/optee");
>>>>>> +	if (optee) {
>>>>>> +		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
>>>>>> +			      0, 0, 0, 0, 0, 0, &res);
>>>>>> +		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
>>>>>> +	} else {
>>>>>> +		_omap_smc1(fn, arg);
>>>>>> +	}
>>>>>> +}
>>>>>
>>>>> I think we're better off just making arm_smccc_smc() work properly.
>>>>> See cat arch/arm*/kernel/smccc-call.S.
>>>>>
>>>>
>>>>
>>>> arm_smccc_smc() does work properly already, I'm using it here.
>>>
>>> OK. I guess I don't follow then why we can't use arm_smccc_smc()
>>> for old code.
>>>
>>
>>
>> Our ROM code needs r12 to have the function code in it, where as the ARM
>> SMC calling convention standard requires that (plus some other
>> information) stored in r0. Our ROM doesn't know anything about the that
>> standard that came out years after we shipped these devices. And as such
>> is not complaint.
> 
> Right.
> 
>> A generic smc() call would be nice, but arm_smccc_smc() is specifically
>> for SMCCC.
> 
> To me it seeems that HAVE_ARM_SMCCC is a generic feature though.
> It's not limited to OPTEE. We have select HAVE_ARM_SMCCC if CPU_V7
> in arch/arm/Kconfig, and OPTEE depends on HAVE_ARM_SMCCC.
> 


It is not just for OP-TEE but for communicating with any compliant
system monitor/trustzone. Our ROM monitor is not compliant.


> From that point of view it seems that we could have HAVE_ARM_SMCCC
> enabled also for v6 and use it for all mach-omap2 with a wrapper.
> 
> So I'd like to have our smc callers eventually just call generic
> generic arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn)...) rather
> than the custom calls. And we want to update to using the generic
> functions one case at a time as the features get tested :)
> 


It wont work, as said above, arm_smccc_smc() is not compatible with our
ROM, no wrapper around that function will make it work with our ROM, it
is not able to load our function number into r12 where the ROM expects
it. The function number for arm_smccc_smc() goes into r0, r12 is not
even exposed through arm_smccc_smc().


> In any case, you should do the necessary checks for HAVE_ARM_SMCCC
> only once during init. I'm not sure how much checking for
> "/firmware/optee" helps here, sounds like we have a broken system
> if the firmware is not there while the arm_smccc_smc() should
> still work just fine :)


arm_smccc_smc() only works on mach-omap2 platforms when OP-TEE is
available. On older system or systems where OP-TEE has not been
installed we need to fall back to our custom smc() calls.

Andrew


> 
> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 4:42 p.m. UTC | #8
* Andrew F. Davis <afd@ti.com> [191119 16:31]:
> arm_smccc_smc() only works on mach-omap2 platforms when OP-TEE is
> available. On older system or systems where OP-TEE has not been
> installed we need to fall back to our custom smc() calls.

Hmm OK so optee probably just adds support for new functions
when loaded.

Do the old bootrom functions stop working if optee is loaded?

If arm_smccc_smc() working depends on optee firmware being
loaded and the old omap_smc1 won't work, then you should
configure the function on init or dynamically modify the
code like we do in smp_on_up case for example.

What I'd like to have though is to make arm_smccc_smc()
work for optee and non-optee case for mach-omap2 as it
already has the features necessary to do the runtime
patching of the code for the quirks.

Mark do you have any comments on this?

Regards,

Tony
Tony Lindgren Nov. 19, 2019, 6:05 p.m. UTC | #9
* Tony Lindgren <tony@atomide.com> [191119 16:43]:
> What I'd like to have though is to make arm_smccc_smc()
> work for optee and non-optee case for mach-omap2 as it
> already has the features necessary to do the runtime
> patching of the code for the quirks.

In any case sounds like we only need the r12 quirk when
optee is _not_ enabled.

So a modified version of your earlier smccc-call.S patch
modified to only enable the r12 quirk when no optee is
loaded just might be all we need :)

Regards,

Tony
Andrew Davis Nov. 19, 2019, 6:20 p.m. UTC | #10
On 11/19/19 1:05 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [191119 16:43]:
>> What I'd like to have though is to make arm_smccc_smc()
>> work for optee and non-optee case for mach-omap2 as it
>> already has the features necessary to do the runtime
>> patching of the code for the quirks.
> 
> In any case sounds like we only need the r12 quirk when
> optee is _not_ enabled.
> 
> So a modified version of your earlier smccc-call.S patch
> modified to only enable the r12 quirk when no optee is
> loaded just might be all we need :)
> 


Doesn't change the reason the earlier patch was NAKd, we would still be
modifying the core SMCCC call to be non-compliant.

And doing it only when OP-TEE is not installed doesn't gain us anything,
we already have our own SMC calls for when OP-TEE is not available, this
patch is specifically so the OMAP2+ boot still works even when OP-TEE is
installed.

If you can get Mark to take my old patch then we can think about moving
more legacy SMC callers to the SMCCC, otherwise this patch is what we
need to get OP-TEE enabled OMAP2+ platforms to boot and we will just
stick to the custom SMC functions we already have for everything else.

Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 6:32 p.m. UTC | #11
* Andrew F. Davis <afd@ti.com> [191119 18:21]:
> On 11/19/19 1:05 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [191119 16:43]:
> >> What I'd like to have though is to make arm_smccc_smc()
> >> work for optee and non-optee case for mach-omap2 as it
> >> already has the features necessary to do the runtime
> >> patching of the code for the quirks.
> > 
> > In any case sounds like we only need the r12 quirk when
> > optee is _not_ enabled.
> > 
> > So a modified version of your earlier smccc-call.S patch
> > modified to only enable the r12 quirk when no optee is
> > loaded just might be all we need :)
> 
> Doesn't change the reason the earlier patch was NAKd, we would still be
> modifying the core SMCCC call to be non-compliant.

Well let's see what Mark says about r12 quirk version
that is only needed when optee is not active.

> And doing it only when OP-TEE is not installed doesn't gain us anything,
> we already have our own SMC calls for when OP-TEE is not available, this
> patch is specifically so the OMAP2+ boot still works even when OP-TEE is
> installed.

It would allow us to completely change over to using
arm_smccc_smc() and forget the custom calls.

> If you can get Mark to take my old patch then we can think about moving
> more legacy SMC callers to the SMCCC, otherwise this patch is what we
> need to get OP-TEE enabled OMAP2+ platforms to boot and we will just
> stick to the custom SMC functions we already have for everything else.

To me it sounds like your old patch won't work as is though,
we just want the code modified dynamically if optee is not
present to enable the r12 quirk.

Of course if both the optee version without the r12 quirk,
and a non-optee version with the r12 of the arm_smccc_smc()
are needed the same time on a booted system, then they should
be kept separate.

Regards,

Tony
Andrew Davis Nov. 19, 2019, 6:50 p.m. UTC | #12
On 11/19/19 1:32 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191119 18:21]:
>> On 11/19/19 1:05 PM, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [191119 16:43]:
>>>> What I'd like to have though is to make arm_smccc_smc()
>>>> work for optee and non-optee case for mach-omap2 as it
>>>> already has the features necessary to do the runtime
>>>> patching of the code for the quirks.
>>>
>>> In any case sounds like we only need the r12 quirk when
>>> optee is _not_ enabled.
>>>
>>> So a modified version of your earlier smccc-call.S patch
>>> modified to only enable the r12 quirk when no optee is
>>> loaded just might be all we need :)
>>
>> Doesn't change the reason the earlier patch was NAKd, we would still be
>> modifying the core SMCCC call to be non-compliant.
> 
> Well let's see what Mark says about r12 quirk version
> that is only needed when optee is not active.
> 
>> And doing it only when OP-TEE is not installed doesn't gain us anything,
>> we already have our own SMC calls for when OP-TEE is not available, this
>> patch is specifically so the OMAP2+ boot still works even when OP-TEE is
>> installed.
> 
> It would allow us to completely change over to using
> arm_smccc_smc() and forget the custom calls.
> 


We would need more than just the r12 quirk to replace all our custom SMC
handlers, we would need quirks for omap_smc2 which puts process ID in r1
and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
SMC calls also trash r4-r11, that is very non SMCCC complaint as only
r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
legacy ROM no matter how much we hack at it :(


>> If you can get Mark to take my old patch then we can think about moving
>> more legacy SMC callers to the SMCCC, otherwise this patch is what we
>> need to get OP-TEE enabled OMAP2+ platforms to boot and we will just
>> stick to the custom SMC functions we already have for everything else.
> 
> To me it sounds like your old patch won't work as is though,
> we just want the code modified dynamically if optee is not
> present to enable the r12 quirk.
> 


I can make OP-TEE also compatible with the r12 quirk, which is what I
used to do. That way we didn't need to do any detection. The issue was
that non-standard SMC calls should not go through the common SMCCC
handler (unless you are QCOM for some reason..).

Andrew


> Of course if both the optee version without the r12 quirk,
> and a non-optee version with the r12 of the arm_smccc_smc()
> are needed the same time on a booted system, then they should
> be kept separate.
> 
> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 7:07 p.m. UTC | #13
* Andrew F. Davis <afd@ti.com> [191119 18:51]:
> On 11/19/19 1:32 PM, Tony Lindgren wrote:
> > It would allow us to completely change over to using
> > arm_smccc_smc() and forget the custom calls.
> 
> We would need more than just the r12 quirk to replace all our custom SMC
> handlers, we would need quirks for omap_smc2 which puts process ID in r1
> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
> legacy ROM no matter how much we hack at it :(

We would just have omap_smc2() call arm_smccc_smc() and in that
case. And omap_smc2() would still deal with saving and restoring
the registers.

Certainly the wrapper functions calling arm_smccc_smc() can deal
with r12 too if the r12-quirk version and the plain version are
never needed the same time on a booted SoC.

Are they ever needed the same time on a booted SoC or not?

> I can make OP-TEE also compatible with the r12 quirk, which is what I
> used to do. That way we didn't need to do any detection. The issue was
> that non-standard SMC calls should not go through the common SMCCC
> handler (unless you are QCOM for some reason..).

Sounds like for optee nothing must be done for r12 :)

Regards,

Tony
Andrew Davis Nov. 19, 2019, 7:12 p.m. UTC | #14
On 11/19/19 2:07 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191119 18:51]:
>> On 11/19/19 1:32 PM, Tony Lindgren wrote:
>>> It would allow us to completely change over to using
>>> arm_smccc_smc() and forget the custom calls.
>>
>> We would need more than just the r12 quirk to replace all our custom SMC
>> handlers, we would need quirks for omap_smc2 which puts process ID in r1
>> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
>> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
>> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
>> legacy ROM no matter how much we hack at it :(
> 
> We would just have omap_smc2() call arm_smccc_smc() and in that
> case. And omap_smc2() would still deal with saving and restoring
> the registers.
> 


Then why call arm_smccc_smc()? omap_smc2() is already an assembly
function, all it needs to do after loading the registers and saving the
right ones is issue an "smc #0" instruction, why would we want to
instead call into some other function to re-save registers and issue the
exact same instruction?


> Certainly the wrapper functions calling arm_smccc_smc() can deal
> with r12 too if the r12-quirk version and the plain version are
> never needed the same time on a booted SoC.
> 
> Are they ever needed the same time on a booted SoC or not?
> 
>> I can make OP-TEE also compatible with the r12 quirk, which is what I
>> used to do. That way we didn't need to do any detection. The issue was
>> that non-standard SMC calls should not go through the common SMCCC
>> handler (unless you are QCOM for some reason..).
> 
> Sounds like for optee nothing must be done for r12 :)
> 


Unless all our calls use the r12 hack, then we would need to fixup
OP-TEE to accept that also.

Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 7:20 p.m. UTC | #15
* Andrew F. Davis <afd@ti.com> [191119 19:13]:
> On 11/19/19 2:07 PM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [191119 18:51]:
> >> On 11/19/19 1:32 PM, Tony Lindgren wrote:
> >>> It would allow us to completely change over to using
> >>> arm_smccc_smc() and forget the custom calls.
> >>
> >> We would need more than just the r12 quirk to replace all our custom SMC
> >> handlers, we would need quirks for omap_smc2 which puts process ID in r1
> >> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
> >> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
> >> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
> >> legacy ROM no matter how much we hack at it :(
> > 
> > We would just have omap_smc2() call arm_smccc_smc() and in that
> > case. And omap_smc2() would still deal with saving and restoring
> > the registers.
> 
> Then why call arm_smccc_smc()? omap_smc2() is already an assembly
> function, all it needs to do after loading the registers and saving the
> right ones is issue an "smc #0" instruction, why would we want to
> instead call into some other function to re-save registers and issue the
> exact same instruction?

To use Linux generic API for smc calls where possible.

> > Certainly the wrapper functions calling arm_smccc_smc() can deal
> > with r12 too if the r12-quirk version and the plain version are
> > never needed the same time on a booted SoC.
> > 
> > Are they ever needed the same time on a booted SoC or not?

   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Sorry but maybe check the font size on your screen. I'm trying to
get your attention again for the second time above to answer a
question I asked.

> >> I can make OP-TEE also compatible with the r12 quirk, which is what I
> >> used to do. That way we didn't need to do any detection. The issue was
> >> that non-standard SMC calls should not go through the common SMCCC
> >> handler (unless you are QCOM for some reason..).
> > 
> > Sounds like for optee nothing must be done for r12 :)

> Unless all our calls use the r12 hack, then we would need to fixup
> OP-TEE to accept that also.

No idea about that that part, but sounds like r12 use is up to
the caller in the optee case.

Regards,

Tony
Andrew Davis Nov. 19, 2019, 7:35 p.m. UTC | #16
On 11/19/19 2:20 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191119 19:13]:
>> On 11/19/19 2:07 PM, Tony Lindgren wrote:
>>> * Andrew F. Davis <afd@ti.com> [191119 18:51]:
>>>> On 11/19/19 1:32 PM, Tony Lindgren wrote:
>>>>> It would allow us to completely change over to using
>>>>> arm_smccc_smc() and forget the custom calls.
>>>>
>>>> We would need more than just the r12 quirk to replace all our custom SMC
>>>> handlers, we would need quirks for omap_smc2 which puts process ID in r1
>>>> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
>>>> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
>>>> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
>>>> legacy ROM no matter how much we hack at it :(
>>>
>>> We would just have omap_smc2() call arm_smccc_smc() and in that
>>> case. And omap_smc2() would still deal with saving and restoring
>>> the registers.
>>
>> Then why call arm_smccc_smc()? omap_smc2() is already an assembly
>> function, all it needs to do after loading the registers and saving the
>> right ones is issue an "smc #0" instruction, why would we want to
>> instead call into some other function to re-save registers and issue the
>> exact same instruction?
> 
> To use Linux generic API for smc calls where possible.
> 


But we are not using generic API calls, we are using omap_smcx() which
cannot call into arm_smccc_smc(). For all the above reasons plus
arm_smccc_smc() uses r12 to save the stack pointer, our ROM expects r12
to store the function ID.


>>> Certainly the wrapper functions calling arm_smccc_smc() can deal
>>> with r12 too if the r12-quirk version and the plain version are
>>> never needed the same time on a booted SoC.
>>>
>>> Are they ever needed the same time on a booted SoC or not?
> 
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 


They should not be needed at the same time, either OP-TEE is on the
secure side or ROM is there.

Andrew


> Sorry but maybe check the font size on your screen. I'm trying to
> get your attention again for the second time above to answer a
> question I asked.
> 
>>>> I can make OP-TEE also compatible with the r12 quirk, which is what I
>>>> used to do. That way we didn't need to do any detection. The issue was
>>>> that non-standard SMC calls should not go through the common SMCCC
>>>> handler (unless you are QCOM for some reason..).
>>>
>>> Sounds like for optee nothing must be done for r12 :)
> 
>> Unless all our calls use the r12 hack, then we would need to fixup
>> OP-TEE to accept that also.
> 
> No idea about that that part, but sounds like r12 use is up to
> the caller in the optee case.
> 
> Regards,
> 
> Tony
>
Tony Lindgren Nov. 19, 2019, 7:44 p.m. UTC | #17
* Andrew F. Davis <afd@ti.com> [191119 19:36]:
> On 11/19/19 2:20 PM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [191119 19:13]:
> >> On 11/19/19 2:07 PM, Tony Lindgren wrote:
> >>> * Andrew F. Davis <afd@ti.com> [191119 18:51]:
> >>>> On 11/19/19 1:32 PM, Tony Lindgren wrote:
> >>>>> It would allow us to completely change over to using
> >>>>> arm_smccc_smc() and forget the custom calls.
> >>>>
> >>>> We would need more than just the r12 quirk to replace all our custom SMC
> >>>> handlers, we would need quirks for omap_smc2 which puts process ID in r1
> >>>> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
> >>>> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
> >>>> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
> >>>> legacy ROM no matter how much we hack at it :(
> >>>
> >>> We would just have omap_smc2() call arm_smccc_smc() and in that
> >>> case. And omap_smc2() would still deal with saving and restoring
> >>> the registers.
> >>
> >> Then why call arm_smccc_smc()? omap_smc2() is already an assembly
> >> function, all it needs to do after loading the registers and saving the
> >> right ones is issue an "smc #0" instruction, why would we want to
> >> instead call into some other function to re-save registers and issue the
> >> exact same instruction?
> > 
> > To use Linux generic API for smc calls where possible.
> 
> But we are not using generic API calls, we are using omap_smcx() which
> cannot call into arm_smccc_smc(). For all the above reasons plus
> arm_smccc_smc() uses r12 to save the stack pointer, our ROM expects r12
> to store the function ID.

Saving and restoring r12 could be handled by the arm_smccc_smc() quirk
for the non-optee case.

Then we could get rid of omap_smc1() and arm_smccc_smc() should work
for the optee case and non-optee case, right.

> >>> Certainly the wrapper functions calling arm_smccc_smc() can deal
> >>> with r12 too if the r12-quirk version and the plain version are
> >>> never needed the same time on a booted SoC.
> >>>
> >>> Are they ever needed the same time on a booted SoC or not?

> They should not be needed at the same time, either OP-TEE is on the
> secure side or ROM is there.

OK thanks. So we could just modify the code dynamically on boot
based on if optee is found or not. The quirk could be done along
the lines of the qcom quirk but only for the non-optee case:

$ git grep -C10 ARM_SMCCC_QUIRK_QCOM_A6

Regards,

Tony
Andrew Davis Nov. 19, 2019, 7:59 p.m. UTC | #18
On 11/19/19 2:44 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191119 19:36]:
>> On 11/19/19 2:20 PM, Tony Lindgren wrote:
>>> * Andrew F. Davis <afd@ti.com> [191119 19:13]:
>>>> On 11/19/19 2:07 PM, Tony Lindgren wrote:
>>>>> * Andrew F. Davis <afd@ti.com> [191119 18:51]:
>>>>>> On 11/19/19 1:32 PM, Tony Lindgren wrote:
>>>>>>> It would allow us to completely change over to using
>>>>>>> arm_smccc_smc() and forget the custom calls.
>>>>>>
>>>>>> We would need more than just the r12 quirk to replace all our custom SMC
>>>>>> handlers, we would need quirks for omap_smc2 which puts process ID in r1
>>>>>> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
>>>>>> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
>>>>>> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
>>>>>> legacy ROM no matter how much we hack at it :(
>>>>>
>>>>> We would just have omap_smc2() call arm_smccc_smc() and in that
>>>>> case. And omap_smc2() would still deal with saving and restoring
>>>>> the registers.
>>>>
>>>> Then why call arm_smccc_smc()? omap_smc2() is already an assembly
>>>> function, all it needs to do after loading the registers and saving the
>>>> right ones is issue an "smc #0" instruction, why would we want to
>>>> instead call into some other function to re-save registers and issue the
>>>> exact same instruction?
>>>
>>> To use Linux generic API for smc calls where possible.
>>
>> But we are not using generic API calls, we are using omap_smcx() which
>> cannot call into arm_smccc_smc(). For all the above reasons plus
>> arm_smccc_smc() uses r12 to save the stack pointer, our ROM expects r12
>> to store the function ID.
> 
> Saving and restoring r12 could be handled by the arm_smccc_smc() quirk
> for the non-optee case.
> 
> Then we could get rid of omap_smc1() and arm_smccc_smc() should work
> for the optee case and non-optee case, right.
> 


Yes, we could have both cases working if we could get the quirk in.


>>>>> Certainly the wrapper functions calling arm_smccc_smc() can deal
>>>>> with r12 too if the r12-quirk version and the plain version are
>>>>> never needed the same time on a booted SoC.
>>>>>
>>>>> Are they ever needed the same time on a booted SoC or not?
> 
>> They should not be needed at the same time, either OP-TEE is on the
>> secure side or ROM is there.
> 
> OK thanks. So we could just modify the code dynamically on boot
> based on if optee is found or not. The quirk could be done along
> the lines of the qcom quirk but only for the non-optee case:
> 


We wouldn't have to patch anything if we could get the quirk in. One has
to state they wish to use the quirk version in a structure passed into
arm_smccc_smc_quirk(), in which case for all legacy user we just fill
out this quirk struct. OP-TEE uses the same arm_smccc_smc() but without
the quirk struct and so it uses the compliant call.

The issue is still the same, I tried adding this, I got NAKd, if you
want to convince Mark to change his mind and allow us the quirk then we
can go down this path. Otherwise this will remain a dead end.

Andrew


> $ git grep -C10 ARM_SMCCC_QUIRK_QCOM_A6
> 
> Regards,
> 
> Tony
>
Andrew Davis Dec. 16, 2019, 8:56 p.m. UTC | #19
On 11/19/19 2:59 PM, Andrew F. Davis wrote:
> On 11/19/19 2:44 PM, Tony Lindgren wrote:
>> * Andrew F. Davis <afd@ti.com> [191119 19:36]:
>>> On 11/19/19 2:20 PM, Tony Lindgren wrote:
>>>> * Andrew F. Davis <afd@ti.com> [191119 19:13]:
>>>>> On 11/19/19 2:07 PM, Tony Lindgren wrote:
>>>>>> * Andrew F. Davis <afd@ti.com> [191119 18:51]:
>>>>>>> On 11/19/19 1:32 PM, Tony Lindgren wrote:
>>>>>>>> It would allow us to completely change over to using
>>>>>>>> arm_smccc_smc() and forget the custom calls.
>>>>>>>
>>>>>>> We would need more than just the r12 quirk to replace all our custom SMC
>>>>>>> handlers, we would need quirks for omap_smc2 which puts process ID in r1
>>>>>>> and puts #0xff in r6, and omap_smc3 that uses smc #1. All of our legacy
>>>>>>> SMC calls also trash r4-r11, that is very non SMCCC complaint as only
>>>>>>> r4-r7 need be caller saved. I don't see arm_smccc_smc() working with
>>>>>>> legacy ROM no matter how much we hack at it :(
>>>>>>
>>>>>> We would just have omap_smc2() call arm_smccc_smc() and in that
>>>>>> case. And omap_smc2() would still deal with saving and restoring
>>>>>> the registers.
>>>>>
>>>>> Then why call arm_smccc_smc()? omap_smc2() is already an assembly
>>>>> function, all it needs to do after loading the registers and saving the
>>>>> right ones is issue an "smc #0" instruction, why would we want to
>>>>> instead call into some other function to re-save registers and issue the
>>>>> exact same instruction?
>>>>
>>>> To use Linux generic API for smc calls where possible.
>>>
>>> But we are not using generic API calls, we are using omap_smcx() which
>>> cannot call into arm_smccc_smc(). For all the above reasons plus
>>> arm_smccc_smc() uses r12 to save the stack pointer, our ROM expects r12
>>> to store the function ID.
>>
>> Saving and restoring r12 could be handled by the arm_smccc_smc() quirk
>> for the non-optee case.
>>
>> Then we could get rid of omap_smc1() and arm_smccc_smc() should work
>> for the optee case and non-optee case, right.
>>
> 
> 
> Yes, we could have both cases working if we could get the quirk in.
> 
> 
>>>>>> Certainly the wrapper functions calling arm_smccc_smc() can deal
>>>>>> with r12 too if the r12-quirk version and the plain version are
>>>>>> never needed the same time on a booted SoC.
>>>>>>
>>>>>> Are they ever needed the same time on a booted SoC or not?
>>
>>> They should not be needed at the same time, either OP-TEE is on the
>>> secure side or ROM is there.
>>
>> OK thanks. So we could just modify the code dynamically on boot
>> based on if optee is found or not. The quirk could be done along
>> the lines of the qcom quirk but only for the non-optee case:
>>
> 
> 
> We wouldn't have to patch anything if we could get the quirk in. One has
> to state they wish to use the quirk version in a structure passed into
> arm_smccc_smc_quirk(), in which case for all legacy user we just fill
> out this quirk struct. OP-TEE uses the same arm_smccc_smc() but without
> the quirk struct and so it uses the compliant call.
> 
> The issue is still the same, I tried adding this, I got NAKd, if you
> want to convince Mark to change his mind and allow us the quirk then we
> can go down this path. Otherwise this will remain a dead end.
> 


Hi Tony,

Looks like the TI quirk idea is not moving forward, even the QCOM quirk
looks like it may get kicked out. arm_smccc_smc() will remain only for
SMCCC compliant calls, but it looks like a generic arm_smc() wouldn't be
too opposed upstream.

Either way this patch would still be valid as when OP-TEE is present
then arm_smccc_smc() will be the right call to make, how we handle the
legacy calls can be sorted out later if a generic SMC call is implemented.

Thanks,
Andrew


> Andrew
> 
> 
>> $ git grep -C10 ARM_SMCCC_QUIRK_QCOM_A6
>>
>> Regards,
>>
>> Tony
>>
Tony Lindgren Dec. 16, 2019, 9:04 p.m. UTC | #20
* Andrew F. Davis <afd@ti.com> [191216 20:57]:
> Looks like the TI quirk idea is not moving forward, even the QCOM quirk
> looks like it may get kicked out. arm_smccc_smc() will remain only for
> SMCCC compliant calls, but it looks like a generic arm_smc() wouldn't be
> too opposed upstream.

Yes so it seems.

> Either way this patch would still be valid as when OP-TEE is present
> then arm_smccc_smc() will be the right call to make, how we handle the
> legacy calls can be sorted out later if a generic SMC call is implemented.

Please see my comment regarding this patch earlier in this thread
pasted below for convenience:

* Tony Lindgren <tony@atomide.com> [191119 16:22]:
> In any case, you should do the necessary checks for HAVE_ARM_SMCCC
> only once during init. I'm not sure how much checking for
> "/firmware/optee" helps here, sounds like we have a broken system
> if the firmware is not there while the arm_smccc_smc() should
> still work just fine :)

So only check once during init. And during init, you should probably
also check that arm_smccc_smc() actually infd optee if
"/firmware/optee" is set, and only then set set the right function
pointer or some flag.

Regards,

Tony
Andrew Davis Dec. 16, 2019, 10:34 p.m. UTC | #21
On 12/16/19 4:04 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191216 20:57]:
>> Looks like the TI quirk idea is not moving forward, even the QCOM quirk
>> looks like it may get kicked out. arm_smccc_smc() will remain only for
>> SMCCC compliant calls, but it looks like a generic arm_smc() wouldn't be
>> too opposed upstream.
> 
> Yes so it seems.
> 
>> Either way this patch would still be valid as when OP-TEE is present
>> then arm_smccc_smc() will be the right call to make, how we handle the
>> legacy calls can be sorted out later if a generic SMC call is implemented.
> 
> Please see my comment regarding this patch earlier in this thread
> pasted below for convenience:
> 
> * Tony Lindgren <tony@atomide.com> [191119 16:22]:
>> In any case, you should do the necessary checks for HAVE_ARM_SMCCC
>> only once during init. I'm not sure how much checking for
>> "/firmware/optee" helps here, sounds like we have a broken system
>> if the firmware is not there while the arm_smccc_smc() should
>> still work just fine :)
> 
> So only check once during init. And during init, you should probably
> also check that arm_smccc_smc() actually infd optee if
> "/firmware/optee" is set, and only then set set the right function
> pointer or some flag.
> 

Okay, I'll check only once and make sure the node is "okay".

I'll do the check during the first call to an SMC caller, I wouldn't
want to pollute the OMAP generic init code for something that is only
called on HS platforms, plus these SMC calls are rare (only 3 calls
during boot of AM57x for instance) so performance is not critical, so I
don't want to do anything fancy like code patching :), I'll just use a flag.

Thanks,
Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Dec. 16, 2019, 10:41 p.m. UTC | #22
* Andrew F. Davis <afd@ti.com> [191216 22:34]:
> On 12/16/19 4:04 PM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [191216 20:57]:
> >> Looks like the TI quirk idea is not moving forward, even the QCOM quirk
> >> looks like it may get kicked out. arm_smccc_smc() will remain only for
> >> SMCCC compliant calls, but it looks like a generic arm_smc() wouldn't be
> >> too opposed upstream.
> > 
> > Yes so it seems.
> > 
> >> Either way this patch would still be valid as when OP-TEE is present
> >> then arm_smccc_smc() will be the right call to make, how we handle the
> >> legacy calls can be sorted out later if a generic SMC call is implemented.
> > 
> > Please see my comment regarding this patch earlier in this thread
> > pasted below for convenience:
> > 
> > * Tony Lindgren <tony@atomide.com> [191119 16:22]:
> >> In any case, you should do the necessary checks for HAVE_ARM_SMCCC
> >> only once during init. I'm not sure how much checking for
> >> "/firmware/optee" helps here, sounds like we have a broken system
> >> if the firmware is not there while the arm_smccc_smc() should
> >> still work just fine :)
> > 
> > So only check once during init. And during init, you should probably
> > also check that arm_smccc_smc() actually infd optee if
> > "/firmware/optee" is set, and only then set set the right function
> > pointer or some flag.
> > 
> 
> Okay, I'll check only once and make sure the node is "okay".

Yes we don't want to parse the dts over and over.

> I'll do the check during the first call to an SMC caller, I wouldn't
> want to pollute the OMAP generic init code for something that is only
> called on HS platforms, plus these SMC calls are rare (only 3 calls
> during boot of AM57x for instance) so performance is not critical, so I
> don't want to do anything fancy like code patching :), I'll just use a flag.

Please just add omap_early_initcall() to omap-secure.c while at it
to deal with this.

Regards,

Tony
Andrew Davis Dec. 17, 2019, 1:14 p.m. UTC | #23
On 12/16/19 5:41 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191216 22:34]:
>> On 12/16/19 4:04 PM, Tony Lindgren wrote:
>>> * Andrew F. Davis <afd@ti.com> [191216 20:57]:
>>>> Looks like the TI quirk idea is not moving forward, even the QCOM quirk
>>>> looks like it may get kicked out. arm_smccc_smc() will remain only for
>>>> SMCCC compliant calls, but it looks like a generic arm_smc() wouldn't be
>>>> too opposed upstream.
>>>
>>> Yes so it seems.
>>>
>>>> Either way this patch would still be valid as when OP-TEE is present
>>>> then arm_smccc_smc() will be the right call to make, how we handle the
>>>> legacy calls can be sorted out later if a generic SMC call is implemented.
>>>
>>> Please see my comment regarding this patch earlier in this thread
>>> pasted below for convenience:
>>>
>>> * Tony Lindgren <tony@atomide.com> [191119 16:22]:
>>>> In any case, you should do the necessary checks for HAVE_ARM_SMCCC
>>>> only once during init. I'm not sure how much checking for
>>>> "/firmware/optee" helps here, sounds like we have a broken system
>>>> if the firmware is not there while the arm_smccc_smc() should
>>>> still work just fine :)
>>>
>>> So only check once during init. And during init, you should probably
>>> also check that arm_smccc_smc() actually infd optee if
>>> "/firmware/optee" is set, and only then set set the right function
>>> pointer or some flag.
>>>
>>
>> Okay, I'll check only once and make sure the node is "okay".
> 
> Yes we don't want to parse the dts over and over.
> 
>> I'll do the check during the first call to an SMC caller, I wouldn't
>> want to pollute the OMAP generic init code for something that is only
>> called on HS platforms, plus these SMC calls are rare (only 3 calls
>> during boot of AM57x for instance) so performance is not critical, so I
>> don't want to do anything fancy like code patching :), I'll just use a flag.
> 
> Please just add omap_early_initcall() to omap-secure.c while at it
> to deal with this.
> 


omap_early_initcall()s are not called until after all the SMC calls have
already happened.

Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Dec. 17, 2019, 3:07 p.m. UTC | #24
* Andrew F. Davis <afd@ti.com> [191217 13:14]:
> On 12/16/19 5:41 PM, Tony Lindgren wrote:
> > Please just add omap_early_initcall() to omap-secure.c while at it
> > to deal with this.
>
> omap_early_initcall()s are not called until after all the SMC calls have
> already happened.

Oh OK. Then let's just add omap_secure_init() that's called from
*_init_early() as late as possible. We will have more use for that
init later on too.

Regards,

Tony
Andrew Davis Dec. 17, 2019, 5:01 p.m. UTC | #25
On 12/17/19 10:07 AM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191217 13:14]:
>> On 12/16/19 5:41 PM, Tony Lindgren wrote:
>>> Please just add omap_early_initcall() to omap-secure.c while at it
>>> to deal with this.
>>
>> omap_early_initcall()s are not called until after all the SMC calls have
>> already happened.
> 
> Oh OK. Then let's just add omap_secure_init() that's called from
> *_init_early() as late as possible. We will have more use for that
> init later on too.
> 


You mean add a call to this omap_secure_init() to every boards init
function?

Andrew


> Regards,
> 
> Tony
>
Tony Lindgren Dec. 17, 2019, 5:11 p.m. UTC | #26
* Andrew F. Davis <afd@ti.com> [191217 17:02]:
> On 12/17/19 10:07 AM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [191217 13:14]:
> >> On 12/16/19 5:41 PM, Tony Lindgren wrote:
> >>> Please just add omap_early_initcall() to omap-secure.c while at it
> >>> to deal with this.
> >>
> >> omap_early_initcall()s are not called until after all the SMC calls have
> >> already happened.
> > 
> > Oh OK. Then let's just add omap_secure_init() that's called from
> > *_init_early() as late as possible. We will have more use for that
> > init later on too.
> > 
> 
> 
> You mean add a call to this omap_secure_init() to every boards init
> function?

Yes please. We also have some SoCs that need clocks enabled
and disabled for omap_smc2() so we can then use that to
intialize the clocks as needed.

Regards,

Tony
Tony Lindgren Dec. 17, 2019, 5:18 p.m. UTC | #27
* Tony Lindgren <tony@atomide.com> [191217 17:11]:
> * Andrew F. Davis <afd@ti.com> [191217 17:02]:
> > On 12/17/19 10:07 AM, Tony Lindgren wrote:
> > > * Andrew F. Davis <afd@ti.com> [191217 13:14]:
> > >> On 12/16/19 5:41 PM, Tony Lindgren wrote:
> > >>> Please just add omap_early_initcall() to omap-secure.c while at it
> > >>> to deal with this.
> > >>
> > >> omap_early_initcall()s are not called until after all the SMC calls have
> > >> already happened.
> > > 
> > > Oh OK. Then let's just add omap_secure_init() that's called from
> > > *_init_early() as late as possible. We will have more use for that
> > > init later on too.
> > > 
> > 
> > 
> > You mean add a call to this omap_secure_init() to every boards init
> > function?
> 
> Yes please. We also have some SoCs that need clocks enabled
> and disabled for omap_smc2() so we can then use that to
> intialize the clocks as needed.

And please add the call to omap_secure_init() the SoC specific
*_init_early() functions in:

$ git grep 'void __init ' arch/arm/mach-omap2/io.c | grep early
...

Regards,

Tony
diff mbox series

Patch

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 6316da3623b3..ffcc580aef01 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -255,7 +255,7 @@  extern void gic_dist_disable(void);
 extern void gic_dist_enable(void);
 extern bool gic_dist_disabled(void);
 extern void gic_timer_retrigger(void);
-extern void omap_smc1(u32 fn, u32 arg);
+extern void _omap_smc1(u32 fn, u32 arg);
 extern void omap4_sar_ram_init(void);
 extern void __iomem *omap4_get_sar_ram_base(void);
 extern void omap4_mpuss_early_init(void);
diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index 24298e47b9f1..a2df3cb74bc3 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -12,10 +12,12 @@ 
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/memblock.h>
-
+#include <linux/of.h>
+#include <linux/arm-smccc.h>
 #include <asm/cacheflush.h>
 #include <asm/memblock.h>
 
+#include "common.h"
 #include "omap-secure.h"
 
 static phys_addr_t omap_secure_memblock_base;
@@ -53,6 +55,29 @@  u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, u32 arg1, u32 arg2,
 	return ret;
 }
 
+#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
+	ARM_SMCCC_OWNER_SIP, (func_num))
+
+void omap_smc1(u32 fn, u32 arg)
+{
+	struct device_node *optee;
+	struct arm_smccc_res res;
+
+	/*
+	 * If this platform has OP-TEE installed we use ARM SMC calls
+	 * otherwise fall back to the OMAP ROM style calls.
+	 */
+	optee = of_find_node_by_path("/firmware/optee");
+	if (optee) {
+		arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
+			      0, 0, 0, 0, 0, 0, &res);
+		WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
+	} else {
+		_omap_smc1(fn, arg);
+	}
+}
+
 /* Allocate the memory to save secure ram */
 int __init omap_secure_ram_reserve_memblock(void)
 {
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 20046e8f8ecb..601d64b2c744 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -60,6 +60,7 @@ 
 
 extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs,
 				u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+extern void omap_smc1(u32 fn, u32 arg);
 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);
diff --git a/arch/arm/mach-omap2/omap-smc.S b/arch/arm/mach-omap2/omap-smc.S
index fd2bcd91f4a1..d4832845a4e8 100644
--- a/arch/arm/mach-omap2/omap-smc.S
+++ b/arch/arm/mach-omap2/omap-smc.S
@@ -18,18 +18,18 @@ 
  * the monitor API number. It uses few CPU registers
  * internally and hence they need be backed up including
  * link register "lr".
- * Function signature : void omap_smc1(u32 fn, u32 arg)
+ * Function signature : void _omap_smc1(u32 fn, u32 arg)
  */
 	.arch armv7-a
 	.arch_extension sec
-ENTRY(omap_smc1)
+ENTRY(_omap_smc1)
 	stmfd   sp!, {r2-r12, lr}
 	mov	r12, r0
 	mov 	r0, r1
 	dsb
 	smc	#0
 	ldmfd   sp!, {r2-r12, pc}
-ENDPROC(omap_smc1)
+ENDPROC(_omap_smc1)
 
 /**
  * u32 omap_smc2(u32 id, u32 falg, u32 pargs)