[U-Boot,2/6] ARM: add assembly routine to switch to non-secure state

While actually switching to non-secure state is one thing, the
more important part of this process is to make sure that we still
have full access to the interrupt controller (GIC).
The GIC is fully aware of secure vs. non-secure state, some
registers are banked, others may be configured to be accessible from
secure state only.
To be as generic as possible, we get the GIC memory mapped address
based on the PERIPHBASE register. We check explicitly for
ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
cores we know the offsets for the GIC CPU interface from the
PERIPHBASE content. Other cores could be added as needed.

With the GIC accessible, we:
a) allow private interrupts to be delivered to the core
   (GICD_IGROUPR0 = 0xFFFFFFFF)
b) enable the CPU interface (GICC_CTLR[0] = 1)
c) set the priority filter to allow non-secure interrupts
   (GICC_PMR = 0x80)

After having switched to non-secure state, we also enable the
non-secure GIC CPU interface, since this register is banked.

Also we allow access to all coprocessor interfaces from non-secure
state by writing the appropriate bits in the NSACR register.

For reasons obvious later we only use caller saved registers r0-r3.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote:
> While actually switching to non-secure state is one thing, the
> more important part of this process is to make sure that we still
> have full access to the interrupt controller (GIC).
> The GIC is fully aware of secure vs. non-secure state, some
> registers are banked, others may be configured to be accessible from
> secure state only.
> To be as generic as possible, we get the GIC memory mapped address
> based on the PERIPHBASE register. We check explicitly for
> ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
> cores we know the offsets for the GIC CPU interface from the
> PERIPHBASE content. Other cores could be added as needed.
> 
> With the GIC accessible, we:
> a) allow private interrupts to be delivered to the core
>    (GICD_IGROUPR0 = 0xFFFFFFFF)
> b) enable the CPU interface (GICC_CTLR[0] = 1)
> c) set the priority filter to allow non-secure interrupts
>    (GICC_PMR = 0x80)
> 
> After having switched to non-secure state, we also enable the
> non-secure GIC CPU interface, since this register is banked.
> 
> Also we allow access to all coprocessor interfaces from non-secure
> state by writing the appropriate bits in the NSACR register.
> 
> For reasons obvious later we only use caller saved registers r0-r3.

You probably want to put that in a comment in the code, and it would
also be super helpful to explain the obvious part here, because most
readers don't look "forward in time" to understand this patch...

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index da48b36..e63e892 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -572,3 +572,50 @@ fiq:
>  
>  #endif /* CONFIG_USE_IRQ */
>  #endif /* CONFIG_SPL_BUILD */
> +
> +#ifdef CONFIG_ARMV7_VIRT
> +/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> + */
> +.globl _nonsec_gic_switch
> +_nonsec_gic_switch:
> +	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE

You should probably check if bits [7:0] == 0x00 here, otherwise you may
end up doing some very strange stuff - if any of those bits are set you
can just error out at this point, but it should be gracefully handled.

Also since it's core specific, you'd probably want to check that before
using it.

> +	add	r3, r2, #0x1000			@ GIC dist i/f offset

Since this is core specific, could the offset please go in an
appropriate header file?  It will also eliminate the need for the
comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)

> +	mvn	r1, #0
> +	str	r1, [r3, #0x80]			@ allow private interrupts

Aren't you makeing an assumption about the number of available
interrupts here?  You should read the ITLinesNumber field from the
GICD_TYPER if I'm not mistaking.

I also think it would be much cleaner if you again used a define for the
0x80 offset.

Also, don't you need to set some enable fields on the GICD_CTLR here to
enable group 1 interrupts?

> +
> +	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
> +	bfc	r0, #16, #8			@ mask out variant, arch
> +	bfc	r0, #0, #4			@ and revision
> +	movw	r1, #0xc070
> +	movt	r1, #0x4100
> +	cmp	r0, r1				@ check for Cortex-A7
> +	orr	r1, #0xf0

wow, nice bit fiddling.  It may be quite a bit easier to read if you
simply had defines for the bitmasks and real values and just did a load
and placed a literal section accordingly.

> +	cmpne	r0, r1				@ check for Cortex-A15
> +	movne	r1, #0x100			@ GIC CPU offset for A9

So I read the ARMV7_VIRT config flag as something you can only enable on
a board that actually supports the virtulization extensions, which the
A9 doesn't so this should probably error out instead (or do an endless
loop, or ignore the case in the code, ...).

