[RFC PATCH] ARM: Make a compile trustzone conditionally

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Kyungmin Park]

From: Kyungmin Park <kyungmin.park@samsung.com>

Some boards can use the trustzone but others can't use trustzone.
However we need to build it simultaneously. To address this issue,
introduce arm trustzone support and use it at each board files.

e.g., In boot_secondary at mach-exynos/platsmp.c

	__raw_writel(virt_to_phys(exynos4_secondary_startup),
		CPU1_BOOT_REG);

	if (trustzone_enabled()) {
		/* Call Exynos specific smc call */
		/* exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); */
	}

	gic_raise_softirq(cpumask_of(cpu), 1);

	if (pen_release == -1)

Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
index 283fa1d..16b7047 100644
--- a/arch/arm/common/Kconfig
+++ b/arch/arm/common/Kconfig
@@ -21,6 +21,9 @@ config ARM_VIC_NR
 	  The maximum number of VICs available in the system, for
 	  power management.
 
+config ARM_TRUSTZONE
+	bool
+
 config ICST
 	bool
 
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index e8a4e58..5820e49 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -4,6 +4,7 @@
 
 obj-$(CONFIG_ARM_GIC)		+= gic.o
 obj-$(CONFIG_ARM_VIC)		+= vic.o
+obj-$(CONFIG_ARM_TRUSTZONE)	+= trustzone.o
 obj-$(CONFIG_ICST)		+= icst.o
 obj-$(CONFIG_SA1111)		+= sa1111.o
 obj-$(CONFIG_PCI_HOST_VIA82C505) += via82c505.o
diff --git a/arch/arm/common/trustzone.c b/arch/arm/common/trustzone.c
new file mode 100644
index 0000000..5104a1f
--- /dev/null
+++ b/arch/arm/common/trustzone.c
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2012 Samsung Electronics.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+
+#include <asm/cache.h>
+
+static bool trustzone_enable __read_mostly = false;
+
+bool trustzone_enabled(void)
+{
+	return trustzone_enable;
+}
+
+void trustzone_set_enable(void)
+{
+	trustzone_enable = true;
+}
diff --git a/arch/arm/include/asm/trustzone.h b/arch/arm/include/asm/trustzone.h
new file mode 100644
index 0000000..126dd22
--- /dev/null
+++ b/arch/arm/include/asm/trustzone.h
@@ -0,0 +1,20 @@
+/*
+ *  Copyright (C) 2012 Samsung Electronics.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ASM_ARM_TRUSTZONE_H
+#define __ASM_ARM_TRUSTZONE_H
+
+#ifdef CONFIG_ARM_TRUSTZONE
+extern bool trustzone_enabled(void);
+extern void trustzone_set_enable(void);
+#else
+#define trustzone_enabled()		(0)
+#define trustzone_set_enable()		do { } while (0)
+#endif
+
+#endif

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Kyungmin Park]

Hi,

Any comments?

On 6/11/12, Kyungmin Park <kmpark@infradead.org> wrote:
> From: Kyungmin Park <kyungmin.park@samsung.com>
>
> Some boards can use the trustzone but others can't use trustzone.
> However we need to build it simultaneously. To address this issue,
> introduce arm trustzone support and use it at each board files.
>
> e.g., In boot_secondary at mach-exynos/platsmp.c
>
> 	__raw_writel(virt_to_phys(exynos4_secondary_startup),
> 		CPU1_BOOT_REG);
>
> 	if (trustzone_enabled()) {
> 		/* Call Exynos specific smc call */
> 		/* exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); */
> 	}
>
> 	gic_raise_softirq(cpumask_of(cpu), 1);
>
> 	if (pen_release == -1)
>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
> index 283fa1d..16b7047 100644
> --- a/arch/arm/common/Kconfig
> +++ b/arch/arm/common/Kconfig
> @@ -21,6 +21,9 @@ config ARM_VIC_NR
>  	  The maximum number of VICs available in the system, for
>  	  power management.
>
> +config ARM_TRUSTZONE
> +	bool
> +
>  config ICST
>  	bool
>
> diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
> index e8a4e58..5820e49 100644
> --- a/arch/arm/common/Makefile
> +++ b/arch/arm/common/Makefile
> @@ -4,6 +4,7 @@
>
>  obj-$(CONFIG_ARM_GIC)		+= gic.o
>  obj-$(CONFIG_ARM_VIC)		+= vic.o
> +obj-$(CONFIG_ARM_TRUSTZONE)	+= trustzone.o
>  obj-$(CONFIG_ICST)		+= icst.o
>  obj-$(CONFIG_SA1111)		+= sa1111.o
>  obj-$(CONFIG_PCI_HOST_VIA82C505) += via82c505.o
> diff --git a/arch/arm/common/trustzone.c b/arch/arm/common/trustzone.c
> new file mode 100644
> index 0000000..5104a1f
> --- /dev/null
> +++ b/arch/arm/common/trustzone.c
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +
> +#include <asm/cache.h>
> +
> +static bool trustzone_enable __read_mostly = false;
> +
> +bool trustzone_enabled(void)
> +{
> +	return trustzone_enable;
> +}
> +
> +void trustzone_set_enable(void)
> +{
> +	trustzone_enable = true;
> +}
> diff --git a/arch/arm/include/asm/trustzone.h
> b/arch/arm/include/asm/trustzone.h
> new file mode 100644
> index 0000000..126dd22
> --- /dev/null
> +++ b/arch/arm/include/asm/trustzone.h
> @@ -0,0 +1,20 @@
> +/*
> + *  Copyright (C) 2012 Samsung Electronics.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ASM_ARM_TRUSTZONE_H
> +#define __ASM_ARM_TRUSTZONE_H
> +
> +#ifdef CONFIG_ARM_TRUSTZONE
> +extern bool trustzone_enabled(void);
> +extern void trustzone_set_enable(void);
> +#else
> +#define trustzone_enabled()		(0)
> +#define trustzone_set_enable()		do { } while (0)
> +#endif
> +
> +#endif
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Olof Johansson]

On Sun, Jun 10, 2012 at 10:15 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> From: Kyungmin Park <kyungmin.park@samsung.com>
>
> Some boards can use the trustzone but others can't use trustzone.
> However we need to build it simultaneously. To address this issue,
> introduce arm trustzone support and use it at each board files.
>
> e.g., In boot_secondary at mach-exynos/platsmp.c
>
> ? ? ? ?__raw_writel(virt_to_phys(exynos4_secondary_startup),
> ? ? ? ? ? ? ? ?CPU1_BOOT_REG);
>
> ? ? ? ?if (trustzone_enabled()) {
> ? ? ? ? ? ? ? ?/* Call Exynos specific smc call */
> ? ? ? ? ? ? ? ?/* exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); */
> ? ? ? ?}
>
> ? ? ? ?gic_raise_softirq(cpumask_of(cpu), 1);
>
> ? ? ? ?if (pen_release == -1)

