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

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

[Dave Martin]

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

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

[Rob Herring]

On 06/20/2012 05:51 AM, Dave Martin wrote:
> 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.

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.

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

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

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

[Dave Martin]

On Wed, Jun 20, 2012 at 10:14:01AM -0500, Rob Herring wrote:
> On 06/20/2012 05:51 AM, Dave Martin wrote:
> > 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.
> 
> 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.
> 
> >>
> >> 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.

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

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

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

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