[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Dave Martin]

Based on recent problems with variable-size Thumb instructions
inside tables, this patch adds an experimental macro for declaring
proc_info structs, as an example of the kind of build-time robustness
we could implement for these and similar structures.

As such, it's an illustration of something we _could_ do, rather
than something we necessarily should do.

I'd be interested in people's views on this.


Potential advantages include:

 * automatic build-time checking that struct proc_info is reasonably
in sync with the assembler definitions;

 * automatic checking that proc_info structure declarations are
complete and correctly structured, with no mis-sized or misaligned
elements (particularly important for Thumb-2 kernels);

 * better readability of the declarations (depending on people's
personal taste, of course);

 * reduction of some boilerplate code (perhaps up to 700 lines net
if we were to convert all arch/arm/mm/proc-*.S to use the macro.


The same general technique could be potentially expanded to cover
any struct defined in assembler.

However, this may be taking a sledgehammer to crack a nut,
and it will cause some churn, though it could leave is with a
cleaner situation afterwards.


Much lighter-weight, less invasive build-time checks could
still deliver significant benefits.


Dave Martin (2):
  ARM: Add a generic macro for declaring proc_info structs
  ARM: proc-v7: Use the new proc_info declaration macro

 arch/arm/mm/proc-macros.S |  141 +++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/proc-v7.S     |   93 ++++++++++++-----------------
 2 files changed, 179 insertions(+), 55 deletions(-)

-- 
1.7.4.1

[RFC PATCH 1/2] ARM: Add a generic macro for declaring proc_info structs

[Dave Martin]

Using a macro allows the structure declarations to be more
readable and shorter, as well as enabling build-time checking
of the structure layout.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/proc-macros.S |  141 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 141 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index 34261f9..fe78f3b 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -254,3 +254,144 @@
 	mcr	p15, 0, r0, c7, c10, 1		@ clean L1 D line
 	mcr	p15, 0, ip, c7, c10, 4		@ data write barrier
 	.endm
+
+/*
+ * proc_info structure declaration macro
+ */
+	.macro proc_info name:req
+		.macro _proc_check_align fieldname:req, align:req
+			.if . - \name != (\align )
+				.error "Unexpected or misaligned proc_info field \fieldname"
+			.endif
+		.endm
+
+		.macro cpu_val value:req
+			_proc_check_align cpu_val, 0
+			.long \value
+		.endm
+
+		.macro cpu_mask value:req
+			_proc_check_align cpu_mask, 4
+			.long \value
+		.endm
+
+		/*
+		 * Normal memory MMU flags for CPUs with no SMP support.
+		 * For CPUs with SMP support, use __cpu_mm_mmu_flags_smp and
+		 * __cpu_mm_mmu_flags_up instead.
+		 */
+		.macro __cpu_mm_mmu_flags value:req
+			_proc_check_align __cpu_mm_mmu_flags, PROCINFO_MM_MMUFLAGS
+			.long \value
+		.endm
+
+		.macro __cpu_mm_mmu_flags_smp value:req
+			_proc_check_align __cpu_mm_mmu_flags_smp, PROCINFO_MM_MMUFLAGS
+ ALT_SMP(		.long \value	)
+		.endm
+
+		/*
+		 * __cpu_mm_mmu_flags_up must follow __cpu_mm_mmu_flags_smp,
+		 * so the assembly-time offset is actually PROCINFO_IO_MMUFLAGS.
+		 * This macro generates fixups, rather than direct output.
+		 */
+		.macro __cpu_mm_mmu_flags_up value:req
+			_proc_check_align __cpu_mm_mmu_flags_up, PROCINFO_IO_MMUFLAGS
+ ALT_UP(		.long \value	)
+		.endm
+
+		.macro __cpu_io_mmu_flags value:req
+			_proc_check_align __cpu_io_mmu_flags, PROCINFO_IO_MMUFLAGS
+			.long \value
+		.endm
+
+		.macro __cpu_flush insn:vararg
+			_proc_check_align __cpu_flush, PROCINFO_INITFUNC
+			\insn
+		.endm
+
+		.macro arch_name string:req
+			_proc_check_align arch_name, 0x14
+			.pushsection .rodata.str, "aMS", 1
+\name\()_arch_name:		.asciz "\string"
+				.size \name\()_arch_name, . - \name\()_arch_name
+				.type \name\()_arch_name, #object
+			.popsection
+			.long \name\()_arch_name
+		.endm
+
+		.macro elf_name string:req
+			_proc_check_align elf_name, 0x18
+			.pushsection .rodata.str, "aMS", 1
+\name\()_elf_name:		.asciz "\string"
+				.size \name\()_elf_name, . - \name\()_elf_name
+				.type \name\()_elf_name, #object
+			.popsection
+			.long \name\()_elf_name
+		.endm
+
+		.macro elf_hwcap value:req
+			_proc_check_align elf_hwcap, 0x1c
+			.long \value
+		.endm
+
+		.macro cpu_name string:req
+			_proc_check_align cpu_name, 0x20
+			.pushsection .rodata.str, "aMS", 1
+\name\()_cpu_name:	.asciz "\string"
+				.size \name\()_cpu_name, . - \name\()_cpu_name
+				.type \name\()_cpu_name, #object
+			.popsection
+			.long \name\()_cpu_name
+		.endm
+
+		.macro proc label:req
+			_proc_check_align proc, 0x24
+			.long \label
+		.endm
+
+		.macro tlb label:req
+			_proc_check_align tlb, 0x28
+			.long \label
+		.endm
+
+		.macro user label:req
+			_proc_check_align user, 0x2c
+			.long \label
+		.endm
+
+		.macro cache label:req
+			_proc_check_align cache, 0x30
+			.long \label
+		.endm
+
+		.macro end_proc_info
+			.if . - \name != PROC_INFO_SZ
+				.error "incorrect number of fields in proc_info"
+			.endif
+			.size \name , . - \name
+			.type \name , #object
+			.popsection
+			.purgem end_proc_info
+			.purgem _proc_check_align
+			.purgem cpu_val
+			.purgem cpu_mask
+			.purgem __cpu_mm_mmu_flags
+			.purgem __cpu_mm_mmu_flags_smp
+			.purgem __cpu_mm_mmu_flags_up
+			.purgem __cpu_io_mmu_flags
+			.purgem __cpu_flush
+			.purgem arch_name
+			.purgem elf_name
+			.purgem elf_hwcap
+			.purgem cpu_name
+			.purgem proc
+			.purgem tlb
+			.purgem user
+			.purgem cache
+		.endm
+
+		.pushsection .proc.info.init, #alloc, #execinstr
+		.align 2
+\name :
+	.endm
-- 
1.7.4.1

[RFC PATCH 2/2] ARM: proc-v7: Use the new proc_info declaration macro

[Dave Martin]

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/proc-v7.S |   93 ++++++++++++++++++++-----------------------------
 1 files changed, 38 insertions(+), 55 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index b3b566e..9e8569c 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -171,8 +171,6 @@ ENTRY(cpu_v7_set_pte_ext)
 	mov	pc, lr
 ENDPROC(cpu_v7_set_pte_ext)
 
-cpu_v7_name:
-	.ascii	"ARMv7 Processor"
 	.align
 
 	/*
@@ -421,78 +419,63 @@ ENTRY(v7_processor_functions)
 	.word	0
 	.size	v7_processor_functions, . - v7_processor_functions
 
-	.section ".rodata"
-
-	.type	cpu_arch_name, #object
-cpu_arch_name:
-	.asciz	"armv7"
-	.size	cpu_arch_name, . - cpu_arch_name
-
-	.type	cpu_elf_name, #object
-cpu_elf_name:
-	.asciz	"v7"
-	.size	cpu_elf_name, . - cpu_elf_name
-	.align
-
-	.section ".proc.info.init", #alloc, #execinstr
-
-	.type   __v7_ca9mp_proc_info, #object
-__v7_ca9mp_proc_info:
-	.long	0x410fc090		@ Required ID value
-	.long	0xff0ffff0		@ Mask for ID
-	ALT_SMP(.long \
+proc_info __v7_ca9mp_proc_info
+	cpu_val		0x410fc090		@ Required ID value
+	cpu_mask	0xff0ffff0		@ Mask for ID
+	__cpu_mm_mmu_flags_smp \
 		PMD_TYPE_SECT | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ | \
-		PMD_FLAGS_SMP)
-	ALT_UP(.long \
+		PMD_FLAGS_SMP
+	__cpu_mm_mmu_flags_up \
 		PMD_TYPE_SECT | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ | \
-		PMD_FLAGS_UP)
-	.long   PMD_TYPE_SECT | \
+		PMD_FLAGS_UP
+	__cpu_io_mmu_flags \
+		PMD_TYPE_SECT | \
 		PMD_SECT_XN | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ
-	W(b)	__v7_ca9mp_setup
-	.long	cpu_arch_name
-	.long	cpu_elf_name
-	.long	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
-	.long	cpu_v7_name
-	.long	v7_processor_functions
-	.long	v7wbi_tlb_fns
-	.long	v6_user_fns
-	.long	v7_cache_fns
-	.size	__v7_ca9mp_proc_info, . - __v7_ca9mp_proc_info
+	__cpu_flush	W(b)	__v7_ca9mp_setup
+	arch_name	"armv7"
+	elf_name	"v7"
+	elf_hwcap	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
+	cpu_name	"ARMv7 Processor"
+	proc		v7_processor_functions
+	tlb		v7wbi_tlb_fns
+	user		v6_user_fns
+	cache		v7_cache_fns
+end_proc_info
 
 	/*
 	 * Match any ARMv7 processor core.
 	 */
-	.type	__v7_proc_info, #object
-__v7_proc_info:
-	.long	0x000f0000		@ Required ID value
-	.long	0x000f0000		@ Mask for ID
-	ALT_SMP(.long \
+proc_info __v7_proc_info
+	cpu_val		0x000f0000		@ Required ID value
+	cpu_mask	0x000f0000		@ Mask for ID
+	__cpu_mm_mmu_flags_smp \
 		PMD_TYPE_SECT | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ | \
-		PMD_FLAGS_SMP)
-	ALT_UP(.long \
+		PMD_FLAGS_SMP
+	__cpu_mm_mmu_flags_up \
 		PMD_TYPE_SECT | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ | \
-		PMD_FLAGS_UP)
-	.long   PMD_TYPE_SECT | \
+		PMD_FLAGS_UP
+	__cpu_io_mmu_flags \
+		PMD_TYPE_SECT | \
 		PMD_SECT_XN | \
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ
-	W(b)	__v7_setup
-	.long	cpu_arch_name
-	.long	cpu_elf_name
-	.long	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
-	.long	cpu_v7_name
-	.long	v7_processor_functions
-	.long	v7wbi_tlb_fns
-	.long	v6_user_fns
-	.long	v7_cache_fns
-	.size	__v7_proc_info, . - __v7_proc_info
+	__cpu_flush	W(b)	__v7_setup
+	arch_name	"armv7"
+	elf_name	"v7"
+	elf_hwcap	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
+	cpu_name	"ARMv7 Processor"
+	proc		v7_processor_functions
+	tlb		v7wbi_tlb_fns
+	user		v6_user_fns
+	cache		v7_cache_fns
+end_proc_info
-- 
1.7.4.1

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Russell King - ARM Linux]