> +	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7

Again, I think defines for these are appropriate.

> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> +
> +	mov	r1, #1
> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> +	mov	r1, #0x80

Why are you not setting this to #0xff ?

> +	str	r1, [r3, #4]			@ set GICC_PIMR[7]
> +

here it seems we're moving into non-gic related stuff in a function
called _nonsec_gic_switch

> +	movw	r1, #0x3fff
> +	movt	r1, #0x0006
> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> +
> +	ldr	r1, =_start
> +	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR

I thought you already took care of the MVBAR during init?
> +
> +	isb
> +	smc	#0				@ call into MONITOR mode
> +	isb					@ clobbers r0 and r1

This isb shouldn't be necessary if you just did an exception return?

> +
> +	mov	r1, #1
> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]

you're doing more than setting the enable bit: you're setting the EOImodeNS,
IRQBypDisGrp1, and FIQBypDisGrp1, as you should.

> +	add	r2, r2, #0x1000			@ GIC dist i/f offset
> +	str	r1, [r2]			@ allow private interrupts

It seems a bit brittle to rely on the smc handler to not clobber r2 and
r3, and it may an annoying thing to track down.  You could just
re-generate the the gic base address from the cp15 register.  Your
choice.

> +
> +	mov	pc, lr
> +#endif /* CONFIG_ARMV7_VIRT */
> -- 
> 1.7.12.1
>

On 05/31/2013 05:04 AM, Christoffer Dall wrote:
> On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote:
>> While actually switching to non-secure state is one thing, the
>> more important part of this process is to make sure that we still
>> have full access to the interrupt controller (GIC).
>> The GIC is fully aware of secure vs. non-secure state, some
>> registers are banked, others may be configured to be accessible from
>> secure state only.
>> To be as generic as possible, we get the GIC memory mapped address
>> based on the PERIPHBASE register. We check explicitly for
>> ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
>> cores we know the offsets for the GIC CPU interface from the
>> PERIPHBASE content. Other cores could be added as needed.
>>
>> With the GIC accessible, we:
>> a) allow private interrupts to be delivered to the core
>>     (GICD_IGROUPR0 = 0xFFFFFFFF)
>> b) enable the CPU interface (GICC_CTLR[0] = 1)
>> c) set the priority filter to allow non-secure interrupts
>>     (GICC_PMR = 0x80)
>>
>> After having switched to non-secure state, we also enable the
>> non-secure GIC CPU interface, since this register is banked.
>>
>> Also we allow access to all coprocessor interfaces from non-secure
>> state by writing the appropriate bits in the NSACR register.
>>
>> For reasons obvious later we only use caller saved registers r0-r3.
>
> You probably want to put that in a comment in the code, and it would
> also be super helpful to explain the obvious part here, because most
> readers don't look "forward in time" to understand this patch...

Agreed.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index da48b36..e63e892 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -572,3 +572,50 @@ fiq:
>>
>>   #endif /* CONFIG_USE_IRQ */
>>   #endif /* CONFIG_SPL_BUILD */
>> +
>> +#ifdef CONFIG_ARMV7_VIRT
>> +/* Routine to initialize GIC CPU interface and switch to nonsecure state.
>> + */
>> +.globl _nonsec_gic_switch
>> +_nonsec_gic_switch:
>> +	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
>
> You should probably check if bits [7:0] == 0x00 here, otherwise you may
> end up doing some very strange stuff - if any of those bits are set you
> can just error out at this point, but it should be gracefully handled.
>
> Also since it's core specific, you'd probably want to check that before
> using it.

As you found out later, I am doing this before calling this routine. But 
I will add a comment here to avoid confusion.

