ARM: Make a compile trustzone conditionally

From: Kyungmin Park <[email protected]>

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 <[email protected]>

---

arch/arm/common/Kconfig +3 -0

index 283fa1d..16b7047 100644
@@ -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
 

arch/arm/common/Makefile +1 -0

index e8a4e58..5820e49 100644
@@ -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

arch/arm/common/trustzone.c new file +23 -0

new file mode 100644
index 0000000..5104a1f
@@ -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;
+}

arch/arm/include/asm/trustzone.h new file +20 -0

new file mode 100644
index 0000000..126dd22
@@ -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

From: Kyungmin Park <[email protected]>

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)

Quoted 16 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>

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); */
       }

Quoted 3 lines

       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.

Quoted 4 lines

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.

Quoted 8 lines · Kyungmin Park

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 2 lines

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_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.

Quoted 4 lines · Kyungmin Park

Quoted 2 lines

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.

Quoted 6 lines

static int exynos4_cpu_suspend(unsigned long arg)

{
        outer_flush_all();

        /* issue the standby signal into the pm unit. */
        if (trustzone_enabled())

Quoted 7 lines

                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.

Quoted 21 lines

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?");
}

Quoted 4 lines

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,
	...
};

From my point of view, the name "smc ops" is a bit bogus. Really these are

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 (?)

Quoted 10 lines · Kyungmin Park

Quoted 4 lines

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.

Quoted 6 lines · Arnd Bergmann

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");

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.

Hi Stephen,

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Arnd Bergmann

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.

Quoted 6 lines

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).

Quoted 5 lines

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?!

Quoted 10 lines

        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,
};

Quoted 12 lines

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.

Quoted 4 lines

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.

Quoted 34 lines · Will Deacon

Hi Stephen,

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Arnd Bergmann

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.

Quoted 6 lines

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).

Quoted 5 lines

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?!

Quoted 5 lines

        regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
        regs->ARM_pc += 4;
        return 0;
}

Quoted 5 lines

Quoted 5 lines

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...)

Quoted 18 lines

Quoted 12 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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";

Quoted 7 lines

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
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 35 lines · Dave Martin

Quoted 34 lines · Will Deacon

Hi Stephen,

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Arnd Bergmann

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.

Quoted 6 lines

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).

Quoted 5 lines

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?!

Quoted 5 lines

        regs->uregs[reg] = BIT(0); /* Trapped = non-secure */
        regs->ARM_pc += 4;
        return 0;
}

Quoted 26 lines

Quoted 5 lines

Quoted 5 lines

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...)

Quoted 18 lines

Quoted 12 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

Quoted 11 lines

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:

Quoted 1 line

Quoted 1 line

	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.

Quoted 11 lines

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 11 lines

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:

Quoted 6 lines

	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.

Quoted 54 lines

	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>;
				}
			};
		};

Quoted 5 lines

		// ...
	};

...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...