On Thu, Jun 09, 2011 at 06:21:50PM +0100, Dave Martin wrote:
> Based on recent problems with variable-size Thumb instructions
> inside tables, this patch adds an experimental macro for declaring
> proc_info structs, as an example of the kind of build-time robustness
> we could implement for these and similar structures.

Could we just check the size of the proc_info region in the linker
is a multiple of the struct size we expect?

> However, this may be taking a sledgehammer to crack a nut,
> and it will cause some churn, though it could leave is with a
> cleaner situation afterwards.

It does look very much like a sledge hammer to me.  All we're really
after is whether the size of the region is what we expect it to be -
which will tell us whether there's a T2 instruction in there.

It's also fragile - if the struct has a member inserted, who says that
the offsets in the macro will be updated anyway... so it still suffers
from the same problem of no real build-time checking.

At least if we check that the size of the region is a multiple of the
struct size, we can catch whether there's any mismatch between the
struct size and assembly rather trivially.

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Dave Martin]

On Thu, Jun 09, 2011 at 06:38:36PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 09, 2011 at 06:21:50PM +0100, Dave Martin wrote:
> > Based on recent problems with variable-size Thumb instructions
> > inside tables, this patch adds an experimental macro for declaring
> > proc_info structs, as an example of the kind of build-time robustness
> > we could implement for these and similar structures.
> 
> Could we just check the size of the proc_info region in the linker
> is a multiple of the struct size we expect?

