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

On Wed, Jun 20, 2012 at 10:14:01AM -0500, Rob Herring wrote:

On 06/20/2012 05:51 AM, Dave Martin wrote:

quoted

On Wed, Jun 20, 2012 at 10:43:34AM +0100, Will Deacon wrote:

quoted

Hi Stephen,

On Wed, Jun 20, 2012 at 02:22:14AM +0100, Stephen Boyd wrote:

quoted

On 06/18/12 07:10, Arnd Bergmann wrote:

quoted

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

quoted

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

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

quoted

quoted

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

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

quoted

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.

quoted

quoted

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

quoted

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.

quoted

	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