I would prefer to see this handled by using the smp_ops patchset that
Marc Zyngier has been posting instead, replacing the boot_secondary
function pointer during early boot depending on whether trustzone is
enabled or not.


-Olof

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Kyungmin Park]

Hi Olof,

On 6/18/12, Olof Johansson <olof@lixom.net> wrote:
> On Sun, Jun 10, 2012 at 10:15 PM, Kyungmin Park <kmpark@infradead.org>
> wrote:
>> From: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> Some boards can use the trustzone but others can't use trustzone.
>> However we need to build it simultaneously. To address this issue,
>> introduce arm trustzone support and use it at each board files.
>>
>> e.g., In boot_secondary at mach-exynos/platsmp.c
>>
>>        __raw_writel(virt_to_phys(exynos4_secondary_startup),
>>                CPU1_BOOT_REG);
>>
>>        if (trustzone_enabled()) {
>>                /* Call Exynos specific smc call */
>>                /* exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); */
>>        }
>>
>>        gic_raise_softirq(cpumask_of(cpu), 1);
>>
>>        if (pen_release == -1)
>
> I would prefer to see this handled by using the smp_ops patchset that
> Marc Zyngier has been posting instead, replacing the boot_secondary
> function pointer during early boot depending on whether trustzone is
> enabled or not.
I understand your request, but it's not only boot_secondary but also
exynos pm common code and so on.even more it's used at devfreq to
adjust the RAM bus timings.

Now these files are covered.

arch/arm/mach-exynos/cpu-exynos4.c
arch/arm/mach-exynos/cpu-exynos5.c
arch/arm/mach-exynos/cpuidle-exynos4.c
arch/arm/mach-exynos/cpuidle-exynos5.c
arch/arm/mach-exynos/idle-exynos4.S
arch/arm/mach-exynos/idle-exynos5.S
arch/arm/mach-exynos/include/mach/entry-macro.S
arch/arm/mach-exynos/platsmp.c
arch/arm/mach-exynos/pm-exynos4.c
arch/arm/mach-exynos/pm-exynos5.c
arch/arm/mach-exynos/sleep-exynos4.S
arch/arm/mach-exynos/sleep-exynos5.S
arch/arm/mach-exynos/tmu.c

How do you think?

Thank you,
Kyungmin Park

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Arnd Bergmann]

On Monday 18 June 2012, Kyungmin Park wrote:
> > I would prefer to see this handled by using the smp_ops patchset that
> > Marc Zyngier has been posting instead, replacing the boot_secondary
> > function pointer during early boot depending on whether trustzone is
> > enabled or not.
> I understand your request, but it's not only boot_secondary but also
> exynos pm common code and so on.even more it's used at devfreq to
> adjust the RAM bus timings.
> 
> Now these files are covered.
> 

Would it help to have a trustzone_ops structure with pointers to
functions if needed, similar to but separate from smp_ops?

Instead of checking for trustzone_enabled() in each place where
we call into smc, we can have a generic implementation that
we call for the disabled case, and provide a vendor specific
version of that struct with functions that call into smp 
where necessary.

	Arnd

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Kyungmin Park]

On 6/18/12, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 18 June 2012, Kyungmin Park wrote:
>> > I would prefer to see this handled by using the smp_ops patchset that
>> > Marc Zyngier has been posting instead, replacing the boot_secondary
>> > function pointer during early boot depending on whether trustzone is
>> > enabled or not.
>> I understand your request, but it's not only boot_secondary but also
>> exynos pm common code and so on.even more it's used at devfreq to
>> adjust the RAM bus timings.
>>
>> Now these files are covered.
>>
>
> Would it help to have a trustzone_ops structure with pointers to
> functions if needed, similar to but separate from smp_ops?
Here's real usages. I'm not sure it's possible since smc call is
vendor specific.

static int exynos4_cpu_suspend(unsigned long arg)
{
        outer_flush_all();

        /* issue the standby signal into the pm unit. */
        if (trustzone_enabled())
                exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
        else
                cpu_do_idle();

        /* we should never get past here */
        panic("sleep resumed to originator?");
}

static int exynos4_pm_add(struct device *dev, struct subsys_interface *sif)
{
        pm_cpu_prep = exynos4_pm_prepare;
        pm_cpu_sleep = exynos4_cpu_suspend;

        return 0;
}

static struct subsys_interface exynos4_pm_interface = {
        .name           = "exynos4_pm",
        .subsys         = &exynos4_subsys,
        .add_dev        = exynos4_pm_add,
};

static __init int exynos4_pm_drvinit(void)
{
        ..snip..
        return subsys_interface_register(&exynos4_pm_interface);
}
arch_initcall(exynos4_pm_drvinit);

