LinuxLists.cc - [PATCH] ARM: tegra: add basic SecureOS support

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On 06/06/2013 06:35 PM, Russell King - ARM Linux wrote:
> On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote:
>> +static int __attribute__((used)) __tegra_smc_stack[10];
>> +
>> +/*
>> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
>> + * function arguments, but we prefer to play safe here and explicitly move
>> + * these values into the expected registers anyway. mov instructions without
>> + * any side-effect are turned into nops by the assembler, which limits
>> + * overhead.
>
> No they aren't. They will be preserved as:
> mov r0, r0
> mov r1, r1
> mov r2, r2

I'm pretty sure I checked with objdump and saw these replaced by nops at
some point, but for some reason I cannot get that behavior again. So
simply put, my statement is wrong indeed.

> Moreover, things will go wrong if the compiler decides for whatever reason
> to move 'arg' into r0 before calling your code sequence. So really this
> is quite fragile.
>
> There's also no point in mentioning EABI in the above paragraph; all ARM
> ABIs under Linux have always placed the arguments in r0..r3 and then on
> the stack. You can assert that this is always true by using the __asmeq()
> macro.

Good to know, thanks.

> Also...
>
>> + */
>> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
>> +{
>> + asm volatile(
>> + ".arch_extension sec\n\t"
>> + "ldr r3, =__tegra_smc_stack\n\t"
>> + "stmia r3, {r4-r12, lr}\n\t"
>
> using a statically allocated area to save the registers in this way is
> probably a bad move; although the hotplug code guarantees that there
> will be no concurrency between CPU hotplug operations, this sets a
> precident for it being used in places where no such protection exists.

Indeed. This function will be called from other places in the future,
and for these we cannot assume there will be no concurrency.

> What is wrong with stacking r4-r12, lr onto the SVC stack?

Nothing, actually. /o\

> You don't
> save the SVC stack pointer, so one can only assume that your SMC
> implmentation preserves this (if it doesn't, that's yet another bug
> with this assembly code.)

SVC stack pointer is ok AFAICT.

> Combining these two issues, you're probably far better off using an
> __attribute__((naked)) function for this - which means you get to
> write the entire function in assembly code without any compiler
> interference:
>
> static void __attribute__((naked)) tegra_generic_smc(u32 type, u32 subtype, u32 arg)
> {
> asm volatile(
> ".arch_extension sec\n\t"
> "stmfd sp!, {r4 - r12, lr}\n\t"
> __asmeq("%0", "r0")
> __asmeq("%1", "r1")
> __asmeq("%2", "r2")
> "mov r3, #0\n\t"
> "mov r4, #0\n\t"
> "dsb\n\t"
> "smc #0\n\t"
> "ldmfd sp!, {r4 - r12, pc}"
> :
> : "r" (type), "r" (subtype), "r" (arg)
> : "memory");
> }

Well, that works beautifully indeed, and results in a much smaller
function that does nothing more beyond what's needed. On top of that, I
feel enlightened.

Thanks, I will resubmit a fixed version soon.
Alex.

2013-06-06 10:37:57

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

Hi Tomasz,

On 06/06/2013 07:17 PM, Tomasz Figa wrote:
>> +Global properties
>> +-------------------------------------------
>> +
>> +The following properties can be specified into the "chosen" root
>> +node:
>> +
>> + nvidia,secure-os: enable SecureOS.
>
> Hmm, on Exynos we had something like
>
> firmware@0203F000 {
> compatible = "samsung,secure-firmware";
> reg = <0x0203F000 0x1000>;
> };
>
> but your solution might be actually the proper one, since firmware is not
> a hardware block. (The address in reg property is pointing to SYSRAM
> memory, which is an additional communication channel with the firmware.)

Yes, I saw your implementation but decided to do it through the chosen
node anyway, since that's what it seems to be designed and we don't need
any reg parameter.

> I think this patch could be split into several patches:
> - add support for firmware
> - split reset function
> - add reset support using firmware.

Mmm possibly yes, but I wonder if that would not be too much splitting.
Stephen?

> Hmm, I wonder if you need all this complexity here. Have a look at our
> exynos_smc function
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606

Yes, I just embarrassed myself showing my ignorance of ARM assembler. ;)
The fix Russel proposed is pretty close to your version.

>> +static const struct firmware_ops tegra_firmware_ops = {
>> + .set_cpu_boot_addr = tegra_set_cpu_boot_addr,
>> +};
>
> It's good that this interface is finally getting some user besides Exynos.

I didn't know about it first but Joseph kindly pointed it out to me and
it indeed makes it easier to implement this.

Thanks,
Alex.

2013-06-06 11:03:40

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote:
> Boot loaders on some Tegra devices can be unlocked but do not let the
> system operate without SecureOS. SecureOS prevents access to some
> registers and requires the operating system to perform certain
> operations through Secure Monitor Calls instead of directly accessing
> the hardware.
>
> This patch introduces basic SecureOS support for Tegra. SecureOS support
> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> node of the device tree.
>
> Currently, only the bringup of secondary CPUs is performed by SMCs, but
> more operations will be added later.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/tegra.txt | 8 +++
> arch/arm/configs/tegra_defconfig | 1 +
> arch/arm/mach-tegra/Kconfig | 11 ++++
> arch/arm/mach-tegra/Makefile | 2 +
> arch/arm/mach-tegra/common.c | 2 +
> arch/arm/mach-tegra/reset.c | 30 +++++++----
> arch/arm/mach-tegra/secureos.c | 70 +++++++++++++++++++++++++
> arch/arm/mach-tegra/secureos.h | 31 +++++++++++
> 8 files changed, 145 insertions(+), 10 deletions(-)
> create mode 100644 arch/arm/mach-tegra/secureos.c
> create mode 100644 arch/arm/mach-tegra/secureos.h
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
> index ed9c853..b543091 100644
> --- a/Documentation/devicetree/bindings/arm/tegra.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> @@ -32,3 +32,11 @@ board-specific compatible values:
> nvidia,whistler
> toradex,colibri_t20-512
> toradex,iris
> +
> +Global properties
> +-------------------------------------------
> +
> +The following properties can be specified into the "chosen" root
> +node:
> +
> + nvidia,secure-os: enable SecureOS.
> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
> index f7ba3161..f6ed0f5 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -28,6 +28,7 @@ CONFIG_ARCH_TEGRA_3x_SOC=y
> CONFIG_ARCH_TEGRA_114_SOC=y
> CONFIG_TEGRA_PCI=y
> CONFIG_TEGRA_EMC_SCALING_ENABLE=y
> +CONFIG_TEGRA_SECUREOS=y
> CONFIG_SMP=y
> CONFIG_PREEMPT=y
> CONFIG_AEABI=y
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index 84d72fc..acb5d0a 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -87,4 +87,15 @@ config TEGRA_AHB
> config TEGRA_EMC_SCALING_ENABLE
> bool "Enable scaling the memory frequency"
>
> +config TEGRA_SECUREOS
> + bool "Enable SecureOS support"
> + help
> + Support for Tegra devices which bootloader sets up a
> + SecureOS environment. This will use Secure Monitor Calls
> + instead of directly accessing the hardware for some protected
> + operations.
> +
> + SecureOS support is enabled by declaring a "nvidia,secure-os"
> + property into the "chosen" node of the device tree.
> +
> endmenu
> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
> index d011f0a..3adafe6 100644
> --- a/arch/arm/mach-tegra/Makefile
> +++ b/arch/arm/mach-tegra/Makefile
> @@ -37,3 +37,5 @@ endif
> obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o
>
> obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o
> +
> +obj-$(CONFIG_TEGRA_SECUREOS) += secureos.o
> diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
> index 9f852c6..b7eea02 100644
> --- a/arch/arm/mach-tegra/common.c
> +++ b/arch/arm/mach-tegra/common.c
> @@ -37,6 +37,7 @@
> #include "sleep.h"
> #include "pm.h"
> #include "reset.h"
> +#include "secureos.h"
>
> /*
> * Storage for debug-macro.S's state.
> @@ -97,6 +98,7 @@ static void __init tegra_init_cache(void)
>
> void __init tegra_init_early(void)
> {
> + tegra_init_secureos();
> tegra_cpu_reset_handler_init();
> tegra_apb_io_init();
> tegra_init_fuse();
> diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
> index 1ac434e..4b9ebf9 100644
> --- a/arch/arm/mach-tegra/reset.c
> +++ b/arch/arm/mach-tegra/reset.c
> @@ -21,38 +21,32 @@
>
> #include <asm/cacheflush.h>
> #include <asm/hardware/cache-l2x0.h>
> +#include <asm/firmware.h>
>
> #include "iomap.h"
> #include "irammap.h"
> #include "reset.h"
> #include "sleep.h"
> #include "fuse.h"
> +#include "secureos.h"
>
> #define TEGRA_IRAM_RESET_BASE (TEGRA_IRAM_BASE + \
> TEGRA_IRAM_RESET_HANDLER_OFFSET)
>
> static bool is_enabled;
>
> -static void __init tegra_cpu_reset_handler_enable(void)
> +static void __init tegra_cpu_reset_handler_set(const u32 reset_address)
> {
> - void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
> void __iomem *evp_cpu_reset =
> IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE + 0x100);
> void __iomem *sb_ctrl = IO_ADDRESS(TEGRA_SB_BASE);
> u32 reg;
>
> - BUG_ON(is_enabled);
> - BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
> -
> - memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
> - tegra_cpu_reset_handler_size);
> -
> /*
> * NOTE: This must be the one and only write to the EVP CPU reset
> * vector in the entire system.
> */
> - writel(TEGRA_IRAM_RESET_BASE + tegra_cpu_reset_handler_offset,
> - evp_cpu_reset);
> + writel(reset_address, evp_cpu_reset);
> wmb();
> reg = readl(evp_cpu_reset);
>
> @@ -66,6 +60,22 @@ static void __init tegra_cpu_reset_handler_enable(void)
> writel(reg, sb_ctrl);
> wmb();
> }
> +}
> +
> +static void __init tegra_cpu_reset_handler_enable(void)
> +{
> + void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
> + const u32 reset_address = TEGRA_IRAM_RESET_BASE +
> + tegra_cpu_reset_handler_offset;
> +
> + BUG_ON(is_enabled);
> + BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
> +
> + memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
> + tegra_cpu_reset_handler_size);
> +
> + if (call_firmware_op(set_cpu_boot_addr, 0, reset_address) == -ENOSYS)
> + tegra_cpu_reset_handler_set(reset_address);
>
> is_enabled = true;
> }
> diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c
> new file mode 100644
> index 0000000..44c3514
> --- /dev/null
> +++ b/arch/arm/mach-tegra/secureos.c
> @@ -0,0 +1,70 @@
> +/*
> + * SecureOS support for Tegra CPUs
> + *
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <asm/firmware.h>
> +
> +static int __attribute__((used)) __tegra_smc_stack[10];

Use __used instead of using GCC attributes directly.

> +
> +/*
> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
> + * function arguments, but we prefer to play safe here and explicitly move
> + * these values into the expected registers anyway. mov instructions without
> + * any side-effect are turned into nops by the assembler, which limits
> + * overhead.
> + */
> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
> +{
> + asm volatile(
> + ".arch_extension sec\n\t"
> + "ldr r3, =__tegra_smc_stack\n\t"

ldr= should be avoided in inline asm, because GCC can't guess it's size,
and can't guarantee that the literal pool word is close enough to be
addressable (though for small compilation units it's unlikely to be a
problem).

If you follow Russell's approach and define a naked function for this,
you can put an explicit literal word after the return:

ldr r3, 1f

/* ... */

/* return instruction */

.align 2
1: .long __tegra_smc_stack

> + "stmia r3, {r4-r12, lr}\n\t"
> + "mov r0, %[type]\n\t"
> + "mov r1, %[subtype]\n\t"
> + "mov r2, %[arg]\n\t"
> + "mov r3, #0\n\t"
> + "mov r4, #0\n\t"
> + "dsb\n\t"

Can you explain what this DSB is for?

> + "smc #0\n\t"
> + "ldr r3, =__tegra_smc_stack\n\t"
> + "ldmia r3, {r4-r12, lr}"
> + :
> + : [type] "r" (type),
> + [subtype] "r" (subtype),
> + [arg] "r" (arg)
> + : "r0", "r1", "r2", "r3", "r4", "memory");

If r5-r12 are not clobbered, why do you save and restore them?

In the ARM ABI, r12 is a caller-save register, so you probably
don't need to save/restore that even if it is clobbered.

Cheers
---Dave

2013-06-06 11:12:00

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 06, 2013 at 12:17:02PM +0200, Tomasz Figa wrote:
> Hi Alex,
>
> On Thursday 06 of June 2013 16:28:07 Alexandre Courbot wrote:
> > Boot loaders on some Tegra devices can be unlocked but do not let the
> > system operate without SecureOS. SecureOS prevents access to some
> > registers and requires the operating system to perform certain
> > operations through Secure Monitor Calls instead of directly accessing
> > the hardware.
> >
> > This patch introduces basic SecureOS support for Tegra. SecureOS support
> > can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> > node of the device tree.
> >
> > Currently, only the bringup of secondary CPUs is performed by SMCs, but
> > more operations will be added later.
> >
> > Signed-off-by: Alexandre Courbot <[email protected]>
> > ---
> > Documentation/devicetree/bindings/arm/tegra.txt | 8 +++
> > arch/arm/configs/tegra_defconfig | 1 +
> > arch/arm/mach-tegra/Kconfig | 11 ++++
> > arch/arm/mach-tegra/Makefile | 2 +
> > arch/arm/mach-tegra/common.c | 2 +
> > arch/arm/mach-tegra/reset.c | 30 +++++++----
> > arch/arm/mach-tegra/secureos.c | 70
> > +++++++++++++++++++++++++ arch/arm/mach-tegra/secureos.h
> > | 31 +++++++++++ 8 files changed, 145 insertions(+), 10 deletions(-)
> > create mode 100644 arch/arm/mach-tegra/secureos.c
> > create mode 100644 arch/arm/mach-tegra/secureos.h
> >
> > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
> > b/Documentation/devicetree/bindings/arm/tegra.txt index
> > ed9c853..b543091 100644
> > --- a/Documentation/devicetree/bindings/arm/tegra.txt
> > +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> > @@ -32,3 +32,11 @@ board-specific compatible values:
> > nvidia,whistler
> > toradex,colibri_t20-512
> > toradex,iris
> > +
> > +Global properties
> > +-------------------------------------------
> > +
> > +The following properties can be specified into the "chosen" root
> > +node:
> > +
> > + nvidia,secure-os: enable SecureOS.
>
> Hmm, on Exynos we had something like
>
> firmware@0203F000 {
> compatible = "samsung,secure-firmware";
> reg = <0x0203F000 0x1000>;
> };
>
> but your solution might be actually the proper one, since firmware is not
> a hardware block. (The address in reg property is pointing to SYSRAM
> memory, which is an additional communication channel with the firmware.)

Calling to SecureOS doesn't sound like a "choice" to me. It's part of the
platform, and if it's present then you have to use it, otherwise major
functionality (i.e., SMP) broken.

Having a proper node still makes sense, because you can put a
"compatible" property on it to track interface compatibility etc. This
was the view taken for PSCI (though in practice, we do have additional
information to put in the DT for that anyway).

Cheers
---Dave

2013-06-06 12:26:56

by Jassi Brar

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <[email protected]> wrote:
> Boot loaders on some Tegra devices can be unlocked but do not let the
> system operate without SecureOS. SecureOS prevents access to some
> registers and requires the operating system to perform certain
> operations through Secure Monitor Calls instead of directly accessing
> the hardware.
>
IOW, some critical h/w controls on Tegra are accessible only from
Secure mode (not unusual). So if we(Linux) run in NS mode we need to
make calls to the SecureOS, over SMC, to do things for us?

> This patch introduces basic SecureOS support for Tegra. SecureOS support
> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> node of the device tree.
>
Probably just a nit, but shouldn't it be "nvidia,nonsecure-os"
instead, denoting the mode Linux is going to run? (and then I wonder
if we could detect the mode (S or NS) at runtime and avoid this flag
at all).

Cheers,
-Jassi

2013-06-06 16:36:52

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On 06/06/2013 04:37 AM, Alex Courbot wrote:
> Hi Tomasz,
>
> On 06/06/2013 07:17 PM, Tomasz Figa wrote:
...
>> I think this patch could be split into several patches:
>> - add support for firmware
>> - split reset function
>> - add reset support using firmware.
>
> Mmm possibly yes, but I wonder if that would not be too much splitting.
> Stephen?

That's probably fine. It might make bisection easier if there are
regressions.

2013-06-06 16:44:54

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
> Boot loaders on some Tegra devices can be unlocked but do not let the
> system operate without SecureOS. SecureOS prevents access to some
> registers and requires the operating system to perform certain
> operations through Secure Monitor Calls instead of directly accessing
> the hardware.
>
> This patch introduces basic SecureOS support for Tegra. SecureOS support
> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> node of the device tree.

I suspect "SecureOS" here is the name of a specific implementation of a
secure monitor, right? It's certainly a very unfortunate name, since it
sounds like a generic concept rather than a specific implementation:-(

There certainly could be (and I believe are in practice?) multiple
implementation of a secure monitor for Tegra. Presumably, each of those
implementations has (or could have) a different definition for what SVC
calls it supports, their parameters, etc.

I think we need to separate the concept of support for *a* secure
monitor, from support for a *particular* secure monitor.

> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt

> +node:
> +
> + nvidia,secure-os: enable SecureOS.

... as such, I think some work is needed here to allow specification of
which secure monitor is present and/or which secure monitor ABI it
implements. The suggestion to use a specific DT node, and hence use
compatible values for this, seems reasonable. Alternatively, perhaps:

nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI";

might be reasonable, although using a node would allow ABI-specific
additional properties to be defined should they be needed, so I guess I
would lean towards that.

Similar comments may apply to the CONFIG_ option and description,
filenames, etc.

> diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c

> +void __init tegra_init_secureos(void)
> +{
> + struct device_node *node = of_find_node_by_path("/chosen");
> +
> + if (node && of_property_read_bool(node, "nvidia,secure-os"))
> + register_firmware_ops(&tegra_firmware_ops);
> +}

I'm tempted to think that function should /always/ exist an be called
(so no dummy inline in secureos.h).

In particular, what happens when a kernel without CONFIG_SECUREOS
enabled is booted under a secure monitor. Presumably we want the init
code to explicitly check for this condition, and either BUG(), or do
something to disable any features (like SMP) that require SVCs?

So, the algorithm here might be more like:

find node
if node exists
if (!IS_ENABLED(SECURE_OS))
BUG/WARN/...
else
register_firmware_ops()

?

2013-06-06 18:09:05

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote:
> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
> > Boot loaders on some Tegra devices can be unlocked but do not let the
> > system operate without SecureOS. SecureOS prevents access to some
> > registers and requires the operating system to perform certain
> > operations through Secure Monitor Calls instead of directly accessing
> > the hardware.
> >
> > This patch introduces basic SecureOS support for Tegra. SecureOS support
> > can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> > node of the device tree.
>
> I suspect "SecureOS" here is the name of a specific implementation of a
> secure monitor, right? It's certainly a very unfortunate name, since it
> sounds like a generic concept rather than a specific implementation:-(
>
> There certainly could be (and I believe are in practice?) multiple
> implementation of a secure monitor for Tegra. Presumably, each of those
> implementations has (or could have) a different definition for what SVC
> calls it supports, their parameters, etc.
>
> I think we need to separate the concept of support for *a* secure
> monitor, from support for a *particular* secure monitor.

There is no fixed set of functionality implemented by these interfaces,
so it might be better to think in terms of a generic "firmware" concept.

Come to think of it...

One option could be to have some standard baseline firmware calling
conventions, so that we could have a few specific backends -- perhaps
this could be built on the "method" notion used by PSCI

(see Documentation/devicetree/bindings/arm/psci.tst; this is probably
the most developed firmware interface binding we have today)

There, method = "smc" means:

populate registers in a certain way
SMC #0
return results from register to caller in a certain way

and method = "hvc" means:

populate registers in a certain way
HVC #0
return results from register to caller in a certain way

The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc()
is probably close to what's needed for the tegra secureos case,
so in theory it could be common, along with some of the DT binding
conventions.

The backends, and the convention for binding a firmware interface
to the appropriate backend, could then theoretically be handled
by a common framework.

Firmwares with strange calling conventions which don't fit the
standard backend could still add another one, within the framework.

>
> > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
>
> > +node:
> > +
> > + nvidia,secure-os: enable SecureOS.
>
> ... as such, I think some work is needed here to allow specification of
> which secure monitor is present and/or which secure monitor ABI it
> implements. The suggestion to use a specific DT node, and hence use
> compatible values for this, seems reasonable. Alternatively, perhaps:
>
> nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI";

So, something like

foo-firmware {
compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware";
method = "smc";

// ...
};

bar-firmware {
compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware";
method = "smc";

// ...
};

Could make sense.

...where we would require all new firmware interface bindings to
include the "-firmware" in their node names and compatible strings
(with a version suffix encouraged for the compatible strings, where
relevant).

This could be a good opportunity to make things a bit more consistent,
otherwise we will just reinvent this over and over.

(Unfortunately the node for PSCI offers no clue that it describes a
firmware interface, unless you go and read the binding documentation.
It may be too late to fix that, but we can at least avoid repeating
the mistake.)

> might be reasonable, although using a node would allow ABI-specific
> additional properties to be defined should they be needed, so I guess I
> would lean towards that.
>
> Similar comments may apply to the CONFIG_ option and description,
> filenames, etc.
>
> > diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c
>
> > +void __init tegra_init_secureos(void)
> > +{
> > + struct device_node *node = of_find_node_by_path("/chosen");
> > +
> > + if (node && of_property_read_bool(node, "nvidia,secure-os"))
> > + register_firmware_ops(&tegra_firmware_ops);
> > +}
>
> I'm tempted to think that function should /always/ exist an be called
> (so no dummy inline in secureos.h).
>
> In particular, what happens when a kernel without CONFIG_SECUREOS
> enabled is booted under a secure monitor. Presumably we want the init
> code to explicitly check for this condition, and either BUG(), or do
> something to disable any features (like SMP) that require SVCs?
>
> So, the algorithm here might be more like:
>
> find node
> if node exists
> if (!IS_ENABLED(SECURE_OS))
> BUG/WARN/...
> else
> register_firmware_ops()

Agreed, something like that would be needed, but it depends on the
firmware interface being described.

Cheers
---Dave

2013-06-06 18:29:20

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On 06/06/2013 12:08 PM, Dave Martin wrote:
> On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote:
>> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
>>> Boot loaders on some Tegra devices can be unlocked but do not let the
>>> system operate without SecureOS. SecureOS prevents access to some
>>> registers and requires the operating system to perform certain
>>> operations through Secure Monitor Calls instead of directly accessing
>>> the hardware.
>>>
>>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>>> node of the device tree.
...
> So, something like
>
> foo-firmware {
> compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware";
> method = "smc";
>
> // ...
> };
>
> bar-firmware {
> compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware";
> method = "smc";
>
> // ...
> };
>
> Could make sense.

I'm not sure if the method property is useful in most cases. For generic
ABIs such as PSCI that actually support multiple communication paths,
yes, it makes sense. However, it seems like for most custom ABIs, such
as is presumably implemented by whatever "SecureOS" is on Tegra, the
firmware type (or compatible value) directly implies that it operates
over SMC not HVC. After all, presumably if the kernel was virtualized,
it wouldn't have access to the raw secure monitor? I suppose you could
argue that the hypervisor might end up emulating the same ABI over HVC
instead? I'm not sure how likely that is. A compromise might be to
assume SMC if no property was present, which would allow it to be
optionally added later if it turned out to be useful.

> ...where we would require all new firmware interface bindings to
> include the "-firmware" in their node names and compatible strings
> (with a version suffix encouraged for the compatible strings, where
> relevant).

That's probably a good convention, but I'm not sure it should be
required (or at least validated by code). Requiring this by code review
might be OK. Node names aren't meant to be interpreted semantically.

2013-06-07 07:14:21

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 6, 2013 at 9:26 PM, Jassi Brar <[email protected]> wrote:
> On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <[email protected]> wrote:
>> Boot loaders on some Tegra devices can be unlocked but do not let the
>> system operate without SecureOS. SecureOS prevents access to some
>> registers and requires the operating system to perform certain
>> operations through Secure Monitor Calls instead of directly accessing
>> the hardware.
>>
> IOW, some critical h/w controls on Tegra are accessible only from
> Secure mode (not unusual). So if we(Linux) run in NS mode we need to
> make calls to the SecureOS, over SMC, to do things for us?

Exactly.

>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>> node of the device tree.
>>
> Probably just a nit, but shouldn't it be "nvidia,nonsecure-os"
> instead, denoting the mode Linux is going to run? (and then I wonder
> if we could detect the mode (S or NS) at runtime and avoid this flag
> at all).

Detection of the secure mode at runtime would only solve half of the
issue: we would know that we are running in non-secure mode, but we
would still not know what monitor is operating. Detecting that part is
impossible AFAIK, so I'm afraid we need to pass that information
through the DT here.

Alex.

2013-06-07 07:25:30

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 6, 2013 at 8:02 PM, Dave Martin <[email protected]> wrote:

>> +static int __attribute__((used)) __tegra_smc_stack[10];
>
> Use __used instead of using GCC attributes directly.
>
>> +
>> +/*
>> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
>> + * function arguments, but we prefer to play safe here and explicitly move
>> + * these values into the expected registers anyway. mov instructions without
>> + * any side-effect are turned into nops by the assembler, which limits
>> + * overhead.
>> + */
>> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
>> +{
>> + asm volatile(
>> + ".arch_extension sec\n\t"
>> + "ldr r3, =__tegra_smc_stack\n\t"
>
> ldr= should be avoided in inline asm, because GCC can't guess it's size,
> and can't guarantee that the literal pool word is close enough to be
> addressable (though for small compilation units it's unlikely to be a
> problem).

With Russel's suggested changes this will go away, but that's good to
know anyway.

>> + "dsb\n\t"
>
> Can you explain what this DSB is for?

Just a safety measure to make sure all memory operations are done
before we enter the secure monitor. Is it unnecessary?

>> + "smc #0\n\t"
>> + "ldr r3, =__tegra_smc_stack\n\t"
>> + "ldmia r3, {r4-r12, lr}"
>> + :
>> + : [type] "r" (type),
>> + [subtype] "r" (subtype),
>> + [arg] "r" (arg)
>> + : "r0", "r1", "r2", "r3", "r4", "memory");
>
> If r5-r12 are not clobbered, why do you save and restore them?

The secure monitor might change them.

> In the ARM ABI, r12 is a caller-save register, so you probably
> don't need to save/restore that even if it is clobbered.

Right, thanks. Didn't know r12 could be used as a scratch register.

Alex.

2013-06-07 08:12:14

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 7, 2013 at 1:44 AM, Stephen Warren <[email protected]> wrote:
> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
>> Boot loaders on some Tegra devices can be unlocked but do not let the
>> system operate without SecureOS. SecureOS prevents access to some
>> registers and requires the operating system to perform certain
>> operations through Secure Monitor Calls instead of directly accessing
>> the hardware.
>>
>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>> node of the device tree.
>
> I suspect "SecureOS" here is the name of a specific implementation of a
> secure monitor, right? It's certainly a very unfortunate name, since it
> sounds like a generic concept rather than a specific implementation:-(

Right. Using the actual name (Trusted Foundations) is probably better.
I don't think the SecureOS denomination is used by anyone else but
NVIDIA.

> There certainly could be (and I believe are in practice?) multiple
> implementation of a secure monitor for Tegra. Presumably, each of those
> implementations has (or could have) a different definition for what SVC
> calls it supports, their parameters, etc.
>
> I think we need to separate the concept of support for *a* secure
> monitor, from support for a *particular* secure monitor.

Agreed. In this case, can we assume that support for a specific secure
monitor is not arch-specific, and that this patch should be moved
outside of arch-tegra and down to arch/arm? In other words, the ABI of
a particular secure monitor should be the same no matter the chip,
shouldn't it?

>> +node:
>> +
>> + nvidia,secure-os: enable SecureOS.
>
> ... as such, I think some work is needed here to allow specification of
> which secure monitor is present and/or which secure monitor ABI it
> implements. The suggestion to use a specific DT node, and hence use
> compatible values for this, seems reasonable. Alternatively, perhaps:
>
> nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI";
>
> might be reasonable, although using a node would allow ABI-specific
> additional properties to be defined should they be needed, so I guess I
> would lean towards that.

Considering your previous comment, I agree this seems to make the most sense.

> Similar comments may apply to the CONFIG_ option and description,
> filenames, etc.
>
>> diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c
>
>> +void __init tegra_init_secureos(void)
>> +{
>> + struct device_node *node = of_find_node_by_path("/chosen");
>> +
>> + if (node && of_property_read_bool(node, "nvidia,secure-os"))
>> + register_firmware_ops(&tegra_firmware_ops);
>> +}
>
> I'm tempted to think that function should /always/ exist an be called
> (so no dummy inline in secureos.h).
>
> In particular, what happens when a kernel without CONFIG_SECUREOS
> enabled is booted under a secure monitor. Presumably we want the init
> code to explicitly check for this condition, and either BUG(), or do
> something to disable any features (like SMP) that require SVCs?
>
> So, the algorithm here might be more like:
>
> find node
> if node exists
> if (!IS_ENABLED(SECURE_OS))
> BUG/WARN/...
> else
> register_firmware_ops()
>
> ?

Yep, let's do it this way.

Alex.

2013-06-07 08:52:14

by Jassi Brar

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 7, 2013 at 12:43 PM, Alexandre Courbot <[email protected]> wrote:
> On Thu, Jun 6, 2013 at 9:26 PM, Jassi Brar <[email protected]> wrote:
>> On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <[email protected]> wrote:
>>> Boot loaders on some Tegra devices can be unlocked but do not let the
>>> system operate without SecureOS. SecureOS prevents access to some
>>> registers and requires the operating system to perform certain
>>> operations through Secure Monitor Calls instead of directly accessing
>>> the hardware.
>>>
>> IOW, some critical h/w controls on Tegra are accessible only from
>> Secure mode (not unusual). So if we(Linux) run in NS mode we need to
>> make calls to the SecureOS, over SMC, to do things for us?
>
> Exactly.
>
OK so this just an instance of a situation that is going to be more
common in future - most modern cpus (CortexA based at least) support
TrustZone and it's a matter of device functionality before a board
designer chucks the Linux kernel (that we usually run in Secure mode
while development) into NS mode and have some
SecureOS/TrustedOS/Hypervisor et al control access to the secure
peripherals from Secure Mode.

>>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>>> node of the device tree.
>>>
>> Probably just a nit, but shouldn't it be "nvidia,nonsecure-os"
>> instead, denoting the mode Linux is going to run? (and then I wonder
>> if we could detect the mode (S or NS) at runtime and avoid this flag
>> at all).
>
> Detection of the secure mode at runtime would only solve half of the
> issue: we would know that we are running in non-secure mode, but we
> would still not know what monitor is operating. Detecting that part is
> impossible AFAIK, so I'm afraid we need to pass that information
> through the DT here.
>
Yes, Stephen's suggestion is far more generally applicable. We should
go for it, just bearing in mind that ABI need and type is going to be
board specific.

Cheers,
-Jassi

2013-06-07 09:04:18

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 7, 2013 at 3:08 AM, Dave Martin <[email protected]> wrote:
>> I think we need to separate the concept of support for *a* secure
>> monitor, from support for a *particular* secure monitor.
>
> There is no fixed set of functionality implemented by these interfaces,
> so it might be better to think in terms of a generic "firmware" concept.
>
>
> Come to think of it...
>
> One option could be to have some standard baseline firmware calling
> conventions, so that we could have a few specific backends -- perhaps
> this could be built on the "method" notion used by PSCI
>
> (see Documentation/devicetree/bindings/arm/psci.tst; this is probably
> the most developed firmware interface binding we have today)
>
> There, method = "smc" means:
>
> populate registers in a certain way
> SMC #0
> return results from register to caller in a certain way
>
> and method = "hvc" means:
>
> populate registers in a certain way
> HVC #0
> return results from register to caller in a certain way
>
>
> The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc()
> is probably close to what's needed for the tegra secureos case,
> so in theory it could be common, along with some of the DT binding
> conventions.
>
> The backends, and the convention for binding a firmware interface
> to the appropriate backend, could then theoretically be handled
> by a common framework.

I'm not sure whether we could use the same backend for many different
firmwares. If I understand you correctly, you propose to have a
backend to the "smc" call that would cover the needs of all firmwares
that rely on the smc instruction to invoke the firmware/secure
monitor.

I can understand the logic, but I'm not sure this is needed or even
possible. For instance, the implementation you have in
__invoke_psci_fn_smc assumes 4 arguments, while Tegra's only needs 3.
Also (and although I have to confess I am not very knowledgeable about
the "SecureOS" covered by this patch and need to double-check what
follows), in Tegra's case registers r3-r11 can be altered by the
secure monitor and need to be preserved - something you don't need to
do with PSCI.

Another example is the function that Tomasz shown
(https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
), which preserves r4-r11 but also assumes r3 is an argument - that's
again another slightly different convention.

All in all the needs of the various firmwares might end up being just
different enough that we need to have a different backend for each of
them. The firmware_ops defined in arch/arm/include/asm/firmware.h
perform the abstraction at a higher level, which seems more fit here
IMHO.

Alex.

2013-06-07 16:33:16

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On 06/07/2013 02:11 AM, Alexandre Courbot wrote:
> On Fri, Jun 7, 2013 at 1:44 AM, Stephen Warren <[email protected]> wrote:
>> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
>>> Boot loaders on some Tegra devices can be unlocked but do not let the
>>> system operate without SecureOS. SecureOS prevents access to some
>>> registers and requires the operating system to perform certain
>>> operations through Secure Monitor Calls instead of directly accessing
>>> the hardware.
>>>
>>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>>> node of the device tree.
>>
>> I suspect "SecureOS" here is the name of a specific implementation of a
>> secure monitor, right? It's certainly a very unfortunate name, since it
>> sounds like a generic concept rather than a specific implementation:-(
>
> Right. Using the actual name (Trusted Foundations) is probably better.
> I don't think the SecureOS denomination is used by anyone else but
> NVIDIA.
>
>> There certainly could be (and I believe are in practice?) multiple
>> implementation of a secure monitor for Tegra. Presumably, each of those
>> implementations has (or could have) a different definition for what SVC
>> calls it supports, their parameters, etc.
>>
>> I think we need to separate the concept of support for *a* secure
>> monitor, from support for a *particular* secure monitor.
>
> Agreed. In this case, can we assume that support for a specific secure
> monitor is not arch-specific, and that this patch should be moved
> outside of arch-tegra and down to arch/arm? In other words, the ABI of
> a particular secure monitor should be the same no matter the chip,
> shouldn't it?

I would like to believe that the Trusted Foundations monitor had the
same ABI irrespective of which Soc it was running on. However, I have
absolutely no idea at all if that's true. Even if there's some common
subset of the ABI that is identical across all SoCs, I wouldn't be too
surprised if there were custom extensions for each different SoC, or
just perhaps even each product.

Can you research this and find out the answer?

What we can always do is make a compatible property that lists
everything[1], and have the driver match on the most specific value for
now, but relax the driver's matching later if it turns out that the ABI
is indeed common.

[1] That'd need to be at least secure OS name, and secure OS version.
Perhaps the SoC and board data can be deduced from the DT's top-level
compatible properties; nvidia,tegra114-shield, nvidia,tegra114?

2013-06-07 17:30:51

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 07, 2013 at 04:25:07PM +0900, Alexandre Courbot wrote:
> On Thu, Jun 6, 2013 at 8:02 PM, Dave Martin <[email protected]> wrote:
>
> >> +static int __attribute__((used)) __tegra_smc_stack[10];
> >
> > Use __used instead of using GCC attributes directly.
> >
> >> +
> >> +/*
> >> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
> >> + * function arguments, but we prefer to play safe here and explicitly move
> >> + * these values into the expected registers anyway. mov instructions without
> >> + * any side-effect are turned into nops by the assembler, which limits
> >> + * overhead.
> >> + */
> >> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
> >> +{
> >> + asm volatile(
> >> + ".arch_extension sec\n\t"
> >> + "ldr r3, =__tegra_smc_stack\n\t"
> >
> > ldr= should be avoided in inline asm, because GCC can't guess it's size,
> > and can't guarantee that the literal pool word is close enough to be
> > addressable (though for small compilation units it's unlikely to be a
> > problem).
>
> With Russel's suggested changes this will go away, but that's good to
> know anyway.
>
> >> + "dsb\n\t"
> >
> > Can you explain what this DSB is for?
>
> Just a safety measure to make sure all memory operations are done
> before we enter the secure monitor. Is it unnecessary?

Most likely it's either unnecessary, or insufficient.

Just for entering call SMC properly, it's not needed. If the Secure
World has its MMU on and maps Normal World memory using the same memory
types as Linux, then the Normal World and Secure World access the memory
coherently with no extra barrier needed.

It the Secure World's MMU is off, or if it maps Normal World memory
as Secure (pagetable NS bit = 0), or if it maps Normal World memory with
memory types incompatible with the ones Linux is using then you will get
coherency problems: the Secure and Normal Worlds won't necessarily see
the same data.

In the latter case, you must do explicit cache maintenance around SMC
for the data you want to exchange. This might also be needed in the
Secure World if caches are enabled over there. DSB isn't enough by
itself.

If the Secure World doesn't access Normal World memory at all, you
don't need to do anything special and can remove the DSB.

Otherwise, for performance reasons, it is definitely preferable to have
the Secure World MMU on if possible, though it's a bit more complex to
set up.

> >> + "smc #0\n\t"
> >> + "ldr r3, =__tegra_smc_stack\n\t"
> >> + "ldmia r3, {r4-r12, lr}"
> >> + :
> >> + : [type] "r" (type),
> >> + [subtype] "r" (subtype),
> >> + [arg] "r" (arg)
> >> + : "r0", "r1", "r2", "r3", "r4", "memory");
> >
> > If r5-r12 are not clobbered, why do you save and restore them?
>
> The secure monitor might change them.

Sure, but you could just add all the registers to the clobber list,
and then the compiler would save and restore them for you in the
function entry/exit sequences, which may be a bit more efficient.

Since you are going to replace this code anyway, it's academic
though.

>
> > In the ARM ABI, r12 is a caller-save register, so you probably
> > don't need to save/restore that even if it is clobbered.
>
> Right, thanks. Didn't know r12 could be used as a scratch register.

What I said is a bit wrong actually: for the naked version of the
function you can go ahead and corrupt r12, because naked functions
are not inlinable and follow the ABI. Adding "r12" in the clobber
list would be harmless in this case, but I don't think it's necessary.

asm() in any other context needs to declare all registers that it
might clobber.

Cheers
---Dave

2013-06-07 17:48:13

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Thu, Jun 06, 2013 at 12:29:14PM -0600, Stephen Warren wrote:
> On 06/06/2013 12:08 PM, Dave Martin wrote:
> > On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote:
> >> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
> >>> Boot loaders on some Tegra devices can be unlocked but do not let the
> >>> system operate without SecureOS. SecureOS prevents access to some
> >>> registers and requires the operating system to perform certain
> >>> operations through Secure Monitor Calls instead of directly accessing
> >>> the hardware.
> >>>
> >>> This patch introduces basic SecureOS support for Tegra. SecureOS support
> >>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> >>> node of the device tree.
> ...
> > So, something like
> >
> > foo-firmware {
> > compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware";
> > method = "smc";
> >
> > // ...
> > };
> >
> > bar-firmware {
> > compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware";
> > method = "smc";
> >
> > // ...
> > };
> >
> > Could make sense.
>
> I'm not sure if the method property is useful in most cases. For generic

I was mostly illustrating. It's not useful in most cases, unless we
have standard code parsing these bindings (probably overkill).

> ABIs such as PSCI that actually support multiple communication paths,
> yes, it makes sense. However, it seems like for most custom ABIs, such
> as is presumably implemented by whatever "SecureOS" is on Tegra, the
> firmware type (or compatible value) directly implies that it operates
> over SMC not HVC. After all, presumably if the kernel was virtualized,
> it wouldn't have access to the raw secure monitor? I suppose you could
> argue that the hypervisor might end up emulating the same ABI over HVC
> instead? I'm not sure how likely that is. A compromise might be to
> assume SMC if no property was present, which would allow it to be
> optionally added later if it turned out to be useful.

Sure.

For most firmware interface bindings, the backend could be implied.
The binding could be extended later if an alternative backend were needed.

If an SMC-called firmware interface is visible to a guest running under
a hypervisor at all, it is probably still callable using SMC, since
hypervisors can trap and proxy/emulate it instead of the call bypassing
the hypervisor and going straight to the Secure World.

If the interface is not visible, the hypervisor shouldn't put the relevant
node in the DT supplied to the guest (just as any physical hardware not
accessible to the guest shouldn't be described in the DT).

Firmware interfaces which don't specifically depend on the Security
Extensions need multiple backends, because hypervisors can't trap SMC on
ARMv8 platforms which don't implement the Security Extensions -- this
was an issue for PSCI, and it's why HVC is used instead by KVM etc.

For "Secure OS" interfaces this won't be an issue since those interfaces
don't make sense if the Security Extensions aren't there.

> > ...where we would require all new firmware interface bindings to
> > include the "-firmware" in their node names and compatible strings
> > (with a version suffix encouraged for the compatible strings, where
> > relevant).
>
> That's probably a good convention, but I'm not sure it should be
> required (or at least validated by code). Requiring this by code review
> might be OK. Node names aren't meant to be interpreted semantically.

Agreed: I don't think this makes sense as a formal binding, but rather
it's a convention we can follow which avoids DTs being unnecessarily
cryptic.

Cheers
---Dave

2013-06-07 18:13:54

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 07, 2013 at 06:03:54PM +0900, Alexandre Courbot wrote:
> On Fri, Jun 7, 2013 at 3:08 AM, Dave Martin <[email protected]> wrote:
> >> I think we need to separate the concept of support for *a* secure
> >> monitor, from support for a *particular* secure monitor.
> >
> > There is no fixed set of functionality implemented by these interfaces,
> > so it might be better to think in terms of a generic "firmware" concept.
> >
> >
> > Come to think of it...
> >
> > One option could be to have some standard baseline firmware calling
> > conventions, so that we could have a few specific backends -- perhaps
> > this could be built on the "method" notion used by PSCI
> >
> > (see Documentation/devicetree/bindings/arm/psci.tst; this is probably
> > the most developed firmware interface binding we have today)
> >
> > There, method = "smc" means:
> >
> > populate registers in a certain way
> > SMC #0
> > return results from register to caller in a certain way
> >
> > and method = "hvc" means:
> >
> > populate registers in a certain way
> > HVC #0
> > return results from register to caller in a certain way
> >
> >
> > The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc()
> > is probably close to what's needed for the tegra secureos case,
> > so in theory it could be common, along with some of the DT binding
> > conventions.
> >
> > The backends, and the convention for binding a firmware interface
> > to the appropriate backend, could then theoretically be handled
> > by a common framework.
>
> I'm not sure whether we could use the same backend for many different
> firmwares. If I understand you correctly, you propose to have a
> backend to the "smc" call that would cover the needs of all firmwares
> that rely on the smc instruction to invoke the firmware/secure
> monitor.
>
> I can understand the logic, but I'm not sure this is needed or even
> possible. For instance, the implementation you have in
> __invoke_psci_fn_smc assumes 4 arguments, while Tegra's only needs 3.
> Also (and although I have to confess I am not very knowledgeable about
> the "SecureOS" covered by this patch and need to double-check what
> follows), in Tegra's case registers r3-r11 can be altered by the
> secure monitor and need to be preserved - something you don't need to
> do with PSCI.

One way to make the backend generic would be to have something like
one of the following (some syntax omitted due to laziness):

u32 __naked __call_smc(u32 r0, ...)
{
asm volatile (
stmfd sp!, {r4-r11,lr}
smc #0
ldmfd sp!, {r4-r11,pc}
::: "memory"
);
}

/* The above works for up to 4 u32 arguments */

u32 __naked __call_smc(u32 r0, ...)
{
asm volatile (
mov ip, sp
stmfd sp!, {r4-r11,lr}
ldmia ip, {r4-r11}
smc #0
ldmfd sp!, {r4-r11,pc}
::: "memory"
);
}

/*
* Works for up to 13 u32 arguments, provided the stack is deep
* enough to provide suitable garbage data to fill the registers
* if the caller supplied fewer arguments (a bit of a hack)
*/

u32 __naked __call_smc(struct pt_regs *regs) {

asm(
stmfd sp!, {r4-r11,lr}
/* load regs from <regs> */
smc #0
/* save regs back to <regs> */
ldmfd sp!, {r4-r11,pc}
);
}

/*
* Most generic, least-efficient version.
* Can return up to 13 u32 results instead of just one.
* For convenience, the r0 value returned by the SMC could be
* left in r0 so that it also determines the return value of the
* function.
*
* Most of the time, SMC shouldn't be called on any hot path,
* otherwise the performance battle is already lost -- so it may
* not be crucial to reach the maximum possible efficiency for
* these calls.
*/

A particular firmware's Linux glue code might have to put extra stuff
around calls to generic_smc, but at least generic_smc itself wouldn't
need to be reinvented, and the firmware-specific glue code could usually
avoid asm.

> Another example is the function that Tomasz shown
> (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
> ), which preserves r4-r11 but also assumes r3 is an argument - that's
> again another slightly different convention.

... for which the above implementations of __call_smc() should work too.

> All in all the needs of the various firmwares might end up being just
> different enough that we need to have a different backend for each of
> them. The firmware_ops defined in arch/arm/include/asm/firmware.h
> perform the abstraction at a higher level, which seems more fit here
> IMHO.
>
> Alex.

Indeed. If you think you could work with one of the above generics, we
could try it and see what it looks like though.

If it's an awkward fit, I might be being too optimistic.

Cheers
---Dave

2013-06-10 07:47:44

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Sat, Jun 8, 2013 at 2:30 AM, Dave Martin <[email protected]> wrote:
> Most likely it's either unnecessary, or insufficient.
>
> Just for entering call SMC properly, it's not needed. If the Secure
> World has its MMU on and maps Normal World memory using the same memory
> types as Linux, then the Normal World and Secure World access the memory
> coherently with no extra barrier needed.
>
> It the Secure World's MMU is off, or if it maps Normal World memory
> as Secure (pagetable NS bit = 0), or if it maps Normal World memory with
> memory types incompatible with the ones Linux is using then you will get
> coherency problems: the Secure and Normal Worlds won't necessarily see
> the same data.
>
> In the latter case, you must do explicit cache maintenance around SMC
> for the data you want to exchange. This might also be needed in the
> Secure World if caches are enabled over there. DSB isn't enough by
> itself.
>
>
> If the Secure World doesn't access Normal World memory at all, you
> don't need to do anything special and can remove the DSB.
>
> Otherwise, for performance reasons, it is definitely preferable to have
> the Secure World MMU on if possible, though it's a bit more complex to
> set up.

Thanks for the information. I will try to understand why we put it
here in the first place.

>> >> + "smc #0\n\t"
>> >> + "ldr r3, =__tegra_smc_stack\n\t"
>> >> + "ldmia r3, {r4-r12, lr}"
>> >> + :
>> >> + : [type] "r" (type),
>> >> + [subtype] "r" (subtype),
>> >> + [arg] "r" (arg)
>> >> + : "r0", "r1", "r2", "r3", "r4", "memory");
>> >
>> > If r5-r12 are not clobbered, why do you save and restore them?
>>
>> The secure monitor might change them.
>
> Sure, but you could just add all the registers to the clobber list,
> and then the compiler would save and restore them for you in the
> function entry/exit sequences, which may be a bit more efficient.
>
> Since you are going to replace this code anyway, it's academic
> though.

Right. I suppose you mentioned this in the context of my initial code
- however if I understand how inline asm works (in case it wasn't
clear already, I probably don't), turning this function into a naked
function will make it impossible for the compiler to generate the
entry/exit sequences since the assembler code will be responsible for
returning from the function.

One could remove the naked attribute and put there registers into the
clobber list, but then the function will be inlined and we will have
to ensure the parameters end up in the right register (and having a
function that cannot be inlined is convenient in that respect). So as
far as I can tell, having the function naked and saving the registers
ourselves seems to be the most convenient way around this.

Thanks,
Alex.

2013-06-10 08:05:27

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Sat, Jun 8, 2013 at 3:13 AM, Dave Martin <[email protected]> wrote:
> One way to make the backend generic would be to have something like
> one of the following (some syntax omitted due to laziness):
>
> u32 __naked __call_smc(u32 r0, ...)
> {
> asm volatile (
> stmfd sp!, {r4-r11,lr}
> smc #0
> ldmfd sp!, {r4-r11,pc}
> ::: "memory"
> );
> }
>
> /* The above works for up to 4 u32 arguments */
>
> u32 __naked __call_smc(u32 r0, ...)
> {
> asm volatile (
> mov ip, sp
> stmfd sp!, {r4-r11,lr}
> ldmia ip, {r4-r11}
> smc #0
> ldmfd sp!, {r4-r11,pc}
> ::: "memory"
> );
> }
>
> /*
> * Works for up to 13 u32 arguments, provided the stack is deep
> * enough to provide suitable garbage data to fill the registers
> * if the caller supplied fewer arguments (a bit of a hack)
> */
>
> u32 __naked __call_smc(struct pt_regs *regs) {
>
> asm(
> stmfd sp!, {r4-r11,lr}
> /* load regs from <regs> */
> smc #0
> /* save regs back to <regs> */
> ldmfd sp!, {r4-r11,pc}
> );
> }
>
> /*
> * Most generic, least-efficient version.
> * Can return up to 13 u32 results instead of just one.
> * For convenience, the r0 value returned by the SMC could be
> * left in r0 so that it also determines the return value of the
> * function.
> *
> * Most of the time, SMC shouldn't be called on any hot path,
> * otherwise the performance battle is already lost -- so it may
> * not be crucial to reach the maximum possible efficiency for
> * these calls.
> */
>
>
> A particular firmware's Linux glue code might have to put extra stuff
> around calls to generic_smc, but at least generic_smc itself wouldn't
> need to be reinvented, and the firmware-specific glue code could usually
> avoid asm.
>
>> Another example is the function that Tomasz shown
>> (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
>> ), which preserves r4-r11 but also assumes r3 is an argument - that's
>> again another slightly different convention.
>
> ... for which the above implementations of __call_smc() should work too.
>
>> All in all the needs of the various firmwares might end up being just
>> different enough that we need to have a different backend for each of
>> them. The firmware_ops defined in arch/arm/include/asm/firmware.h
>> perform the abstraction at a higher level, which seems more fit here
>> IMHO.
>>
>> Alex.
>
> Indeed. If you think you could work with one of the above generics, we
> could try it and see what it looks like though.
>
> If it's an awkward fit, I might be being too optimistic.

I agree that your versions would most likely work in our (and probably
many others) case. But I wonder if individual platforms will not
prefer to sacrifice the ease of use of a generic version for the
ability to write faster code that will just preserve what is needed
(whether that performance gain is justified or not is of course
subject to debate). I don't have enough hindsight to decide which
approach is the best, but until we have more examples of firmwares
that would justify such a factorization, I think I'd like to go with
our own version first - for there is already more than enough to fix
in this patch. :)

Thanks,
Alex.

2013-06-10 08:11:39

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <[email protected]> wrote:
>>> I think we need to separate the concept of support for *a* secure
>>> monitor, from support for a *particular* secure monitor.
>>
>> Agreed. In this case, can we assume that support for a specific secure
>> monitor is not arch-specific, and that this patch should be moved
>> outside of arch-tegra and down to arch/arm? In other words, the ABI of
>> a particular secure monitor should be the same no matter the chip,
>> shouldn't it?
>
> I would like to believe that the Trusted Foundations monitor had the
> same ABI irrespective of which Soc it was running on. However, I have
> absolutely no idea at all if that's true. Even if there's some common
> subset of the ABI that is identical across all SoCs, I wouldn't be too
> surprised if there were custom extensions for each different SoC, or
> just perhaps even each product.
>
> Can you research this and find out the answer?

Will do. Information about TF is scarce unfortunately.

> What we can always do is make a compatible property that lists
> everything[1], and have the driver match on the most specific value for
> now, but relax the driver's matching later if it turns out that the ABI
> is indeed common.
>
> [1] That'd need to be at least secure OS name, and secure OS version.
> Perhaps the SoC and board data can be deduced from the DT's top-level
> compatible properties; nvidia,tegra114-shield, nvidia,tegra114?

They can probably, but in theory nothing prevents a board from coming
with different secure monitors (or none at all). In this case, just
having the board name might not be enough.

Having a proper node for the firmware like David and Tomasz suggested
seems to be the best way to make sure we cover all cases - I think I
will try to do it this way for the next version, and hopefully come
with a binding that is useful for everyone.

Thanks,
Alex.

2013-06-10 09:10:38

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Mon, Jun 10, 2013 at 04:47:22PM +0900, Alexandre Courbot wrote:
> One could remove the naked attribute and put there registers into the
> clobber list, but then the function will be inlined and we will have
> to ensure the parameters end up in the right register (and having a
> function that cannot be inlined is convenient in that respect). So as
> far as I can tell, having the function naked and saving the registers
> ourselves seems to be the most convenient way around this.

If you use such a large clobber list, you risk the compiler barfing on
you that it's run out of registers.

2013-06-10 09:15:37

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

2013-06-10 11:16:48

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Mon, Jun 10, 2013 at 05:11:15PM +0900, Alexandre Courbot wrote:
> On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <[email protected]> wrote:
> >>> I think we need to separate the concept of support for *a* secure
> >>> monitor, from support for a *particular* secure monitor.
> >>
> >> Agreed. In this case, can we assume that support for a specific secure
> >> monitor is not arch-specific, and that this patch should be moved
> >> outside of arch-tegra and down to arch/arm? In other words, the ABI of
> >> a particular secure monitor should be the same no matter the chip,
> >> shouldn't it?
> >
> > I would like to believe that the Trusted Foundations monitor had the
> > same ABI irrespective of which Soc it was running on. However, I have
> > absolutely no idea at all if that's true. Even if there's some common
> > subset of the ABI that is identical across all SoCs, I wouldn't be too
> > surprised if there were custom extensions for each different SoC, or
> > just perhaps even each product.
> >
> > Can you research this and find out the answer?
>
> Will do. Information about TF is scarce unfortunately.

I don't have full information on this topic, but there is certainly no
common standard ABI. Every example I've seen is different so far,
though some will be less different than others.

ARM are baking some proposabls for that, but that doesn't change the
existing software, and it's impossible to predict how rapidly a new
standards proposal would be adopted. So unfortunately, diversity is
something we will have to cope with for the foreseeable future.

> > What we can always do is make a compatible property that lists
> > everything[1], and have the driver match on the most specific value for
> > now, but relax the driver's matching later if it turns out that the ABI
> > is indeed common.
> >
> > [1] That'd need to be at least secure OS name, and secure OS version.
> > Perhaps the SoC and board data can be deduced from the DT's top-level
> > compatible properties; nvidia,tegra114-shield, nvidia,tegra114?
>
> They can probably, but in theory nothing prevents a board from coming
> with different secure monitors (or none at all). In this case, just
> having the board name might not be enough.
>
> Having a proper node for the firmware like David and Tomasz suggested
> seems to be the best way to make sure we cover all cases - I think I
> will try to do it this way for the next version, and hopefully come
> with a binding that is useful for everyone.

Since existing SMC based firmwares are not safely probeable, describing
what's there using DT feels like the best bet for now.

Cheers
---Dave

2013-06-10 11:21:25

[permalink] [raw]

Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Mon, Jun 10, 2013 at 05:05:04PM +0900, Alexandre Courbot wrote:
> On Sat, Jun 8, 2013 at 3:13 AM, Dave Martin <[email protected]> wrote:
> > One way to make the backend generic would be to have something like
> > one of the following (some syntax omitted due to laziness):
> >
> > u32 __naked __call_smc(u32 r0, ...)
> > {
> > asm volatile (
> > stmfd sp!, {r4-r11,lr}
> > smc #0
> > ldmfd sp!, {r4-r11,pc}
> > ::: "memory"
> > );
> > }
> >
> > /* The above works for up to 4 u32 arguments */
> >
> > u32 __naked __call_smc(u32 r0, ...)
> > {
> > asm volatile (
> > mov ip, sp
> > stmfd sp!, {r4-r11,lr}
> > ldmia ip, {r4-r11}
> > smc #0
> > ldmfd sp!, {r4-r11,pc}
> > ::: "memory"
> > );
> > }
> >
> > /*
> > * Works for up to 13 u32 arguments, provided the stack is deep
> > * enough to provide suitable garbage data to fill the registers
> > * if the caller supplied fewer arguments (a bit of a hack)
> > */
> >
> > u32 __naked __call_smc(struct pt_regs *regs) {
> >
> > asm(
> > stmfd sp!, {r4-r11,lr}
> > /* load regs from <regs> */
> > smc #0
> > /* save regs back to <regs> */
> > ldmfd sp!, {r4-r11,pc}
> > );
> > }
> >
> > /*
> > * Most generic, least-efficient version.
> > * Can return up to 13 u32 results instead of just one.
> > * For convenience, the r0 value returned by the SMC could be
> > * left in r0 so that it also determines the return value of the
> > * function.
> > *
> > * Most of the time, SMC shouldn't be called on any hot path,
> > * otherwise the performance battle is already lost -- so it may
> > * not be crucial to reach the maximum possible efficiency for
> > * these calls.
> > */
> >
> >
> > A particular firmware's Linux glue code might have to put extra stuff
> > around calls to generic_smc, but at least generic_smc itself wouldn't
> > need to be reinvented, and the firmware-specific glue code could usually
> > avoid asm.
> >
> >> Another example is the function that Tomasz shown
> >> (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
> >> ), which preserves r4-r11 but also assumes r3 is an argument - that's
> >> again another slightly different convention.
> >
> > ... for which the above implementations of __call_smc() should work too.
> >
> >> All in all the needs of the various firmwares might end up being just
> >> different enough that we need to have a different backend for each of
> >> them. The firmware_ops defined in arch/arm/include/asm/firmware.h
> >> perform the abstraction at a higher level, which seems more fit here
> >> IMHO.
> >>
> >> Alex.
> >
> > Indeed. If you think you could work with one of the above generics, we
> > could try it and see what it looks like though.
> >
> > If it's an awkward fit, I might be being too optimistic.
>
> I agree that your versions would most likely work in our (and probably
> many others) case. But I wonder if individual platforms will not
> prefer to sacrifice the ease of use of a generic version for the
> ability to write faster code that will just preserve what is needed
> (whether that performance gain is justified or not is of course
> subject to debate). I don't have enough hindsight to decide which
> approach is the best, but until we have more examples of firmwares
> that would justify such a factorization, I think I'd like to go with
> our own version first - for there is already more than enough to fix
> in this patch. :)

Sure, I'll have another think based on your repost.

Cheers
---Dave

2013-06-10 16:36:04