>> +	add	r3, r2, #0x1000			@ GIC dist i/f offset
>
> Since this is core specific, could the offset please go in an
> appropriate header file?  It will also eliminate the need for the
> comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)
>
>> +	mvn	r1, #0
>> +	str	r1, [r3, #0x80]			@ allow private interrupts
>
> Aren't you makeing an assumption about the number of available
> interrupts here?  You should read the ITLinesNumber field from the
> GICD_TYPER if I'm not mistaking.

This is the per core private interrupts. All bits should be implemented.
 From the GIC spec, chapter 4.3.4:
"In a multiprocessor implementation, GICD_IGROUPR0 is banked for each 
connected processor. This register holds the group status bits for 
interrupts 0-31."

> I also think it would be much cleaner if you again used a define for the
> 0x80 offset.
>
> Also, don't you need to set some enable fields on the GICD_CTLR here to
> enable group 1 interrupts?

Since this a non-banked per-system register, I do this later in the C part.

>> +
>> +	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
>> +	bfc	r0, #16, #8			@ mask out variant, arch
>> +	bfc	r0, #0, #4			@ and revision
>> +	movw	r1, #0xc070
>> +	movt	r1, #0x4100
>> +	cmp	r0, r1				@ check for Cortex-A7
>> +	orr	r1, #0xf0
>
> wow, nice bit fiddling.  It may be quite a bit easier to read if you
> simply had defines for the bitmasks and real values and just did a load
> and placed a literal section accordingly.

The sequence is necessary since we are short on registers. I agree it is 
a bit obfuscated, will make it more readable.

>> +	cmpne	r0, r1				@ check for Cortex-A15
>> +	movne	r1, #0x100			@ GIC CPU offset for A9
>
> So I read the ARMV7_VIRT config flag as something you can only enable on
> a board that actually supports the virtulization extensions, which the
> A9 doesn't so this should probably error out instead (or do an endless
> loop, or ignore the case in the code, ...).

Yeah, originally I had the idea to support a non-sec switch only on 
non-virt capable CPUs. My first version had a separate non-sec switch 
command. This here is kind of a leftover of that early version. But 
until patch 5/6 is applied this actually works (and we had a use-case 
internally here), so I decided to leave this in.

>> +	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
>
> Again, I think defines for these are appropriate.

Good point. Will fix this.

>> +	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
>> +
>> +	mov	r1, #1
>> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
>> +	mov	r1, #0x80
>
> Why are you not setting this to #0xff ?

Because a certain Christoffer Dall did this the same way in his Arndale 
specific patch ;-)
+       /* Set GIC priority mask bit [7] = 1 */
+       addr = EXYNOS5_GIC_CPU_BASE;
+       writel(0x80, addr + ARM_GICV2_ICCPMR);
(and I remember having read this recommendation somewhere)

>> +	str	r1, [r3, #4]			@ set GICC_PIMR[7]
>> +
>
> here it seems we're moving into non-gic related stuff in a function
> called _nonsec_gic_switch

Right, but this is per CPU and needs to be done in secure monitor mode, 
so it belongs here. Shall I rename the function to be called 
non_secure_init or the like?

>
>> +	movw	r1, #0x3fff
>> +	movt	r1, #0x0006
>> +	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
>> +
>> +	ldr	r1, =_start
>> +	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
>> +	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR
>
> I thought you already took care of the MVBAR during init?

Right, as mentioned earlier I have fixed this already.

>> +
>> +	isb
>> +	smc	#0				@ call into MONITOR mode
>> +	isb					@ clobbers r0 and r1
>
> This isb shouldn't be necessary if you just did an exception return?

Seems to be a paranoid leftover of one debug session...

>> +
>> +	mov	r1, #1
>> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
>
> you're doing more than setting the enable bit: you're setting the EOImodeNS,
> IRQBypDisGrp1, and FIQBypDisGrp1, as you should.

But 0x01 is the correct value? And the reset value is 0, right?
I will extend the comment.

>> +	add	r2, r2, #0x1000			@ GIC dist i/f offset
>> +	str	r1, [r2]			@ allow private interrupts
>
> It seems a bit brittle to rely on the smc handler to not clobber r2 and
> r3, and it may an annoying thing to track down.  You could just
> re-generate the the gic base address from the cp15 register.  Your
> choice.

So shall I put comments here and at the smc handler?

Thanks again for the comments!
Andre.

>> +
>> +	mov	pc, lr
>> +#endif /* CONFIG_ARMV7_VIRT */
>> --
>> 1.7.12.1
>>

On Fri, May 31, 2013 at 11:26:06AM +0200, Andre Przywara wrote:
> On 05/31/2013 05:04 AM, Christoffer Dall wrote:
> >On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote:
> >>While actually switching to non-secure state is one thing, the
> >>more important part of this process is to make sure that we still
> >>have full access to the interrupt controller (GIC).
> >>The GIC is fully aware of secure vs. non-secure state, some
> >>registers are banked, others may be configured to be accessible from
> >>secure state only.
> >>To be as generic as possible, we get the GIC memory mapped address
> >>based on the PERIPHBASE register. We check explicitly for
> >>ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those
> >>cores we know the offsets for the GIC CPU interface from the
> >>PERIPHBASE content. Other cores could be added as needed.
> >>
> >>With the GIC accessible, we:
> >>a) allow private interrupts to be delivered to the core
> >>    (GICD_IGROUPR0 = 0xFFFFFFFF)
> >>b) enable the CPU interface (GICC_CTLR[0] = 1)
> >>c) set the priority filter to allow non-secure interrupts
> >>    (GICC_PMR = 0x80)
> >>
> >>After having switched to non-secure state, we also enable the
> >>non-secure GIC CPU interface, since this register is banked.
> >>
> >>Also we allow access to all coprocessor interfaces from non-secure
> >>state by writing the appropriate bits in the NSACR register.
> >>
> >>For reasons obvious later we only use caller saved registers r0-r3.
> >
> >You probably want to put that in a comment in the code, and it would
> >also be super helpful to explain the obvious part here, because most
> >readers don't look "forward in time" to understand this patch...
> 
> Agreed.
> 
> >>
> >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >>---
> >>  arch/arm/cpu/armv7/start.S | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 47 insertions(+)
> >>
> >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >>index da48b36..e63e892 100644
> >>--- a/arch/arm/cpu/armv7/start.S
> >>+++ b/arch/arm/cpu/armv7/start.S
> >>@@ -572,3 +572,50 @@ fiq:
> >>
> >>  #endif /* CONFIG_USE_IRQ */
> >>  #endif /* CONFIG_SPL_BUILD */
> >>+
> >>+#ifdef CONFIG_ARMV7_VIRT
> >>+/* Routine to initialize GIC CPU interface and switch to nonsecure state.
> >>+ */
> >>+.globl _nonsec_gic_switch
> >>+_nonsec_gic_switch:
> >>+	mrc	p15, 4, r2, c15, c0, 0		@ r2 = PERIPHBASE
> >
> >You should probably check if bits [7:0] == 0x00 here, otherwise you may
> >end up doing some very strange stuff - if any of those bits are set you
> >can just error out at this point, but it should be gracefully handled.
> >
> >Also since it's core specific, you'd probably want to check that before
> >using it.
> 
> As you found out later, I am doing this before calling this routine.
> But I will add a comment here to avoid confusion.
> 