In this case, it should be sepated two functions.

static int exynos4_cpu_suspend(unsigned long arg)
{
        outer_flush_all();

        /* issue the standby signal into the pm unit. */
        cpu_do_idle();

        /* we should never get past here */
        panic("sleep resumed to originator?");
}

static int exynos4_cpu_smc_suspend(unsigned long arg)
{
        outer_flush_all();

        exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);

        /* we should never get past here */
        panic("sleep resumed to originator?");
}

but still we should check it's trustzone is enabled or not to assign
proper function into pm_cpu_suspend

I think it's different from smp_ops.

In case of resume it's similar situcation.

static void exynos4_pm_resume(void)
{
        ... snip ...
        if (trustzone_enabled()) {
                exynos_smc(SMC_CMD_C15RESUME, save_arm_register[0],
                           save_arm_register[1], 0);
        } else {
                /* Restore Power control register */
                tmp = save_arm_register[0];
                asm volatile ("mcr p15, 0, %0, c15, c0, 0"
                              : : "r" (tmp)
                              : "cc");

                /* Restore Diagnostic register */
                tmp = save_arm_register[1];
                asm volatile ("mcr p15, 0, %0, c15, c0, 1"
                              : : "r" (tmp)
                              : "cc");
        }
        ... snip ...
}

static struct syscore_ops exynos4_pm_syscore_ops = {
        .suspend        = exynos4_pm_suspend,
        .resume         = exynos4_pm_resume,
};

>
> Instead of checking for trustzone_enabled() in each place where
> we call into smc, we can have a generic implementation that
> we call for the disabled case, and provide a vendor specific
> version of that struct with functions that call into smp
> where necessary.
Okay I'll think it more to make it generic. BTW omap uses smc call at
l2x0 codes.

Thank you,
Kyungmin Park
>
> 	Arnd
>

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Arnd Bergmann]

On Tuesday 19 June 2012, Kyungmin Park wrote:
> > Would it help to have a trustzone_ops structure with pointers to
> > functions if needed, similar to but separate from smp_ops?
> Here's real usages. I'm not sure it's possible since smc call is
> vendor specific.

I would hope that there is at last some overlap, as well as only a
limited number of things that you might want to do with smc.

> static int exynos4_cpu_suspend(unsigned long arg)
> {
>         outer_flush_all();
> 
>         /* issue the standby signal into the pm unit. */
>         if (trustzone_enabled())
>                 exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
>         else
>                 cpu_do_idle();
> 
>         /* we should never get past here */
>         panic("sleep resumed to originator?");
> }

This looks straightforward to implement as an indirect call just for
cpu_do_idle. We already have an indirection layer for cpu-specific
do_idle functions. It would be ideal to have only one level of
indirection, but the extra level would work as well.

> static int exynos4_cpu_suspend(unsigned long arg)
> {
>         outer_flush_all();
> 
>         /* issue the standby signal into the pm unit. */
>         cpu_do_idle();
> 
>         /* we should never get past here */
>         panic("sleep resumed to originator?");
> }
> 
> static int exynos4_cpu_smc_suspend(unsigned long arg)
> {
>         outer_flush_all();
> 
>         exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
> 
>         /* we should never get past here */
>         panic("sleep resumed to originator?");
> }
> 
> but still we should check it's trustzone is enabled or not to assign
> proper function into pm_cpu_suspend
> 
> I think it's different from smp_ops.

This is a different method from what I had in mind, but it would
work too. It's not platform independent though.

What I was thinking of is something along the lines of

static void nosmc_cpu_do_idle(void)
{
	cpu_do_idle();
}

struct smc_ops {
	void (*do_idle)(void);
	...
};

struct smc_ops default_smc_ops = {
	.do_idle = nosmc_cpu_do_idle,
	...
};

So the exynos4_cpu_suspend() function would just do an indirect call to
smc->do_idle(), which is either nosmc_cpu_do_idle or a function specific
to the smc firmware.

	Arnd

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Dave Martin]

On Wed, Jun 20, 2012 at 11:14:16AM +0000, Arnd Bergmann wrote:
> On Tuesday 19 June 2012, Kyungmin Park wrote:
> > > Would it help to have a trustzone_ops structure with pointers to
> > > functions if needed, similar to but separate from smp_ops?
> > Here's real usages. I'm not sure it's possible since smc call is
> > vendor specific.
> 
> I would hope that there is at last some overlap, as well as only a
> limited number of things that you might want to do with smc.
> 
> > static int exynos4_cpu_suspend(unsigned long arg)
> > {
> >         outer_flush_all();
> > 
> >         /* issue the standby signal into the pm unit. */
> >         if (trustzone_enabled())

For purely cosmetic reasons, I don't think we should use a name like
"trustzone_enabled" -- actually we sort of have the opposite: the
trustzone architectural features are blocked from Linux by the
presence of Secure World firmware in this case.

If we need a function at all, a name like secure_firmware_present()
might be better.  But I agree that it's probably better to just have
some alternative structs of function pointers, and install one at
boot time depending on whether the firmware is there or not.


> >                 exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
> >         else
> >                 cpu_do_idle();
> > 
> >         /* we should never get past here */
> >         panic("sleep resumed to originator?");
> > }
> 
> This looks straightforward to implement as an indirect call just for
> cpu_do_idle. We already have an indirection layer for cpu-specific
> do_idle functions. It would be ideal to have only one level of
> indirection, but the extra level would work as well.
> 
> > static int exynos4_cpu_suspend(unsigned long arg)
> > {
> >         outer_flush_all();
> > 
> >         /* issue the standby signal into the pm unit. */
> >         cpu_do_idle();
> > 
> >         /* we should never get past here */
> >         panic("sleep resumed to originator?");
> > }
> > 
> > static int exynos4_cpu_smc_suspend(unsigned long arg)
> > {
> >         outer_flush_all();
> > 
> >         exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
> > 
> >         /* we should never get past here */
> >         panic("sleep resumed to originator?");
> > }
> > 
> > but still we should check it's trustzone is enabled or not to assign
> > proper function into pm_cpu_suspend
> > 
> > I think it's different from smp_ops.
> 
> This is a different method from what I had in mind, but it would
> work too. It's not platform independent though.
> 
> What I was thinking of is something along the lines of
> 
> static void nosmc_cpu_do_idle(void)
> {
> 	cpu_do_idle();
> }
> 
> struct smc_ops {
> 	void (*do_idle)(void);
> 	...
> };
> 
> struct smc_ops default_smc_ops = {
> 	.do_idle = nosmc_cpu_do_idle,
> 	...
> };

