Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Sam Ravnborg]

On Wed, Jun 06, 2007 at 06:24:53PM +0100, Catalin Marinas wrote:
> The following series of patches add support for compiling the kernel
> to the Thumb-2 ISA on an ARMv7 CPU.
> 
> Because of the issues listed below, merging the Thumb-2 support into
> the existing arch/arm files would clutter the existing code with
> macros like ARM, THUMB or W (for conditional compilation and the wide
> instruction format). The IT instruction is also not recognised by
> older toolchains and additional (assembler) macros would be needed,
> including support for the inline assembly. I therefore decided to
> create a separate arch/arm_t2 directory that shares a lot of code with
> the existing arch/arm.

Hi Catalin.
Can you please share with us the strong arguments why another (third)
arm architecture are needed?

Browsing through your patches there seems to be a common pattern:
1) Adding a bit more info to many assembler function
2) Small modifications to assembler function
3) Almost replaced assembler functions
4) Simpler Kconfig and Makefile


1) Could be solved in trivial ways using a few macros.
   One example:
   .type   printhex8, %function

Add a common macro like this:

#define FUNCTION(name)   .type name, %function
Introducing this simple macro in the generic ARM codebase would
bring down the diff significantly


2) For some parts it looked like:
   "We can do this different so lets do it different"
   I am rather ARM assembler ignorant so that may not hold true.
   But if it is true then look into what is really needed to change
   and if nothing else factor these out in files for the different
   situations and select the right one in the Makefile.

3) Factor these out in separate files.


4) Short term win. The maintenance will be much simpler avoiding
the cleanup.

As you are introducing a new architecture I have added linux-arch
to list of receivers - maybe someone there has more to say.

	Sam

[Rest of mail kept so linux-arch readers can see it]

Pointers to the mails with patches:
http://marc.info/?l=linux-arm-kernel&m=118115330624752&w=2
http://marc.info/?l=linux-arm-kernel&m=118115324023036&w=2
http://marc.info/?l=linux-arm-kernel&m=118115281427358&w=2
[PATCH 4/9] was not available at marc.info yet
[PATCH 5/9] was not available at marc.info yet
http://marc.info/?l=linux-arm-kernel&m=118115384801763&w=2
http://marc.info/?l=linux-arm-kernel&m=118115310810711&w=2
http://marc.info/?l=linux-arm-kernel&m=118115399504496&w=2
http://marc.info/?l=linux-arm-kernel&m=118115421005454&w=2

See full list at: http://marc.info/?l=linux-arm-kernel&r=1&b=200706&w=2


> 
> I avoided posting the first big patch in the series as it only
> contains the code duplicated from arch/arm into arch/arm_t2. Most of
> the .h files only contain an #include clause to the corresponding
> asm-arm version. Subsequent patches would show the differences with
> the original arch/arm code, making the review a lot easier.
> 
> The patches are also available on my GIT tree -
> http://www.linux-arm.org/git?p=linux-2.6.git;a=shortlog;h=2.6.21-thumb.
> They are currently based on 2.6.21 with some additional patches for
> the ARMv7 support (most of them being already merged by Russell in
> 2.6.22-rc1).
> 
> Thumb-2 ISA consists of either 16 or 32 bit instructions and are
> aligned to a two-byte boundary. ARM defined the unified assembler
> language that provides a canonical form for all the ARM and Thumb-2
> instructions. This is currently supported by the CodeSourcery's
> toolchains.
> 
> Even with the unified syntax, there are some differences between the
> instructions supported in ARM or Thumb modes (though the Thumb-2 code
> could be compiled into ARM code but with some performance penalties).
> 
> 
> Some of the main points regarding the Thumb-2 ISA:
> 
> - conditional execution is achieved using the IT (if-then) instruction
>   which can specify the execution of up to four subsequent
>   instructions (when compiling to ARM mode, this instruction is
>   ignored)
> 
> - post-indexing ldr/str with a register (ldr r0, [r1], r2) is not
>   supported in Thumb-2
> 
> - pre-indexing ldr/str with a shifted register only "lsl" is allowed
>   (ldr r0, [r1, r2, lsl #2])
> 
> - ldrt/strt do not support post-indexing
> 
> - ldm/stm cannot have "sp" in the register list (the latest ARM ARM
>   also states that ARM instructions that include "sp" in the list are
>   deprecated)
> 
> - ldm cannot have both "pc" and "lr" in the list (the latest ARM ARM
>   also states that these ARM instructions are deprecated)
> 
> - ldm/stm <registers-without-pc>^ are not available (kernel access to
>   the banked "sp" and "lr" is done by switching to the System mode)
> 
> - ldm <registers-with-pc>^ is not available (return from exception is
>   done with the "rfe" instruction)
> 
> - ldmda/stmib are not available
> 
> 
> Some other issues with Thumb-2 or the unified assembler syntax:
> 
> - the condition flags should always be placed at the end of the
>   instruction, i.e. "ldmiane" rather than "ldmneia". The new form is
>   not compilable by older assemblers (though the new assembler could
>   be modified to not warn about the old syntax)
> 
> - because Thumb-2 instructions are 16 bit aligned, instructions
>   loading data from the literal pool may trigger an alignment
>   fault. The assembler doesn't automatically align ".word" or ".long"
>   declarations to 32 bit. This has to be explicitly set via ".align"
> 
> - Thumb-2 32 bit instructions can be generated by appending ".w" to
>   the instruction, i.e. "mov.w pc, lr"
> 
> 
> Comments and suggestions are welcomed.
> 
> Regards.
> 
> -- 
> Catalin
> 
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Russell King - ARM Linux]

On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:
> #define FUNCTION(name)   .type name, %function
> Introducing this simple macro in the generic ARM codebase would
> bring down the diff significantly

We have such a macro already - ENDPROC().  Recently introduced,
we should probably make more use of it.

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Sam Ravnborg]