This might work; it's possible to add assertions in the linker
script, but section alignment padding would mask some errors.
It would be better than having no check, though.

> > However, this may be taking a sledgehammer to crack a nut,
> > and it will cause some churn, though it could leave is with a
> > cleaner situation afterwards.
> 
> It does look very much like a sledge hammer to me.  All we're really
> after is whether the size of the region is what we expect it to be -
> which will tell us whether there's a T2 instruction in there.

True, although the intent was no to solve just that one problem,
but to show how to avoid a whole variety of trivial mistakes.
Since proc_info structs don't tend to get changed much after
they're initially written, I guess that such mistakes don't actually
occur very often, though.

> It's also fragile - if the struct has a member inserted, who says that
> the offsets in the macro will be updated anyway... so it still suffers
> from the same problem of no real build-time checking.

That is actually somewhat solvable using the automatically updated
asm-offsets.h constants.  I only use the constants which already
exist already: there isn't one for every field, but one could be defined
for every field in the structure.

> At least if we check that the size of the region is a multiple of the
> struct size, we can catch whether there's any mismatch between the
> struct size and assembly rather trivially.

For the proc_types stuff in compressed/head.S, I've proposed basically
the same check you describe:
http://article.gmane.org/gmane.linux.ports.arm.kernel/119940/match=proc_type

