RFC: Status of kernel/fiq.c

RFC: Status of kernel/fiq.c

[Dave Martin]

Hi all,

I've hit a Thumb-2 compatibility issue in the assembler functions
in kernel/fiq.c, and I'm wondering how to proceed.

With regard to set_fiq_regs() and get_fiq_regs(), does anyone know:

  * whether we still need the stack frame manipulation code in this
	leaf function, e.g.

        "mov    ip, sp\n\
        stmfd  sp!, {fp, ip, lr, pc}\n\
        sub    fp, ip, #4\n\

	etc.

  * what the nop (mov r0,r0) after the CPSR writes is for
        (I'm guessing there may have been some 1-instruction hazard
        here for some old processors, but I'm not aware of a need
        for this in anything current.)

  * should anyone be using fiq.c on v7 platforms?

As the code stands, it could also go wrong if the compiler
happens to allocate r8-r12 or lr for one of the inline asm
constraints -- though this is pretty unlikely to happen, and
I doubt if it ever has happened in practice.


To make this Thumb-2/v7 compatible, I'd prefer to recode a single
version that works for ARM and Thumb and avoids deprecated
instruction forms, but that may result in some ugly ifdefs to work
around the stack frame manipulation etc., unless we can conclude
that that code isn't needed.

Splitting these functions out into a separate assembler file might
also make sense, since that makes it impossible for the compiler
to mess things up, and makes is possible to use ARM()/THUMB()
which is a bit cleaner than #ifdefs.

Comments welcome.

Cheers
---Dave

RFC: Status of kernel/fiq.c

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 05:05:47PM +0100, Dave Martin wrote:
> Hi all,
> 
> I've hit a Thumb-2 compatibility issue in the assembler functions
> in kernel/fiq.c, and I'm wondering how to proceed.
> 
> With regard to set_fiq_regs() and get_fiq_regs(), does anyone know:
> 
>   * whether we still need the stack frame manipulation code in this
> 	leaf function, e.g.
> 
>         "mov    ip, sp\n\
>         stmfd  sp!, {fp, ip, lr, pc}\n\
>         sub    fp, ip, #4\n\

That's the standard APCS stack frame.  Look at the function closely
and you'll notice that its declared NAKED, so the compiler doesn't
generate anything for it.  So we have to do this by hand.

>   * what the nop (mov r0,r0) after the CPSR writes is for
>         (I'm guessing there may have been some 1-instruction hazard
>         here for some old processors, but I'm not aware of a need
>         for this in anything current.)

You're correct, and its for ARMv3 CPUs.  After writing the CPSR you
have to wait one cycle before accessing the banked registers.  On
other platforms that has been solved so the nop is harmless even
though it's not necessary.  Simplification of code dictates that we
don't try to ifdef it.

>   * should anyone be using fiq.c on v7 platforms?

Is the FIQ wired to anything useful on ARMv7.

> As the code stands, it could also go wrong if the compiler
> happens to allocate r8-r12 or lr for one of the inline asm
> constraints -- though this is pretty unlikely to happen, and
> I doubt if it ever has happened in practice.

Actually, that goes for r8 to r14 inclusive - as we switch the CPU mode to
FIQ, we lose access to any values held in those registers prior to the
assembly block being started.

The only possibility of it being unsafe is if the compiler allocates 'ip'
for 'tmp'.  We could work around that by getting rid of 'tmp' or telling
the compiler that 'tmp' is a register variable and it will be r3.

RFC: Status of kernel/fiq.c

[Dave Martin]

On Tue, Apr 5, 2011 at 5:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 05, 2011 at 05:05:47PM +0100, Dave Martin wrote:
>> Hi all,
>>
>> I've hit a Thumb-2 compatibility issue in the assembler functions
>> in kernel/fiq.c, and I'm wondering how to proceed.
>>
>> With regard to set_fiq_regs() and get_fiq_regs(), does anyone know:
>>
>> ? * whether we still need the stack frame manipulation code in this
>> ? ? ? leaf function, e.g.
>>
>> ? ? ? ? "mov ? ?ip, sp\n\
>> ? ? ? ? stmfd ?sp!, {fp, ip, lr, pc}\n\
>> ? ? ? ? sub ? ?fp, ip, #4\n\
>
> That's the standard APCS stack frame. ?Look at the function closely
> and you'll notice that its declared NAKED, so the compiler doesn't
> generate anything for it. ?So we have to do this by hand.

I guess my question was more whether it's necessary to generate the
stack frame at all for a leaf function such as this -- partly because
the instruction forms involved are themselves deprecated in v7 (lr and
pc in the same ldm/stm, stm of the pc).  We can code around that, but
it gets increasingly ugly.

I guess it we get a fault while transferring the registers, the stack
frame might be needed in order to get an intelligible crash dump, but
otherwise... ?

For Thumb-2 it's not applicable of course, since we don't have
APCS-style stack frames there; and since coding the equivalent code in
Thumb would be painful my preference would be to #ifdef it out.