On Wed, Jun 06, 2007 at 09:47:49PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:
> > #define FUNCTION(name)   .type name, %function
> > Introducing this simple macro in the generic ARM codebase would
> > bring down the diff significantly
> 
> We have such a macro already - ENDPROC().  Recently introduced,
> we should probably make more use of it.
Took a short look at include/linux/linkage.h
There is also ENTRY() that seems resonable to introduce.

And btw. if arm in any way could benefit from using @function
it is obvious why we should not split it up. Becasue this pathset
did only add @function for one architecture.

	Sam

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Catalin Marinas]

On Wed, 2007-06-06 at 23:10 +0200, Sam Ravnborg wrote:
> On Wed, Jun 06, 2007 at 09:47:49PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:
> > > #define FUNCTION(name)   .type name, %function
> > > Introducing this simple macro in the generic ARM codebase would
> > > bring down the diff significantly
> > 
> > We have such a macro already - ENDPROC().  Recently introduced,
> > we should probably make more use of it.
> Took a short look at include/linux/linkage.h
> There is also ENTRY() that seems resonable to introduce.

The problem is that ENTRY is not used for functions only (I tried this
initially). The linker needs to know which .globl object is a function
because the Thumb-2 function addresses used in branch instructions have
bit 0 set (even if the address is 2 bytes aligned).

-- 
Catalin

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Russell King - ARM Linux]

On Wed, Jun 06, 2007 at 11:10:42PM +0200, Sam Ravnborg wrote:
> On Wed, Jun 06, 2007 at 09:47:49PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:
> > > #define FUNCTION(name)   .type name, %function
> > > Introducing this simple macro in the generic ARM codebase would
> > > bring down the diff significantly
> > 
> > We have such a macro already - ENDPROC().  Recently introduced,
> > we should probably make more use of it.
> Took a short look at include/linux/linkage.h
> There is also ENTRY() that seems resonable to introduce.
> 
> And btw. if arm in any way could benefit from using @function
> it is obvious why we should not split it up. Becasue this pathset
> did only add @function for one architecture.

The advantage is purely cosmetic for non-T2 ARM - it means that objdump
knows whether to disassemble stuff as code or data, which makes its
output very much easier to read.

Since it is merely cosmetic for non-T2, I've not been in a rush to
produce patches for it, but I've no objection to applying any such
patches during a merge window.  (I do have some open coded .type's
scattered around which should also be cleaned up at the same time,
particularly the head and entry code.)

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Catalin Marinas]

On Wed, 2007-06-06 at 20:29 +0200, Sam Ravnborg wrote:
> Can you please share with us the strong arguments why another (third)
> arm architecture are needed?