This is at the other end of the the spectrum, but is pretty non-
invasive, and will probably catch the common mistakes.


As for the heavyweight version, I will file it away under
"interesting exercises".  That technique might come in handy
sometime, but I agree it's probably overkill for this kind of case.

Cheers
---Dave

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Russell King - ARM Linux]

On Fri, Jun 10, 2011 at 09:57:52AM +0100, Dave Martin wrote:
> On Thu, Jun 09, 2011 at 06:38:36PM +0100, Russell King - ARM Linux wrote:
> > It does look very much like a sledge hammer to me.  All we're really
> > after is whether the size of the region is what we expect it to be -
> > which will tell us whether there's a T2 instruction in there.
> 
> True, although the intent was no to solve just that one problem,
> but to show how to avoid a whole variety of trivial mistakes.
> Since proc_info structs don't tend to get changed much after
> they're initially written, I guess that such mistakes don't actually
> occur very often, though.

We have several other structures encoded in assembly - and to do this
for every one would be excessive.  The amount of churn produced too
would be large.

Historically we've had little problem with these structs being wrong -
about once or twice pre-Thumb for a decade IIRC (where at least one was
down to "using an old patch but didn't update it properly for the new
proc-*.S files".)

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Russell King - ARM Linux]

On Sun, Jun 12, 2011 at 09:22:21AM +0100, Russell King - ARM Linux wrote:
> On Fri, Jun 10, 2011 at 09:57:52AM +0100, Dave Martin wrote:
> > On Thu, Jun 09, 2011 at 06:38:36PM +0100, Russell King - ARM Linux wrote:
> > > It does look very much like a sledge hammer to me.  All we're really
> > > after is whether the size of the region is what we expect it to be -
> > > which will tell us whether there's a T2 instruction in there.
> > 
> > True, although the intent was no to solve just that one problem,
> > but to show how to avoid a whole variety of trivial mistakes.
> > Since proc_info structs don't tend to get changed much after
> > they're initially written, I guess that such mistakes don't actually
> > occur very often, though.
> 
> We have several other structures encoded in assembly - and to do this
> for every one would be excessive.  The amount of churn produced too
> would be large.
> 
> Historically we've had little problem with these structs being wrong -
> about once or twice pre-Thumb for a decade IIRC (where at least one was
> down to "using an old patch but didn't update it properly for the new
> proc-*.S files".)

What may be worth doing is something like this, where all entries follow
a well defined naming scheme:

	.macro	processor_functions, name, dabort, pabort, suspend, nommu
	.type	\name\()_processor_functions, #object
ENTRY(\name\()_processor_functions)
	.word	\dabort\()_abort
	.word	\pabort\()_pabort
	.word	cpu_\name\()_proc_init
	.word	cpu_\name\()_proc_fin
	.word	cpu_\name\()_reset
	.word	cpu_\name\()_do_idle
	.word	cpu_\name\()_dcache_clean_area
	.word	cpu_\name\()_switch_mm
	.ifne \nommu
	.word	0
	.else
	.word	cpu_\name\()_set_pte_ext
	.endif
	.ifne \suspend
	.word	cpu_\name\()_suspend_size
	.word	cpu_\name\()_do_suspend
	.word	cpu_\name\()_do_resume
	.else
	.word	0
	.word	0
	.word	0
	.endif
	.size	\name\()_processor_functions, . - \name\()_processor_functions
	.endm

and this has great value to ensuring that the structure in assembly
matches what is in the headers as it means that instead of having N
places to update these structures, we're down to its header and one
macro - and also ensures that proc files which miss out on the update
won't link to an apparantly working kernel.

The cache and tlb structures follow a very similar naming scheme to
the above.

The proc_info structures are much harder to deal with because they
don't contain such commonality, but I'm sure it may be possible to
come up with something along these lines.

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Dave Martin]