This would my preferred model.

>From my point of view, the name "smc ops" is a bit bogus.  Really these are
platform-specific hooks which may need to be handled by firmware depending
on the configuration, and the fact that they may be implemented by calling
SMC is more of an implementation detail of _one_ of the alternative
implementations of those hooks.

So your struct might instead be called struct exynos4_firmware_ops, and
your two instances might be called exynos4_native_firmware_ops and
exynos4_smc_firmware_ops, or similar.  This is purely cosmetic, though.

As discussed in previous mails, the best way to choose which struct
exynos4_firmware_ops to use may be to scan for something in the device
tree, rather than using some scary probing hack.


If we have standard DT bindings for this stuff, we could have generic DT
support code for selecting the correct exynos4_firmware_ops structure
based on the DT.  Only the definition and contents of the platform-
specific firmware_ops structs would need to vary.

Whether we could end up with a common firmware_ops structure across all
platforms is less certain (?)

Cheers
---Dave

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Arnd Bergmann]

On Wednesday 20 June 2012, Dave Martin wrote:

> From my point of view, the name "smc ops" is a bit bogus.  Really these are
> platform-specific hooks which may need to be handled by firmware depending
> on the configuration, and the fact that they may be implemented by calling
> SMC is more of an implementation detail of one of the alternative
> implementations of those hooks.
> 
> So your struct might instead be called struct exynos4_firmware_ops, and
> your two instances might be called exynos4_native_firmware_ops and
> exynos4_smc_firmware_ops, or similar.  This is purely cosmetic, though.
> 
> As discussed in previous mails, the best way to choose which struct
> exynos4_firmware_ops to use may be to scan for something in the device
> tree, rather than using some scary probing hack.

Naming it firmware_ops rather than smc_ops sounds reasonable.

> If we have standard DT bindings for this stuff, we could have generic DT
> support code for selecting the correct exynos4_firmware_ops structure
> based on the DT.  Only the definition and contents of the platform-
> specific firmware_ops structs would need to vary.
> 
> Whether we could end up with a common firmware_ops structure across all
> platforms is less certain (?)

I would prefer to have a common one. Ideally I'd also like to see a
standardized firmware binary interface so that we can actually
have a generic instance of this structure for all platforms that
follow the standard and let the nonstandard ones provide their own
instance of this struct. We might need more than one standard though,
e.g. one for direct SMC calls and another one for ACPI in the future.

If we want to do something local to each platform, I would not even
call it firmware_ops, but just do whatever that platform needs.
In case of exynos, having two implementations of the pm_interface
as Kyungmin suggested sounds sufficient there, and we can do something
else for the next part of the platform that needs it.

If we want to have a more generic way to do it, I'd go all the way
and make it a platform-independent structure.

	Arnd

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Stephen Boyd]

On 06/18/12 07:10, Arnd Bergmann wrote:
> On Monday 18 June 2012, Kyungmin Park wrote:
>>> I would prefer to see this handled by using the smp_ops patchset that
>>> Marc Zyngier has been posting instead, replacing the boot_secondary
>>> function pointer during early boot depending on whether trustzone is
>>> enabled or not.
>> I understand your request, but it's not only boot_secondary but also
>> exynos pm common code and so on.even more it's used at devfreq to
>> adjust the RAM bus timings.
>>
>> Now these files are covered.
>>
> Would it help to have a trustzone_ops structure with pointers to
> functions if needed, similar to but separate from smp_ops?
>
> Instead of checking for trustzone_enabled() in each place where
> we call into smc, we can have a generic implementation that
> we call for the disabled case, and provide a vendor specific
> version of that struct with functions that call into smp 
> where necessary.
>
>

What if we tried to read the SCR.NS bit to determine if we're running in
secure state or not? It looks like reading SCR is UNDEFINED (i.e. causes
an undefined instruction exception) if we're running in the non-secure
state so I propose we set up an undef hook that traps the SCR access and
lies about the value of the NS bit to indicate we're non-secure.
Basically this:

static int scr_trap(struct pt_regs *regs, unsigned int instr)
{
        int reg = (instr >> 12) & 15;
        if (reg == 15)
                return 1;
        regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
        regs->ARM_pc += 4;
        return 0;
}

static struct undef_hook scr_hook = {
        .instr_mask     = 0x0fff0fff,
        .instr_val      = 0x0e110f11,
        .fn             = scr_trap,
};

int in_secure_state(void)
{
        unsigned int scr;

	register_undef_hook(&scr_hook);

        asm volatile(
        "       mrc p15, 0, %0, c1, c1, 0\n"
        : "=r" (scr)
        :
        : "cc");

	unregister_undef_hook(&scr_hook);

        return !(scr & BIT(0));
}
EXPORT_SYMBOL(in_secure_state);


It seems to mostly work, although I haven't figured out what you do
about the hypervisor case when the hypervisor has disabled the smc
instruction entirely (SCR.SCD=1). At that point I throw up my hands.
Maybe Will has some idea.

