Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org> wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
>> index ed9c853..d59bc19 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
>> @@ -32,3 +32,14 @@ board-specific compatible values:
>>    nvidia,whistler
>>    toradex,colibri_t20-512
>>    toradex,iris
>> +
>> +Optional nodes
>> +-------------------------------------------
>> +
>> +Trusted Foundations:
>> +Boards using the Trusted Foundations secure monitor should signal its
>> +presence by declaring a node compatible with "tl,trusted-foundations":
>> +
>> +     firmware {
>
> You need to make a clear statement about whether the node name is
>
> I think it shouldn't required to be exactly equal to "firmware", because
> that would lead to problems if there were multiple independent firmware
> APIs present (which is certainly possible, whether or not it is true
> on Tegra).

Yes, the name of the node is not fixed (doing so would make its lookup
faster, but the gain is not obvious). Will make it explicit in the
doc.

>> +             compatible = "tl,trusted-foundations";
>> +     };
>
> For now, it might make more sense to make this binding tegra-specific,
> and to interpret the node is only implying the presence of the low-
> level SoC functions you are using on Tegra, not TF as a whole.

Do you mean the vendor should be changed from "tl" to "nvidia" here?

> Otherwise, it feels too generic.  There is no description of exactly
> what functionality will be available if this node it present: if
> this is going to be a generic binding for TF, then it needs to work
> for all deployments of TF, not just your specific case.  For example,
> how to you find out what functionality is present?  Will there be
> a standard probing ABI for all versions and deployments of TF, or
> would extra information need to be described in the DT?
>
> Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will
> be present, working, and with compatible ABI and semantics, on every SoC
> where an implementation of TF is present.  Someone from Trusted Logic, or
> someone with visibility of the relevant ABI/API specs would need to
> judge on that -- do you have that info?

I'm currently looking into that. This patch makes the assumption that
all TF implementations have the same features and the same interface -
if this is the case, do you agree this binding is ok as it is?

If indeed TF's functionality and ABI is the same no matter whether we
are on Tegra on not, then its support should even be moved outside of
mach-tegra.


>> --- /dev/null
>> +++ b/arch/arm/mach-tegra/firmware.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * SecureOS support for Tegra CPUs
>
> Should the name "SecureOS" change in these comment headers? (affects
> multiple files)

Yes, I missed these ones, thanks. Another missed opportunity to use git grep...

>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
>> +             pr_warn("Trusted Foundations detected but support missing!\n");
>
> Should this be more than just a warning?
>
> It looks to me like tegra_cpu_reset_handler_set() might either silently
> fail or trigger an external abort in this situation, depending on the
> hardware and on how TF sets things up.

What will happen (from 3.10) is that tegra_cpu_reset_handler_set()
will output a "CPUX: failed to come online" for each secondary CPU and
boot will continue (albeit on one CPU). The system's features are
degraded, but it is not fatal, so I think it is reasonable to continue
here.

> There seems to be no way to signal an error when attempting to set a
> CPU's reset address.

Indeed. But even if that fails the system can still survive, at least on Tegra.

>> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
>> +     else if (node)
>> +             register_firmware_ops(&tegra_trusted_foundations_ops);
>> +#endif
>> +}
>
>
> IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer
> as:
>
>         node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>         if (!node)
>                 return;
>
>         if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) {
>                 pr_warn("Trusted Foundations detected but support missing!\n");
>                 return;
>         }
>
>         register_firmware_ops(&tegra_trusted_foundations_ops);

But then you will get a link error when TF support is not compiled in
because tegra_trusted_foundations_ops will not be defined. That's why
I used a #ifdef here - I agree it is terribly inelegant though.

>> +     asm volatile(
>> +             ".arch_extension        sec\n\t"
>> +             "stmfd  sp!, {r3 - r11, lr}\n\t"
>
> I think you don't need to save r3: the ARM PCS makes r3 caller-save, so
> it shouldn't matter if the SMC call causes it to get trashed.

One less register to save, I take it! :)

Thanks,
Alex.