yeah, or just expect that this address is in r0 upon calling the
routine, then you're in the clear.

> >>+	add	r3, r2, #0x1000			@ GIC dist i/f offset
> >
> >Since this is core specific, could the offset please go in an
> >appropriate header file?  It will also eliminate the need for the
> >comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...)
> >
> >>+	mvn	r1, #0
> >>+	str	r1, [r3, #0x80]			@ allow private interrupts
> >
> >Aren't you makeing an assumption about the number of available
> >interrupts here?  You should read the ITLinesNumber field from the
> >GICD_TYPER if I'm not mistaking.
> 
> This is the per core private interrupts. All bits should be implemented.
> From the GIC spec, chapter 4.3.4:
> "In a multiprocessor implementation, GICD_IGROUPR0 is banked for
> each connected processor. This register holds the group status bits
> for interrupts 0-31."
> 

I understand it, but the comments or naming of the routine never
suggested that this was the code that was called per-core.  I really
think that is the core objective of this function: The NS-init that each
core must do, it's not really GIC specific, so I suggest you rename it.

> >I also think it would be much cleaner if you again used a define for the
> >0x80 offset.
> >
> >Also, don't you need to set some enable fields on the GICD_CTLR here to
> >enable group 1 interrupts?
> 
> Since this a non-banked per-system register, I do this later in the C part.
> 

later in the patch series, before in the flow of the code, right? :)

> >>+
> >>+	mrc	p15, 0, r0, c0, c0, 0		@ MIDR
> >>+	bfc	r0, #16, #8			@ mask out variant, arch
> >>+	bfc	r0, #0, #4			@ and revision
> >>+	movw	r1, #0xc070
> >>+	movt	r1, #0x4100
> >>+	cmp	r0, r1				@ check for Cortex-A7
> >>+	orr	r1, #0xf0
> >
> >wow, nice bit fiddling.  It may be quite a bit easier to read if you
> >simply had defines for the bitmasks and real values and just did a load
> >and placed a literal section accordingly.
> 
> The sequence is necessary since we are short on registers. I agree
> it is a bit obfuscated, will make it more readable.
> 

yeah but:

#define ARM_CORTEX_A15_ID	0x4100c070