Well, it's not a strong argument but Thumb-2 is a different instruction
set. Even though there is a unified assembly language which tries to
reduce/eliminate the gap between ARM and Thumb, there are still
differences and, probably the most important, older toolchains don't
support it.

> Browsing through your patches there seems to be a common pattern:
> 1) Adding a bit more info to many assembler function
> 2) Small modifications to assembler function
> 3) Almost replaced assembler functions
> 4) Simpler Kconfig and Makefile
> 
> 
> 1) Could be solved in trivial ways using a few macros.
>    One example:
>    .type   printhex8, %function

Yes, this is needed so that the linker knows that the address of
printhex8 should have bit 1 set and also generate BL instead of BLX
instructions.

Adding this to the ARM code is harmless anyway (and I think we should
probably do it anyway). As Russell said, there is a macro already for
this.

> 2) For some parts it looked like:
>    "We can do this different so lets do it different"
>    I am rather ARM assembler ignorant so that may not hold true.
                 ^^^^^^^^^^^^^^^^^^^^^^
Indeed, it doesn't hold true :-). I tried to modify as little as
possible but see the Thumb-2 limitations in my original post for why it
isn't possible to use the existing code (not all variants of ldm/stm,
ldr/str, different model for exception return etc.).

> 3) Factor these out in separate files.

For those which are completely re-written, it makes sense to move them
to separate files.

> 4) Short term win. The maintenance will be much simpler avoiding
> the cleanup.

It didn't make sense to keep all the config options for platforms that
would never run Thumb-2 code.

> As you are introducing a new architecture I have added linux-arch
> to list of receivers - maybe someone there has more to say.

I think these patches are in a too early stage at the moment to cc
linux-arch and, if we decide not to go on this route (separate arch),
only discussing on linux-arm-kernel would be enough.

-- 
Catalin

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Russell King - ARM Linux]

On Wed, Jun 06, 2007 at 10:19:07PM +0100, Catalin Marinas wrote:
> On Wed, 2007-06-06 at 23:10 +0200, Sam Ravnborg wrote:
> > On Wed, Jun 06, 2007 at 09:47:49PM +0100, Russell King - ARM Linux wrote:
> > > On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:
> > > > #define FUNCTION(name)   .type name, %function
> > > > Introducing this simple macro in the generic ARM codebase would
> > > > bring down the diff significantly
> > > 
> > > We have such a macro already - ENDPROC().  Recently introduced,
> > > we should probably make more use of it.
> > Took a short look at include/linux/linkage.h
> > There is also ENTRY() that seems resonable to introduce.
> 
> The problem is that ENTRY is not used for functions only (I tried this
> initially). The linker needs to know which .globl object is a function
> because the Thumb-2 function addresses used in branch instructions have
> bit 0 set (even if the address is 2 bytes aligned).

Not suggesting we change ENTRY - that'd be bad.  Just add the relevant
sprinkling of END and ENDPROC statements in the assembly code (which
will add the .size for everything, and .type for functions.)

As I say, for existing ARM support it's merely cosmetic which is why
there isn't a rush to do it yet, but if it's needed for T2 there's no
reason not to.

Re: [RFC PATCH 0/9] Thumb-2 ISA kernel port

[Ralf Baechle]

On Wed, Jun 06, 2007 at 08:29:14PM +0200, Sam Ravnborg wrote:

> > Because of the issues listed below, merging the Thumb-2 support into
> > the existing arch/arm files would clutter the existing code with
> > macros like ARM, THUMB or W (for conditional compilation and the wide
> > instruction format). The IT instruction is also not recognised by
> > older toolchains and additional (assembler) macros would be needed,
> > including support for the inline assembly. I therefore decided to
> > create a separate arch/arm_t2 directory that shares a lot of code with
> > the existing arch/arm.
> 
> Hi Catalin.
> Can you please share with us the strong arguments why another (third)
> arm architecture are needed?

It's been my experience with 64-bit MIPS that starting off with a 2nd
arch then eventually factoring out the differences and unifying things
back into a single mips architecture was actually the easier road to go.

Downside: this approach takes long term careful coding.  It is hell with
the kind of "code once and resuse never" style that's only too common in
the embedded industry.

  Ralf