__setup()'s not processed in bk-current

__setup()'s not processed in bk-current

[Ricky Beam]

Bootdata ok (command line is ro console=ttyS0,115200 debug numa=off md=d0,0,2,0,
/dev/sda,/dev/sdb,/dev/sdc,/dev/sdd root=/dev/hdd1)
...
Built 1 zonelists
Kernel command line: ro console=ttyS0,115200 debug numa=off md=d0,0,2,0,/dev/sda
,/dev/sdb,/dev/sdc,/dev/sdd root=/dev/hdd1
Parsing ARGS: ro console=ttyS0,115200 debug numa=off md=d0,0,2,0,/dev/sda,/dev/s
db,/dev/sdc,/dev/sdd root=/dev/hdd1
parse_one(): params[0]: 8250.share_irqs
parse_one(): params[1]: 8250.probe_rsa
parse_one(): params[2]: scsi_mod.scsi_logging_level
parse_one(): params[3]: scsi_mod.max_luns
parse_one(): params[4]: scsi_mod.max_report_luns
parse_one(): params[5]: scsi_mod.inq_timeout
parse_one(): params[6]: scsi_mod.dev_flags
parse_one(): params[7]: scsi_mod.default_dev_flags
parse_one(): params[8]: usbcore.blinkenlights
parse_one(): params[9]: usbcore.usbfs_snoop
parse_one(): params[10]: ehci_hcd.log2_irq_thresh
parse_one(): params[11]: ohci_hcd.power_switching
parse_one(): params[12]: mousedev.xres
parse_one(): params[13]: mousedev.yres
parse_one(): params[14]: atkbd.set
parse_one(): params[15]: atkbd.reset
parse_one(): params[16]: atkbd.softrepeat
parse_one(): params[17]: atkbd.scroll
parse_one(): params[18]: atkbd.extra
parse_one(): params[19]: psmouse.proto
parse_one(): params[20]: psmouse.resolution
parse_one(): params[21]: psmouse.rate
parse_one(): params[22]: psmouse.smartscroll
parse_one(): params[23]: psmouse.resetafter
parse_one(): params[24]: i8042.noaux
parse_one(): params[25]: i8042.nomux
parse_one(): params[26]: i8042.unlock
parse_one(): params[27]: i8042.reset
parse_one(): params[28]: i8042.direct
parse_one(): params[29]: i8042.dumbkbd
Unknown argument: calling ffffffff805713c0
obsolete_checksetup(): line: console=ttyS0,115200
obsolete_checksetup(): checking: nosmp(5)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: H<83><EC>^XH<89>|H<8D>t$^TH<8D>|<E8>HQ<CD><FF>
<85><C0>t$HcD$^TH<C7><C7>+^N;<80><C7>^EN<D6><FA><FF>^A(47)
obsolete_checksetup(): checking: debug(5)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: H<89>=<91>k<F9><FF><BA>^A(9)
obsolete_checksetup(): checking: initcall_debug(14)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <NULL>(0)
obsolete_checksetup(): checking: H<83><ECH<C7><C6>^E<B6>:<80>H<85><FF>H^OE<F7>1
<C0>H<C7><C7>!<B6>:<80><E8><B0><D8>^B(31)
obsolete_checksetup(): checking: load_ramdisk=(13)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <80>?(2)
obsolete_checksetup(): checking: root=(5)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <NULL>(1)
obsolete_checksetup(): checking: <B8>^A(2)
...
obsolete_checksetup(): checking: log_buf_len=(12)
...

Odd that it sees "log_buf_len=", but not the "console=" that's defined right
ahead of it:
(kernel/printk.o)
Contents of section .init.data:
 0000 636f6e73 6f6c653d 006c6f67 5f627566  console=.log_buf
 0010 5f6c656e 3d00                        _len=.

init/main.o:
Contents of section .init.data:
 0000 6e6f736d 70006d61 78637075 733d0070  nosmp.maxcpus=.p
 0010 726f6669 6c653d00 64656275 67007175  rofile=.debug.qu
 0020 69657400 696e6974 3d000000 00000000  iet.init=.......
 0030 00000000 00000000 00000000 00000000  ................
...
 0140 696e6974 63616c6c 5f646562 75670074  initcall_debug.t
 0150 65737473 65747570 00746573 74736574  estsetup.testset
 0160 75705f6c 6f6e6700 00000000           up_long.....

The linked kernel *looks* correct (x86_64 here):
Contents of section .init.data:
 ffffffff8058d000 6e6f736d 70006d61 78637075 733d0070  nosmp.maxcpus=.p

Contents of section .init.setup:
 ffffffff80593510 00d05880 ffffffff 60125780 ffffffff  ..X.....`.W.....
 ffffffff80593520 00000000 00000000 00000000 00000000  ................
 ffffffff80593530 06d05880 ffffffff 70125780 ffffffff  ..X.....p.W.....
 ffffffff80593540 00000000 00000000 00000000 00000000  ................

Do I smell some bad pointer math?  Yeap:
DEBUG: sizeof(obs_kernel_param): 24 (0x18)

obsolete_checksetup(): line: ro
obsolete_checksetup(): checking: nosmp(5) @ ffffffff80593510
obsolete_checksetup(): checking: <NULL>(1) @ ffffffff80593528

p++ moved the pointer sizeof(obs_kernel_param) ahead, but that's 8 bytes
short.

--Ricky

Re: __setup()'s not processed in bk-current

[Ricky Beam]

On Mon, 28 Jun 2004, Ricky Beam wrote:
>The linked kernel *looks* correct (x86_64 here):
>Contents of section .init.data:
> ffffffff8058d000 6e6f736d 70006d61 78637075 733d0070  nosmp.maxcpus=.p
>
>Contents of section .init.setup:
> ffffffff80593510 00d05880 ffffffff 60125780 ffffffff  ..X.....`.W.....
> ffffffff80593520 00000000 00000000 00000000 00000000  ................
> ffffffff80593530 06d05880 ffffffff 70125780 ffffffff  ..X.....p.W.....
> ffffffff80593540 00000000 00000000 00000000 00000000  ................
>
>Do I smell some bad pointer math?  Yeap:
>DEBUG: sizeof(obs_kernel_param): 24 (0x18)

Ahh.  Not bad pointer math.  Bad kernel source :-)  The compiler is not
aware of the packing/alignment of the .init.setup section until link
time which is too late.  The .init.setup section is ALIGN(16)'d for
almost all archs.  (h8300 is 4 bytes.)

There may be other instances like this that'll eventually bite us in the
ass.

(A) Fix... at least for x86_64.

===== include/linux/init.h 1.33 vs edited =====
--- 1.33/include/linux/init.h   2004-06-27 03:19:38 -04:00
+++ edited/include/linux/init.h 2004-06-28 18:39:37 -04:00
@@ -107,11 +107,12 @@
        static initcall_t __initcall_##fn \
        __attribute_used__ __attribute__((__section__(".security_initcall.init"))) = fn

+/* FIXME: not all arch's are aligned on a 16byte boundry */
 struct obs_kernel_param {
        const char *str;
        int (*setup_func)(char *);
        int early;
-};
+} __attribute__((aligned(16)));

 /* Only for really core code.  See moduleparam.h for the normal way. */
 #define __setup_param(str, unique_id, fn, early)                       \

===== arch/x86_64/kernel/vmlinux.lds.S 1.23 vs edited =====
--- 1.23/arch/x86_64/kernel/vmlinux.lds.S       2004-05-30 07:33:25 -04:00
+++ edited/arch/x86_64/kernel/vmlinux.lds.S     2004-06-28 19:20:20 -04:00
@@ -87,7 +87,7 @@
        _einittext = .;
   }
   .init.data : { *(.init.data) }
-  . = ALIGN(16);
+  . = ALIGN(32);
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;

(don't ask me why __setup_start is landing 16b less than it should)

--Ricky

Re: __setup()'s not processed in bk-current

[Andrew Morton]

Ricky Beam <jfbeam@bluetronic.net> wrote:
>
> Do I smell some bad pointer math?  Yeap:
>  DEBUG: sizeof(obs_kernel_param): 24 (0x18)
> 
>  obsolete_checksetup(): line: ro
>  obsolete_checksetup(): checking: nosmp(5) @ ffffffff80593510
>  obsolete_checksetup(): checking: <NULL>(1) @ ffffffff80593528
> 
>  p++ moved the pointer sizeof(obs_kernel_param) ahead, but that's 8 bytes
>  short.

Thanks for working that out.  It's been handing around for ages.



We're now putting 24-byte structures into .init.setup via __setup.  But
x86_64's compiler is emitting a `.align 16' in there, so they end up on
32-byte boundaries and do_early_param()'s pointer arithmetic goes wrong.

Fix that up by forcing the compiler to align these structures to sizeof(long).



---

 25-akpm/include/linux/init.h |    1 +
 1 files changed, 1 insertion(+)

diff -puN include/linux/init.h~x86_64-setup-section-alignment-fix include/linux/init.h
--- 25/include/linux/init.h~x86_64-setup-section-alignment-fix	2004-06-28 16:47:41.000000000 -0700
+++ 25-akpm/include/linux/init.h	2004-06-28 16:47:41.000000000 -0700
@@ -119,6 +119,7 @@ struct obs_kernel_param {
 	static struct obs_kernel_param __setup_##unique_id	\
 		 __attribute_used__				\
 		 __attribute__((__section__(".init.setup")))	\
+		__attribute__((aligned((sizeof(long)))))	\
 		= { __setup_str_##unique_id, fn, early }
 
 #define __setup_null_param(str, unique_id)			\

_

Re: __setup()'s not processed in bk-current

[Rusty Russell]

On Tue, 2004-06-29 at 09:57, Andrew Morton wrote:
> We're now putting 24-byte structures into .init.setup via __setup.  But
> x86_64's compiler is emitting a `.align 16' in there, so they end up on
> 32-byte boundaries and do_early_param()'s pointer arithmetic goes wrong.
> 
> Fix that up by forcing the compiler to align these structures to sizeof(long).

Um, that's really odd, and at least deserves a comment.

There are a number of places where we assume that we can iterate through
all entries in a section as an array, rth would know if we've just been
lucky...

Thanks,
Rusty.

> diff -puN include/linux/init.h~x86_64-setup-section-alignment-fix include/linux/init.h
> --- 25/include/linux/init.h~x86_64-setup-section-alignment-fix	2004-06-28 16:47:41.000000000 -0700
> +++ 25-akpm/include/linux/init.h	2004-06-28 16:47:41.000000000 -0700
> @@ -119,6 +119,7 @@ struct obs_kernel_param {
>  	static struct obs_kernel_param __setup_##unique_id	\
>  		 __attribute_used__				\
>  		 __attribute__((__section__(".init.setup")))	\
> +		__attribute__((aligned((sizeof(long)))))	\
>  		= { __setup_str_##unique_id, fn, early }
>  
>  #define __setup_null_param(str, unique_id)			\
> 
> _
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: __setup()'s not processed in bk-current

[Richard Henderson]

On Tue, Jun 29, 2004 at 12:43:41PM +1000, Rusty Russell wrote:
> There are a number of places where we assume that we can iterate through
> all entries in a section as an array, rth would know if we've just been
> lucky...

You've been lucky.


r~