ARM: Add a generic macro for declaring proc_info structs
cover ARM: Add a generic macro for declaring proc_info structs
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(-)
Signature · 1 line
1.7.4.1
RFC PATCH 1/2 ARM: Add a generic macro for declaring proc_info structs
Using a macro allows the structure declarations to be more readable and shorter, as well as enabling build-time checking of the structure layout.
--- arch/arm/mm/proc-macros.S | 141 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 141 insertions(+), 0 deletions(-)
arch/arm/mm/proc-macros.S +141 -0
@@ -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
Signature · 1 line
1.7.4.1
RFC PATCH 2/2 ARM: proc-v7: Use the new proc_info declaration macro
In reply to Dave Martin--- arch/arm/mm/proc-v7.S | 93 ++++++++++++++++++++----------------------------- 1 files changed, 38 insertions(+), 55 deletions(-)
arch/arm/mm/proc-v7.S +38 -55
@@ -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
Signature · 1 line
1.7.4.1
cover ARM: Add a generic macro for declaring proc_info structs
In reply to Dave MartinQuoted 4 lines · Dave Martin
Based on recent problems with variable-size Thumb instructions inside tables, this patch adds an experimental macro for declaring
proc_infostructs, 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?
Quoted 3 lines
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.
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Dave Martin
Based on recent problems with variable-size Thumb instructions inside tables, this patch adds an experimental macro for declaring
proc_infostructs, 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_inforegion 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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 10 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
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_infostructs 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".)
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 12 lines · Russell King - ARM Linux
Quoted 10 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
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_infostructs don't tend to get changed much after they're initially written, I guess that such mistakes don't actually occur very often, though.
Quoted 8 lines
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.
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 59 lines · Russell King - ARM Linux
Quoted 12 lines · Russell King - ARM Linux
Quoted 10 lines · Dave Martin
Quoted 3 lines · Russell King - ARM Linux
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_infostructs don't tend to get changed much after they're initially written, I guess that such mistakes don't actually occur very often, though.Quoted 8 lines
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 .endmand 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.
Quoted 6 lines
The cache and tlb structures follow a very similar naming scheme to the above.
The
proc_infostructures 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
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 5 lines · Dave Martin
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.
cover ARM: Add a generic macro for declaring proc_info structs
Quoted 11 lines · Russell King - ARM Linux
Quoted 5 lines · Dave Martin
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