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

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

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

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.

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.

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@0 {
		arm,smc = "vendor,platform-basic-firmware";
		
		cpu@0 {
			arm,smc = "vendor,platform-fancy-firmware";
		};

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

		cpu@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@0 {
		arm,smc {
			basic-firmware {
				compatible = "vendor,platform-basic-firmware";
			};
		}

		cpu@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-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss