| Message ID | 1308049105-16080-2-git-send-email-dave.martin@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 14 Jun 2011, Dave Martin wrote: > This patch adds some generic macros to reduce boilerplate when > declaring certain common structures in arch/arm/mm/*.S > > Thanks to Russell King for outlining what the > define_processor_functions macro could look like. > > Signed-off-by: Dave Martin <dave.martin@linaro.org> > --- > arch/arm/mm/proc-macros.S | 79 +++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 79 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S > index 34261f9..3c5ffbb 100644 > --- a/arch/arm/mm/proc-macros.S > +++ b/arch/arm/mm/proc-macros.S > @@ -254,3 +255,81 @@ > mcr p15, 0, r0, c7, c10, 1 @ clean L1 D line > mcr p15, 0, ip, c7, c10, 4 @ data write barrier > .endm > + > +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0 > + .pushsection .text > + > + __INITDATA Was this __INITDATA right after a .pushsection intentional? One might be confused about the effective section after this. The __INITDATA tag is actually doing a .section ".init.data","aw",%progbits Maybe this could be used as argument to .pushsection instead of .text (which in this case should probably have been .rodata otherwise anyway). Nicolas
On Wed, Jun 15, 2011 at 07:57:13PM -0400, Nicolas Pitre wrote: > On Tue, 14 Jun 2011, Dave Martin wrote: > > > This patch adds some generic macros to reduce boilerplate when > > declaring certain common structures in arch/arm/mm/*.S > > > > Thanks to Russell King for outlining what the > > define_processor_functions macro could look like. > > > > Signed-off-by: Dave Martin <dave.martin@linaro.org> > > --- > > arch/arm/mm/proc-macros.S | 79 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 79 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S > > index 34261f9..3c5ffbb 100644 > > --- a/arch/arm/mm/proc-macros.S > > +++ b/arch/arm/mm/proc-macros.S > > @@ -254,3 +255,81 @@ > > mcr p15, 0, r0, c7, c10, 1 @ clean L1 D line > > mcr p15, 0, ip, c7, c10, 4 @ data write barrier > > .endm > > + > > +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0 > > + .pushsection .text > > + > > + __INITDATA > > Was this __INITDATA right after a .pushsection intentional? One might > be confused about the effective section after this. The .pushsection is needed in order to save the previous state of the section stack so that it can be restored later with .popsection. .pushsection syntactically requires an argument, though in this case it's not useful, so I just specify .text since this should be harmless. This is kind of unclear, so at the very least it could benefit from a comment. > > The __INITDATA tag is actually doing a > > .section ".init.data","aw",%progbits > > Maybe this could be used as argument to .pushsection instead of .text > (which in this case should probably have been .rodata otherwise anyway). The problem is that __INITDATA conflates the information about the section with the action of actually switching to the section. Nowhere are the section name and flags available by themselves. Since the point of a macro is to avoid copy-paste, I prefer not to manually copy-paste this stuff into a .pushsection directive in a random assembler file... Perhaps we should instead use __INITDATA /* ... */ .previous .endm I tend to avoid ".previous" because I consider it somewhat broken: in the presence of macros, the section restored by .previous is indeterminate because a called macro may switch sections and hence change what .previous will refer to. There is no way to restore what .previous points at after switching sections. In this case, we don't call any nested macro, so .previous _should_ refer to the right thing. We mess up .previous for the caller, but then we always mess that up when switching sections anyway. Nothing in arch/arm/mm/*.S uses .previous, so we shouldn't get any problems resulting from this, in this particular case. Alternatively, we could propose a change <linux/init.h> to split the __INITDATA macro up, and define the related macros in a sane way: #define __INITDATA_NAME .init.data #define __INITDATA_FLAGS "aw",%progbtis #define __initdata __section(__INITDATA_NAME) #define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS Any thoughts on that? Cheers ---Dave
On Thu, 16 Jun 2011, Dave Martin wrote: > Alternatively, we could propose a change <linux/init.h> to split the > __INITDATA macro up, and define the related macros in a sane way: > > #define __INITDATA_NAME .init.data > #define __INITDATA_FLAGS "aw",%progbtis > #define __initdata __section(__INITDATA_NAME) > #define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS > > Any thoughts on that? I do like it. Nicolas
On Sun, Jun 19, 2011 at 11:13:46PM -0400, Nicolas Pitre wrote: > On Thu, 16 Jun 2011, Dave Martin wrote: > > > Alternatively, we could propose a change <linux/init.h> to split the > > __INITDATA macro up, and define the related macros in a sane way: > > > > #define __INITDATA_NAME .init.data > > #define __INITDATA_FLAGS "aw",%progbtis > > #define __initdata __section(__INITDATA_NAME) > > #define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS > > > > Any thoughts on that? > > I do like it. I think that all this data (the cache, tlb and processor funcs tables, and the name strings) should theoretically go in __INITRODATA, since it is not written and is only referenced via .proc.info.init so is not findable after init data has been discarded anyway. This data is all copied by setup.c during startup, AFAICT. However, the current section assignment situation is somewhat inconsistent. * processor_functions, cache_fns: __INITDATA (writable) * tlb_fns: various (tlb-v3.S has __INITDATA; tlb-v7.S has __INIT) * name strings: .rodata (not initdata) I don't really understand how this stuff interacts with CPU hotplug however; maybe this stuff should all be in __CPUINITRODATA instead. Since I will likely patch all the proc-*.S and cache-*.S files, I will try to avoid any potential breakage in the initial series; so I'll keep the section directives out of the macros so that we can prove in an automated way that there is zero change the generated binaries. This still brings much of the benefits. Moving the section directives in the macros could possibly occur as a separate follow-up. Cheers ---Dave
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S index 34261f9..3c5ffbb 100644 --- a/arch/arm/mm/proc-macros.S +++ b/arch/arm/mm/proc-macros.S @@ -4,6 +4,7 @@ * VMA_VM_FLAGS * VM_EXEC */ +#include <linux/init.h> #include <asm/asm-offsets.h> #include <asm/thread_info.h> @@ -254,3 +255,81 @@ mcr p15, 0, r0, c7, c10, 1 @ clean L1 D line mcr p15, 0, ip, c7, c10, 4 @ data write barrier .endm + +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0 + .pushsection .text + + __INITDATA + + .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 + + .if \nommu + .word 0 + .else + .word cpu_\name\()_set_pte_ext + .endif + + .if \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 + + .popsection +.endm + +.macro define_proc_names name:req, arch:req, elf:req, cpu:req + .pushsection .rodata.str, "aMS", 1 + + .type \name\()_arch_name, #object +\name\()_arch_name: .asciz "\arch" + .size \name\()_arch_name, . - \name\()_arch_name + + .type \name\()_elf_name, #object +\name\()_elf_name: .asciz "\elf" + .size \name\()_elf_name, . - \name\()_elf_name + + .type \name\()_cpu_name, #object +\name\()_cpu_name: .asciz "\cpu" + .size \name\()_cpu_name, . - \name\()_cpu_name + + .popsection +.endm + +.macro define_cache_functions name:req + .pushsection .text + + __INITDATA + + .type \name\()_cache_fns, #object +ENTRY(\name\()_cache_fns) + .long \name\()_flush_icache_all + .long \name\()_flush_kern_cache_all + .long \name\()_flush_user_cache_all + .long \name\()_flush_user_cache_range + .long \name\()_coherent_kern_range + .long \name\()_coherent_user_range + .long \name\()_flush_kern_dcache_area + .long \name\()_dma_map_area + .long \name\()_dma_unmap_area + .long \name\()_dma_flush_range + .size \name\()_cache_fns, . - \name\()_cache_fns + + .popsection +.endm
This patch adds some generic macros to reduce boilerplate when declaring certain common structures in arch/arm/mm/*.S Thanks to Russell King for outlining what the define_processor_functions macro could look like. Signed-off-by: Dave Martin <dave.martin@linaro.org> --- arch/arm/mm/proc-macros.S | 79 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 79 insertions(+), 0 deletions(-)