>
>> ? * what the nop (mov r0,r0) after the CPSR writes is for
>> ? ? ? ? (I'm guessing there may have been some 1-instruction hazard
>> ? ? ? ? here for some old processors, but I'm not aware of a need
>> ? ? ? ? for this in anything current.)
>
> You're correct, and its for ARMv3 CPUs. ?After writing the CPSR you
> have to wait one cycle before accessing the banked registers. ?On
> other platforms that has been solved so the nop is harmless even
> though it's not necessary. ?Simplification of code dictates that we
> don't try to ifdef it.

OK; it's harmless to leave that instruction in there, but it's
interesting to know.

>
>> ? * should anyone be using fiq.c on v7 platforms?
>
> Is the FIQ wired to anything useful on ARMv7.

FIQ gets used by TrustZone on platforms which use it, so I think it's
becoming less common to use it for normal peripherals.  The latency
difference between IRC and FIQ is also less important in more recent
platforms, so I think it's generally seeing less use.

I need to check my config -- maybe CONFIG_FIQ is getting pulled in
inappropriately.

set_fiq_regs() for example isn't used by much...

linux$ git grep set_fiq_regs
arch/arm/include/asm/fiq.h:extern void set_fiq_regs(struct pt_regs *regs);
arch/arm/kernel/fiq.c:void __naked set_fiq_regs(struct pt_regs *regs)
arch/arm/kernel/fiq.c:EXPORT_SYMBOL(set_fiq_regs);
arch/arm/mach-omap1/ams-delta-fiq.c:	set_fiq_regs(&FIQ_regs);
arch/arm/mach-rpc/dma.c:	set_fiq_regs(&regs);
drivers/media/video/mx1_camera.c:	set_fiq_regs(&regs);
drivers/spi/spi_s3c24xx.c:	set_fiq_regs(&regs);
sound/soc/imx/imx-pcm-fiq.c:	set_fiq_regs(&regs);

>
>> As the code stands, it could also go wrong if the compiler
>> happens to allocate r8-r12 or lr for one of the inline asm
>> constraints -- though this is pretty unlikely to happen, and
>> I doubt if it ever has happened in practice.
>
> Actually, that goes for r8 to r14 inclusive - as we switch the CPU mode to
> FIQ, we lose access to any values held in those registers prior to the
> assembly block being started.

True -- I was assuming that the compiler won't touch sp for register
allocation purposes, though the fact that we temporarily switch out
this register provides yet another reason why the compiler shouldn't
touch it (albeit a reason the compiler doesn't know about).

>
> The only possibility of it being unsafe is if the compiler allocates 'ip'
> for 'tmp'. ?We could work around that by getting rid of 'tmp' or telling
> the compiler that 'tmp' is a register variable and it will be r3.
>

That was basically my fix too.  Using the same style as the existing
code but using register variables with explicit register assignments
keeps things pretty clear.

I find it hard to imagine the compiler being so crazy in its register
allocation, but there are occasional surprises...

Cheers
---Dave

RFC: Status of kernel/fiq.c

[Nicolas Pitre]

On Tue, 5 Apr 2011, Dave Martin wrote:

> Hi all,
> 
> I've hit a Thumb-2 compatibility issue in the assembler functions
> in kernel/fiq.c, and I'm wondering how to proceed.
> 
> With regard to set_fiq_regs() and get_fiq_regs(), does anyone know:
> 
>   * whether we still need the stack frame manipulation code in this
> 	leaf function, e.g.
> 
>         "mov    ip, sp\n\
>         stmfd  sp!, {fp, ip, lr, pc}\n\
>         sub    fp, ip, #4\n\
> 
> 	etc.

You could use #ifdef CONFIG_FRAME_POINTER ... #endif around it.

>   * what the nop (mov r0,r0) after the CPSR writes is for
>         (I'm guessing there may have been some 1-instruction hazard
>         here for some old processors, but I'm not aware of a need
>         for this in anything current.)

It may not hurt either.

>   * should anyone be using fiq.c on v7 platforms?
> 
> As the code stands, it could also go wrong if the compiler
> happens to allocate r8-r12 or lr for one of the inline asm
> constraints -- though this is pretty unlikely to happen, and
> I doubt if it ever has happened in practice.

I don't think this is a high concern indeed.

> To make this Thumb-2/v7 compatible, I'd prefer to recode a single
> version that works for ARM and Thumb and avoids deprecated
> instruction forms, but that may result in some ugly ifdefs to work
> around the stack frame manipulation etc., unless we can conclude
> that that code isn't needed.

I don't think this is really needed. Many other assembly coded functions 
in the kernel don't have frame pointer handling at all.  If some 
backtrace is ever generated from within this function then the call 
trace will still show the caller.

> Splitting these functions out into a separate assembler file might
> also make sense, since that makes it impossible for the compiler
> to mess things up, and makes is possible to use ARM()/THUMB()
> which is a bit cleaner than #ifdefs.

That could effectively be a cleaner solution.


Nicolas