.ltorg
[...]
ldr	r0, =ARM_CORTEX_A15_ID	

only uses a single register as well.


> >>+	cmpne	r0, r1				@ check for Cortex-A15
> >>+	movne	r1, #0x100			@ GIC CPU offset for A9
> >
> >So I read the ARMV7_VIRT config flag as something you can only enable on
> >a board that actually supports the virtulization extensions, which the
> >A9 doesn't so this should probably error out instead (or do an endless
> >loop, or ignore the case in the code, ...).
> 
> Yeah, originally I had the idea to support a non-sec switch only on
> non-virt capable CPUs. My first version had a separate non-sec
> switch command. This here is kind of a leftover of that early
> version. But until patch 5/6 is applied this actually works (and we
> had a use-case internally here), so I decided to leave this in.
> 

I think it's a good idea (as I said on one of the other patches) to
follow your initial approach, mainly because it's going to make the code
easier to read and understand, and I think you're going to pay only a
penalty for a few call in/out instructions in terms of code size.  But
then you need to introduce two different config options.

> >>+	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
> >
> >Again, I think defines for these are appropriate.
> 
> Good point. Will fix this.
> 
> >>+	add	r3, r2, r1			@ r3 = GIC CPU i/f addr
> >>+
> >>+	mov	r1, #1
> >>+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> >>+	mov	r1, #0x80
> >
> >Why are you not setting this to #0xff ?
> 
> Because a certain Christoffer Dall did this the same way in his
> Arndale specific patch ;-)

lol, got me.

> +       /* Set GIC priority mask bit [7] = 1 */
> +       addr = EXYNOS5_GIC_CPU_BASE;
> +       writel(0x80, addr + ARM_GICV2_ICCPMR);
> (and I remember having read this recommendation somewhere)
> 

I may have read the Arndale data sheet and seen that there was a max of
128 interrupts, but I really don't remember why - it was clear from the
get go that my patch was just a fast path from Arndale.

But as I read the GIC2 specs, if there are less than 256 interrupts,
those other bits will just be RAZ/WI.

> >>+	str	r1, [r3, #4]			@ set GICC_PIMR[7]
> >>+
> >
> >here it seems we're moving into non-gic related stuff in a function
> >called _nonsec_gic_switch
> 
> Right, but this is per CPU and needs to be done in secure monitor
> mode, so it belongs here. Shall I rename the function to be called
> non_secure_init or the like?
> 

cpu_non_secure_init would be a good name.

> >
> >>+	movw	r1, #0x3fff
> >>+	movt	r1, #0x0006
> >>+	mcr	p15, 0, r1, c1, c1, 2		@ NSACR = all copros to non-sec
> >>+
> >>+	ldr	r1, =_start
> >>+	mcr	p15, 0, r1, c12, c0, 0		@ set VBAR
> >>+	mcr	p15, 0, r1, c12, c0, 1		@ set MVBAR
> >
> >I thought you already took care of the MVBAR during init?
> 
> Right, as mentioned earlier I have fixed this already.
> 
> >>+
> >>+	isb
> >>+	smc	#0				@ call into MONITOR mode
> >>+	isb					@ clobbers r0 and r1
> >
> >This isb shouldn't be necessary if you just did an exception return?
> 
> Seems to be a paranoid leftover of one debug session...
> 

actually, neither isb should be necessary...

> >>+
> >>+	mov	r1, #1
> >>+	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
> >
> >you're doing more than setting the enable bit: you're setting the EOImodeNS,
> >IRQBypDisGrp1, and FIQBypDisGrp1, as you should.
> 
> But 0x01 is the correct value? And the reset value is 0, right?
> I will extend the comment.
> 

yeah my comment was only in reference to the code comment, the code is
correct.

> >>+	add	r2, r2, #0x1000			@ GIC dist i/f offset
> >>+	str	r1, [r2]			@ allow private interrupts
> >
> >It seems a bit brittle to rely on the smc handler to not clobber r2 and
> >r3, and it may an annoying thing to track down.  You could just
> >re-generate the the gic base address from the cp15 register.  Your
> >choice.
> 
> So shall I put comments here and at the smc handler?
> 

hmm, so I was thinking before that you should not rely on whatever the
smc handler does, but with the code that's added later, and especially
if you change this routine to take parameters, then it's fine as it is,
and we should just make sure the smc routine follows the AAPCS.


Thanks,
-Christoffer