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

[Dave Martin <dave.martin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

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

(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