At the least I would hope we read the cpuid registers to see if the
processor supports the SMC instruction so that we can't say
trustzone_set_enable() on a CPU that doesn't even support it.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC PATCH] ARM: Make a compile trustzone conditionally

[Will Deacon]

Hi Stephen,

On Wed, Jun 20, 2012 at 02:22:14AM +0100, Stephen Boyd wrote:
> On 06/18/12 07:10, Arnd Bergmann wrote:
> > Instead of checking for trustzone_enabled() in each place where
> > we call into smc, we can have a generic implementation that
> > we call for the disabled case, and provide a vendor specific
> > version of that struct with functions that call into smp 
> > where necessary.
> >
> 
> What if we tried to read the SCR.NS bit to determine if we're running in
> secure state or not? It looks like reading SCR is UNDEFINED (i.e. causes
> an undefined instruction exception) if we're running in the non-secure
> state so I propose we set up an undef hook that traps the SCR access and
> lies about the value of the NS bit to indicate we're non-secure.
> Basically this:

Well, I can't resist reviewing the code, but I don't think we should be
trying to detect this (see below).

> static int scr_trap(struct pt_regs *regs, unsigned int instr)
> {
>         int reg = (instr >> 12) & 15;
>         if (reg == 15)
>                 return 1;

Eek -- surely GCC won't allocate PC for the destination register?!

>         regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
>         regs->ARM_pc += 4;
>         return 0;
> }
> 
> static struct undef_hook scr_hook = {
>         .instr_mask     = 0x0fff0fff,
>         .instr_val      = 0x0e110f11,
>         .fn             = scr_trap,
> };
> 
> int in_secure_state(void)
> {
>         unsigned int scr;
> 
> 	register_undef_hook(&scr_hook);
> 
>         asm volatile(
>         "       mrc p15, 0, %0, c1, c1, 0\n"
>         : "=r" (scr)
>         :
>         : "cc");

I don't think you need this clobber either.

> It seems to mostly work, although I haven't figured out what you do
> about the hypervisor case when the hypervisor has disabled the smc
> instruction entirely (SCR.SCD=1). At that point I throw up my hands.
> Maybe Will has some idea.

I think this is part of a bigger problem, which is about how we know where
we live in the privilege hierarchy and what we have sitting above us. We
have exactly the same issue with hypervisors and the hvc instruction.

Rather than try to probe the instruction (which by itself isn't enough,
since we can't guarantee that the exception will be handled by the upper
layers) I would personally prefer to see this described in the device tree.
We could have a simple property in the CPU node that says what the interface
looks like:

	smc-interface = "samsung, exynos";

or whatever you need to identify the interface uniquely. You could have a
corresponding entry for hvc-interface (and something like KVM could pass
its version to the guest for paravirualisation). If the property is missing,
then we take that to mean that the instruction shouldn't be executed on that
core -- it may be undefined or there may not be anything to pick up the
exception.

I realise I'm glossing over a lot of detail, but I prefer something along
these lines to the fragile probing code.

Cheers,

Will

Bindings for SMC/HVC firmware interfaces on ARM (Re: [RFC PATCH] ARM: Make a compile trustzone conditionally)

[Dave Martin]

On Wed, Jun 20, 2012 at 10:43:34AM +0100, Will Deacon wrote:
> Hi Stephen,
> 
> On Wed, Jun 20, 2012 at 02:22:14AM +0100, Stephen Boyd wrote:
> > On 06/18/12 07:10, Arnd Bergmann wrote:
> > > Instead of checking for trustzone_enabled() in each place where
> > > we call into smc, we can have a generic implementation that
> > > we call for the disabled case, and provide a vendor specific
> > > version of that struct with functions that call into smp 
> > > where necessary.
> > >
> > 
> > What if we tried to read the SCR.NS bit to determine if we're running in
> > secure state or not? It looks like reading SCR is UNDEFINED (i.e. causes
> > an undefined instruction exception) if we're running in the non-secure
> > state so I propose we set up an undef hook that traps the SCR access and
> > lies about the value of the NS bit to indicate we're non-secure.
> > Basically this:
> 
> Well, I can't resist reviewing the code, but I don't think we should be
> trying to detect this (see below).
> 
> > static int scr_trap(struct pt_regs *regs, unsigned int instr)
> > {
> >         int reg = (instr >> 12) & 15;
> >         if (reg == 15)
> >                 return 1;
> 
> Eek -- surely GCC won't allocate PC for the destination register?!
> 
> >         regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
> >         regs->ARM_pc += 4;
> >         return 0;
> > }
> > 
> > static struct undef_hook scr_hook = {
> >         .instr_mask     = 0x0fff0fff,
> >         .instr_val      = 0x0e110f11,
> >         .fn             = scr_trap,
> > };

(you'd also need to handle the Thumb case; I digress...)

> > 
> > int in_secure_state(void)
> > {
> >         unsigned int scr;
> > 
> > 	register_undef_hook(&scr_hook);
> > 
> >         asm volatile(
> >         "       mrc p15, 0, %0, c1, c1, 0\n"
> >         : "=r" (scr)
> >         :
> >         : "cc");
> 
> I don't think you need this clobber either.
> > It seems to mostly work, although I haven't figured out what you do
> > about the hypervisor case when the hypervisor has disabled the smc
> > instruction entirely (SCR.SCD=1). At that point I throw up my hands.
> > Maybe Will has some idea.
> 
> I think this is part of a bigger problem, which is about how we know where
> we live in the privilege hierarchy and what we have sitting above us. We
> have exactly the same issue with hypervisors and the hvc instruction.
> 
> Rather than try to probe the instruction (which by itself isn't enough,
> since we can't guarantee that the exception will be handled by the upper
> layers) I would personally prefer to see this described in the device tree.
> We could have a simple property in the CPU node that says what the interface
> looks like:
> 
> 	smc-interface = "samsung, exynos";
> 
> or whatever you need to identify the interface uniquely. You could have a
> corresponding entry for hvc-interface (and something like KVM could pass
> its version to the guest for paravirualisation). If the property is missing,
> then we take that to mean that the instruction shouldn't be executed on that
> core -- it may be undefined or there may not be anything to pick up the
> exception.

I concur.

The firmware is the only thing that really knows what's there,
and in reality there's no safe way to probe.  Even if we know we're in the
Normal World, we have no idea what's on the other side of SMC.  One
firmware's "probe" call may be another firmware's "halt and catch fire"
call.

So DT bindings permitting the firmware to tell us what's there make a lot
of sense.


Fleshing this out a bit...

Since the SMC and HVC instructions are features of the architecture, it may
make sense for the names to be:

	arm,smc = "vendor,interface-name";
	arm,hvc = "vendor,interface-name";

Some platforms may not fully service SMC on all CPUs, so there might be a
need to set this per CPU node.  This can be topologically determined, so
you might have:

	cluster at 0 {
		arm,smc = "vendor,platform-basic-firmware";
		
		cpu at 0 {
			arm,smc = "vendor,platform-fancy-firmware";
		};

		cpu at 1 {
			// no arm,smc property
		};

		cpu at 2 {
			// no arm,smc property
		};

		// ...
	}

The set of supported SMC calls on a CPU would then be determined by the
list of all strings on arm,smc properties on that CPU node and its
ancestors.

If the arm,smc or arm,hvc property is absent at all levels of the
topology, this does not mean that there is nothing there, but it does
mean that nothing is known about what's there.  Platform code would fall
back to the legacy case in that situation.


If additional properties are needed, like version or capability info,
we could have nodes instead:

	cluster at 0 {
		arm,smc {
			basic-firmware {
				compatible = "vendor,platform-basic-firmware";
			};
		}

		cpu at 0 {
			arm,smc {
				fancy-firmware {
					compatible = "vendor,platform-fancy-firmware";
					version = <1 12>;
					capabilities = "whizz", "bang", "kapow";
				};

				common-firmware {
					compatible = "standards-body,common-firmware";
					version = <1 0>;
				}
			};
		};

		// ...
	};

...or something equally esoteric.

(If node names like "arm,smc" are acceptable ... we could have something
else if not.)

It would be up to the platform's code handling "vendor,platform-fancy-
firmware" etc. to know about and deal with the precise calling
conventions for that firmware, though if there are generic firmware
interfaces someday, then those nodes' support code could be shared.

Cheers
---Dave

Bindings for SMC/HVC firmware interfaces on ARM (Re: [RFC PATCH] ARM: Make a compile trustzone conditionally)

[Rob Herring]

On 06/20/2012 05:51 AM, Dave Martin wrote:
> On Wed, Jun 20, 2012 at 10:43:34AM +0100, Will Deacon wrote:
>> Hi Stephen,
>>
>> On Wed, Jun 20, 2012 at 02:22:14AM +0100, Stephen Boyd wrote:
>>> On 06/18/12 07:10, Arnd Bergmann wrote:
>>>> Instead of checking for trustzone_enabled() in each place where
>>>> we call into smc, we can have a generic implementation that
>>>> we call for the disabled case, and provide a vendor specific
>>>> version of that struct with functions that call into smp 
>>>> where necessary.
>>>>
>>>
>>> What if we tried to read the SCR.NS bit to determine if we're running in
>>> secure state or not? It looks like reading SCR is UNDEFINED (i.e. causes
>>> an undefined instruction exception) if we're running in the non-secure
>>> state so I propose we set up an undef hook that traps the SCR access and
>>> lies about the value of the NS bit to indicate we're non-secure.
>>> Basically this:
>>
>> Well, I can't resist reviewing the code, but I don't think we should be
>> trying to detect this (see below).
>>
>>> static int scr_trap(struct pt_regs *regs, unsigned int instr)
>>> {
>>>         int reg = (instr >> 12) & 15;
>>>         if (reg == 15)
>>>                 return 1;
>>
>> Eek -- surely GCC won't allocate PC for the destination register?!
>>
>>>         regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
>>>         regs->ARM_pc += 4;
>>>         return 0;
>>> }
>>>
>>> static struct undef_hook scr_hook = {
>>>         .instr_mask     = 0x0fff0fff,
>>>         .instr_val      = 0x0e110f11,
>>>         .fn             = scr_trap,
>>> };
> 
> (you'd also need to handle the Thumb case; I digress...)
> 
>>>
>>> int in_secure_state(void)
>>> {
>>>         unsigned int scr;
>>>
>>> 	register_undef_hook(&scr_hook);
>>>
>>>         asm volatile(
>>>         "       mrc p15, 0, %0, c1, c1, 0\n"
>>>         : "=r" (scr)
>>>         :
>>>         : "cc");
>>
>> I don't think you need this clobber either.
>>> It seems to mostly work, although I haven't figured out what you do
>>> about the hypervisor case when the hypervisor has disabled the smc
>>> instruction entirely (SCR.SCD=1). At that point I throw up my hands.
>>> Maybe Will has some idea.

Perhaps there are other registers with fewer/different bits in
non-secure mode. IIRC, the A9 GIC has different secure and non-secure
enable bits in a mirrored register. This wouldn't be the best choice
given it is A9 specific and in the GIC.

>>
>> I think this is part of a bigger problem, which is about how we know where
>> we live in the privilege hierarchy and what we have sitting above us. We
>> have exactly the same issue with hypervisors and the hvc instruction.
>>
>> Rather than try to probe the instruction (which by itself isn't enough,
>> since we can't guarantee that the exception will be handled by the upper
>> layers) I would personally prefer to see this described in the device tree.
>> We could have a simple property in the CPU node that says what the interface
>> looks like:
>>
>> 	smc-interface = "samsung, exynos";

You already know you're on samsung,exynos, so you minimally only need to
know non-secure or secure. Presence of the property can indicate
non-secure mode and then the value can be something meaningful to that
platform like version.

>>
>> or whatever you need to identify the interface uniquely. You could have a
>> corresponding entry for hvc-interface (and something like KVM could pass
>> its version to the guest for paravirualisation). If the property is missing,
>> then we take that to mean that the instruction shouldn't be executed on that
>> core -- it may be undefined or there may not be anything to pick up the
>> exception.
> 
> I concur.
> 
> The firmware is the only thing that really knows what's there,
> and in reality there's no safe way to probe.  Even if we know we're in the
> Normal World, we have no idea what's on the other side of SMC.  One
> firmware's "probe" call may be another firmware's "halt and catch fire"
> call.

That reminds me. I need to go write the halt and catch fire call.

> 
> So DT bindings permitting the firmware to tell us what's there make a lot
> of sense.
> 
> 
> Fleshing this out a bit...
> 
> Since the SMC and HVC instructions are features of the architecture, it may
> make sense for the names to be:
> 
> 	arm,smc = "vendor,interface-name";
> 	arm,hvc = "vendor,interface-name";
> 
> Some platforms may not fully service SMC on all CPUs, so there might be a
> need to set this per CPU node.  This can be topologically determined, so
> you might have:

You would know this by knowing the type of firmware.

> 
> 	cluster at 0 {
> 		arm,smc = "vendor,platform-basic-firmware";
> 		
> 		cpu at 0 {
> 			arm,smc = "vendor,platform-fancy-firmware";
> 		};
> 
> 		cpu at 1 {
> 			// no arm,smc property
> 		};
> 
> 		cpu at 2 {
> 			// no arm,smc property
> 		};
> 
> 		// ...
> 	}
> 
> The set of supported SMC calls on a CPU would then be determined by the
> list of all strings on arm,smc properties on that CPU node and its
> ancestors.
> 
> If the arm,smc or arm,hvc property is absent at all levels of the
> topology, this does not mean that there is nothing there, but it does
> mean that nothing is known about what's there.  Platform code would fall
> back to the legacy case in that situation.
> 
> 
> If additional properties are needed, like version or capability info,
> we could have nodes instead:
> 
> 	cluster at 0 {
> 		arm,smc {
> 			basic-firmware {
> 				compatible = "vendor,platform-basic-firmware";
> 			};
> 		}
> 
> 		cpu at 0 {
> 			arm,smc {
> 				fancy-firmware {
> 					compatible = "vendor,platform-fancy-firmware";
> 					version = <1 12>;
> 					capabilities = "whizz", "bang", "kapow";
> 				};
> 
> 				common-firmware {
> 					compatible = "standards-body,common-firmware";
> 					version = <1 0>;
> 				}
> 			};
> 		};
> 
> 		// ...
> 	};
> 
> ...or something equally esoteric.
> 

This seems overly complex. While all of these conditions could happen,
what is reality?

I hope this smc mess is getting standardized for v8...

Rob

> (If node names like "arm,smc" are acceptable ... we could have something
> else if not.)
> 
> It would be up to the platform's code handling "vendor,platform-fancy-
> firmware" etc. to know about and deal with the precise calling
> conventions for that firmware, though if there are generic firmware
> interfaces someday, then those nodes' support code could be shared.
> 
> Cheers
> ---Dave
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Bindings for SMC/HVC firmware interfaces on ARM (Re: [RFC PATCH] ARM: Make a compile trustzone conditionally)

[Dave Martin]

On Wed, Jun 20, 2012 at 10:14:01AM -0500, Rob Herring wrote:
> On 06/20/2012 05:51 AM, Dave Martin wrote:
> > On Wed, Jun 20, 2012 at 10:43:34AM +0100, Will Deacon wrote:
> >> Hi Stephen,
> >>
> >> On Wed, Jun 20, 2012 at 02:22:14AM +0100, Stephen Boyd wrote:
> >>> On 06/18/12 07:10, Arnd Bergmann wrote:
> >>>> Instead of checking for trustzone_enabled() in each place where
> >>>> we call into smc, we can have a generic implementation that
> >>>> we call for the disabled case, and provide a vendor specific
> >>>> version of that struct with functions that call into smp 
> >>>> where necessary.
> >>>>
> >>>
> >>> What if we tried to read the SCR.NS bit to determine if we're running in
> >>> secure state or not? It looks like reading SCR is UNDEFINED (i.e. causes
> >>> an undefined instruction exception) if we're running in the non-secure
> >>> state so I propose we set up an undef hook that traps the SCR access and
> >>> lies about the value of the NS bit to indicate we're non-secure.
> >>> Basically this:
> >>
> >> Well, I can't resist reviewing the code, but I don't think we should be
> >> trying to detect this (see below).
> >>
> >>> static int scr_trap(struct pt_regs *regs, unsigned int instr)
> >>> {
> >>>         int reg = (instr >> 12) & 15;
> >>>         if (reg == 15)
> >>>                 return 1;
> >>
> >> Eek -- surely GCC won't allocate PC for the destination register?!
> >>
> >>>         regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
> >>>         regs->ARM_pc += 4;
> >>>         return 0;
> >>> }
> >>>
> >>> static struct undef_hook scr_hook = {
> >>>         .instr_mask     = 0x0fff0fff,
> >>>         .instr_val      = 0x0e110f11,
> >>>         .fn             = scr_trap,
> >>> };
> > 
> > (you'd also need to handle the Thumb case; I digress...)
> > 
> >>>
> >>> int in_secure_state(void)
> >>> {
> >>>         unsigned int scr;
> >>>
> >>> 	register_undef_hook(&scr_hook);
> >>>
> >>>         asm volatile(
> >>>         "       mrc p15, 0, %0, c1, c1, 0\n"
> >>>         : "=r" (scr)
> >>>         :
> >>>         : "cc");
> >>
> >> I don't think you need this clobber either.
> >>> It seems to mostly work, although I haven't figured out what you do
> >>> about the hypervisor case when the hypervisor has disabled the smc
> >>> instruction entirely (SCR.SCD=1). At that point I throw up my hands.
> >>> Maybe Will has some idea.
> 
> Perhaps there are other registers with fewer/different bits in
> non-secure mode. IIRC, the A9 GIC has different secure and non-secure
> enable bits in a mirrored register. This wouldn't be the best choice
> given it is A9 specific and in the GIC.
> 
> >>
> >> I think this is part of a bigger problem, which is about how we know where
> >> we live in the privilege hierarchy and what we have sitting above us. We
> >> have exactly the same issue with hypervisors and the hvc instruction.
> >>
> >> Rather than try to probe the instruction (which by itself isn't enough,
> >> since we can't guarantee that the exception will be handled by the upper
> >> layers) I would personally prefer to see this described in the device tree.
> >> We could have a simple property in the CPU node that says what the interface
> >> looks like:
> >>
> >> 	smc-interface = "samsung, exynos";
> 
> You already know you're on samsung,exynos, so you minimally only need to
> know non-secure or secure. Presence of the property can indicate
> non-secure mode and then the value can be something meaningful to that
> platform like version.

There is not necessarily just one possible firmware configuration per platform.
For example, I believe that in OMAP's "High Security" configuration, they
ship a lot more firmware functionality than in the basic configuration.
I believe it's still and same SoC.

So the choices are not "Secure" or "Non-Secure", but "what happens when
I execute SMC" (and similarly "HVC").  This is not a binary choice, even
for known platforms today like OMAP.

Also, assuming standardisation efforts ever succeed, there may be
generic firmware interface subsets present across many platforms, with
possible platform-specific extensions.

At the moment though, it does feel very premature to have any kind of
generic-ness here.  It would be nice to be able to handle in the the future,
but perhaps I'm too optimistic.

> 
> >>
> >> or whatever you need to identify the interface uniquely. You could have a
> >> corresponding entry for hvc-interface (and something like KVM could pass
> >> its version to the guest for paravirualisation). If the property is missing,
> >> then we take that to mean that the instruction shouldn't be executed on that
> >> core -- it may be undefined or there may not be anything to pick up the
> >> exception.
> > 
> > I concur.
> > 
> > The firmware is the only thing that really knows what's there,
> > and in reality there's no safe way to probe.  Even if we know we're in the
> > Normal World, we have no idea what's on the other side of SMC.  One
> > firmware's "probe" call may be another firmware's "halt and catch fire"
> > call.
> 
> That reminds me. I need to go write the halt and catch fire call.

(It's probably already there on most platforms ... intentionally or not ...)

> 
> > 
> > So DT bindings permitting the firmware to tell us what's there make a lot
> > of sense.
> > 
> > 
> > Fleshing this out a bit...
> > 
> > Since the SMC and HVC instructions are features of the architecture, it may
> > make sense for the names to be:
> > 
> > 	arm,smc = "vendor,interface-name";
> > 	arm,hvc = "vendor,interface-name";
> > 
> > Some platforms may not fully service SMC on all CPUs, so there might be a
> > need to set this per CPU node.  This can be topologically determined, so
> > you might have:
> 
> You would know this by knowing the type of firmware.

In principle yes, but what if a single type of firmware is deployable on
multiple topologies, or if this is firmware configuration dependent?

Maybe that can be addressed by the same fix as for handling multiple
firmware configurations -- i.e., the firmware choice is a string, not a
boolean, or we have some extra property somewhere, but global instead
of per-node.

I would strongly hope that any competent firmware would have an interface
for finding out things like topological limitations, once you've
determined that it's safe to call it.  The may be exceptions, though.

> > 
> > 	cluster at 0 {
> > 		arm,smc = "vendor,platform-basic-firmware";
> > 		
> > 		cpu at 0 {
> > 			arm,smc = "vendor,platform-fancy-firmware";
> > 		};
> > 
> > 		cpu at 1 {
> > 			// no arm,smc property
> > 		};
> > 
> > 		cpu at 2 {
> > 			// no arm,smc property
> > 		};
> > 
> > 		// ...
> > 	}
> > 
> > The set of supported SMC calls on a CPU would then be determined by the
> > list of all strings on arm,smc properties on that CPU node and its
> > ancestors.
> > 
> > If the arm,smc or arm,hvc property is absent at all levels of the
> > topology, this does not mean that there is nothing there, but it does
> > mean that nothing is known about what's there.  Platform code would fall
> > back to the legacy case in that situation.
> > 
> > 
> > If additional properties are needed, like version or capability info,
> > we could have nodes instead:
> > 
> > 	cluster at 0 {
> > 		arm,smc {
> > 			basic-firmware {
> > 				compatible = "vendor,platform-basic-firmware";
> > 			};
> > 		}
> > 
> > 		cpu at 0 {
> > 			arm,smc {
> > 				fancy-firmware {
> > 					compatible = "vendor,platform-fancy-firmware";
> > 					version = <1 12>;
> > 					capabilities = "whizz", "bang", "kapow";
> > 				};
> > 
> > 				common-firmware {
> > 					compatible = "standards-body,common-firmware";
> > 					version = <1 0>;
> > 				}
> > 			};
> > 		};
> > 
> > 		// ...
> > 	};
> > 
> > ...or something equally esoteric.
> > 
> 
> This seems overly complex. While all of these conditions could happen,
> what is reality?

Well, I did say that was getting esoteric :)

And in truth, spraying extra nodes all over the DT just for this does
seem rather excessive.

> I hope this smc mess is getting standardized for v8...

Quite.

---Dave