On Sun, Jun 12, 2011 at 04:14:22PM +0100, Russell King - ARM Linux wrote:
> On Sun, Jun 12, 2011 at 09:22:21AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Jun 10, 2011 at 09:57:52AM +0100, Dave Martin wrote:
> > > On Thu, Jun 09, 2011 at 06:38:36PM +0100, Russell King - ARM Linux wrote:
> > > > It does look very much like a sledge hammer to me.  All we're really
> > > > after is whether the size of the region is what we expect it to be -
> > > > which will tell us whether there's a T2 instruction in there.
> > > 
> > > True, although the intent was no to solve just that one problem,
> > > but to show how to avoid a whole variety of trivial mistakes.
> > > Since proc_info structs don't tend to get changed much after
> > > they're initially written, I guess that such mistakes don't actually
> > > occur very often, though.
> > 
> > We have several other structures encoded in assembly - and to do this
> > for every one would be excessive.  The amount of churn produced too
> > would be large.
> > 
> > Historically we've had little problem with these structs being wrong -
> > about once or twice pre-Thumb for a decade IIRC (where at least one was
> > down to "using an old patch but didn't update it properly for the new
> > proc-*.S files".)
> 
> What may be worth doing is something like this, where all entries follow
> a well defined naming scheme:
> 
> 	.macro	processor_functions, name, dabort, pabort, suspend, nommu
> 	.type	\name\()_processor_functions, #object
> ENTRY(\name\()_processor_functions)
> 	.word	\dabort\()_abort
> 	.word	\pabort\()_pabort
> 	.word	cpu_\name\()_proc_init
> 	.word	cpu_\name\()_proc_fin
> 	.word	cpu_\name\()_reset
> 	.word	cpu_\name\()_do_idle
> 	.word	cpu_\name\()_dcache_clean_area
> 	.word	cpu_\name\()_switch_mm
> 	.ifne \nommu
> 	.word	0
> 	.else
> 	.word	cpu_\name\()_set_pte_ext
> 	.endif
> 	.ifne \suspend
> 	.word	cpu_\name\()_suspend_size
> 	.word	cpu_\name\()_do_suspend
> 	.word	cpu_\name\()_do_resume
> 	.else
> 	.word	0
> 	.word	0
> 	.word	0
> 	.endif
> 	.size	\name\()_processor_functions, . - \name\()_processor_functions
> 	.endm
> 
> and this has great value to ensuring that the structure in assembly
> matches what is in the headers as it means that instead of having N
> places to update these structures, we're down to its header and one
> macro - and also ensures that proc files which miss out on the update
> won't link to an apparantly working kernel.
> 
> The cache and tlb structures follow a very similar naming scheme to
> the above.
> 
> The proc_info structures are much harder to deal with because they
> don't contain such commonality, but I'm sure it may be possible to
> come up with something along these lines.

Hmmm, that could be quite a nice approach.

I guess it's not urgent, but I'll have a think about it and maybe
propose a patch for just one of the structures initially, to see what
it looks like.

Using macros such as you suggest would force a consistent naming on
different CPUs' helper functions, which actually seems rather a good thing.
A possible downside is that the relationship between those functions
and the macro becomes invisible in proc-*.S (though a very brief comment
or two could easibly address that).

Cheers
---Dave

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Russell King - ARM Linux]

On Mon, Jun 13, 2011 at 02:10:08PM +0100, Dave Martin wrote:
> Using macros such as you suggest would force a consistent naming on
> different CPUs' helper functions, which actually seems rather a good thing.
> A possible downside is that the relationship between those functions
> and the macro becomes invisible in proc-*.S (though a very brief comment
> or two could easibly address that).

The consistent naming is already required for the !MULTI_CPU, etc.
Same for the cache and tlb structs.

The processor structure has less strict requirements though.

[RFC PATCH 0/2] ARM: Add a generic macro for declaring proc_info structs

[Dave Martin]

On Mon, Jun 13, 2011 at 02:12:57PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 13, 2011 at 02:10:08PM +0100, Dave Martin wrote:
> > Using macros such as you suggest would force a consistent naming on
> > different CPUs' helper functions, which actually seems rather a good thing.
> > A possible downside is that the relationship between those functions
> > and the macro becomes invisible in proc-*.S (though a very brief comment
> > or two could easibly address that).
> 
> The consistent naming is already required for the !MULTI_CPU, etc.
> Same for the cache and tlb structs.
> 
> The processor structure has less strict requirements though.

OK, well I might have a go at this for the CPU func structs at some point,
and see how it turns out.

Cheers
---Dave