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

[Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org>]

On Fri, Jun 07, 2013 at 06:03:54PM +0900, Alexandre Courbot wrote:
> On Fri, Jun 7, 2013 at 3:08 AM, Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org> 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