On Sun, Jan 13, 2013 at 09:37:08PM -0800, Yinghai Lu wrote:
> > Question: if the bootloader sets ext_* properly, is it going to set
> > sentinel to 0 so that it can signal to the code further on that ext_*
> > are valid?
>
> old bootloaders have no idea of sentinel, but if they initialize boot_param
> properly that new sentinel will be 0 and new kernel will know.
>
> >
> > This is kinda missing from the mechanism of the sentinel and it should
> > be documented too.
>
> No, we should have too much duplicated info.
This is not a complicated info - it should explain the basic mechanism
of the sentinel.
> >> -v7: change to 0x1ef instead of 0x1f0, HPA said:
> >> it is quite plausible that someone may (fairly sanely) start the
> >> copy range at 0x1f0 instead of 0x1f1
> >
> > Right, all those -vX notes are all important and should *definitely* be
> > at least in the commit message.
>
> No, I want to keep them in order to track the reviewing progress.
Are you saying "no" just for the fun of it? Or do you have a general
aversion to documenting your code?
Give me *one* good reason where having a short, concise and clear
comment which helps people understand what the intent of the mechanism
is a bad thing.
[ … ]
> >> diff --git a/arch/x86/boot/compressed/cmdline.c
> >> b/arch/x86/boot/compressed/cmdline.c
> >> index b4c913c..bffd73b 100644
> >> --- a/arch/x86/boot/compressed/cmdline.c
> >> +++ b/arch/x86/boot/compressed/cmdline.c
> >> @@ -17,6 +17,8 @@ static unsigned long get_cmd_line_ptr(void)
> >> {
> >> unsigned long cmd_line_ptr = real_mode->hdr.cmd_line_ptr;
> >>
> >> + cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
> >> +
> >> return cmd_line_ptr;
> >> }
> >
> > On 32-bit, this unsigned long cmd_line_ptr is 4 bytes and the OR doesn't
> > have any effect on the final result. You probably want to do:
>
> yes, that is what we want to keep 32bit and 64bit unified.
>
> >
> > #ifdef CONFIG_64BIT
> > cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
> > #endif
> >
> > right?
> >
> > Or instead look at ->sentinel to know whether the ext_* fields are valid
> > or not, and save yourself the OR if not.
>
> no.
>
> that is whole point of sentinel, we don't need to check sentinel everywhere
> because ext_* are valid.
Dude, do you even read my comments? This line:
cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
doesn't do a whit on 32-bit. So execute it *only* on 32-bit!
[ … ]
> >> diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
> >> index 03c0683..9333d37 100644
> >> --- a/arch/x86/boot/setup.ld
> >> +++ b/arch/x86/boot/setup.ld
> >> @@ -13,6 +13,13 @@ SECTIONS
> >> .bstext : { *(.bstext) }
> >> .bsdata : { *(.bsdata) }
> >>
> >> + /* sentinel: make sure if boot_params from bootloader is right */
> >
> > This should say:
> >
> > /*
> > * The bootloader signals the validity of the three ext_* boot
> > params with this.
> > */
>
> no, bootloader does not signal that.
> old bootloaders have no idea of sentinel, but if they initialize boot_param
> properly that new sentinel will be 0 and new kernel will know.
So say
* A RECENT bootloader signals the validity of the three ext_* boot
params with this.
but say something.
Understand this (and we've been chewing this same shit for two weeks
now): you need to document your code and you need to document it
properly for other people to understand what you're doing. I'm not
talking about writing an essay or whatever - I'm talking about helpful
comments placed where it makes most sense so that others can understand
the mechanism.
[ … ]
> >> + __u16 xloadflags;
> >> +#define CAN_BE_LOADED_ABOVE_4G (1<<0)
> >> __u32 cmdline_size;
> >> __u32 hardware_subarch;
> >> __u64 hardware_subarch_data;
> >> @@ -106,7 +108,10 @@ struct boot_params {
> >> __u8 hd1_info[16]; /* obsolete! */ /* 0x090 */
> >> struct sys_desc_table sys_desc_table; /* 0x0a0 */
> >> struct olpc_ofw_header olpc_ofw_header; /* 0x0b0 */
> >> - __u8 _pad4[128]; /* 0x0c0 */
> >> + __u32 ext_ramdisk_image; /* 0x0c0 */
> >> + __u32 ext_ramdisk_size; /* 0x0c4 */
> >> + __u32 ext_cmd_line_ptr; /* 0x0c8 */
> >> + __u8 _pad4[116]; /* 0x0cc */
> >> struct edid_info edid_info; /* 0x140 */
> >> struct efi_info efi_info; /* 0x1c0 */
> >> __u32 alt_mem_k; /* 0x1e0 */
> >> @@ -115,7 +120,9 @@ struct boot_params {
> >> __u8 eddbuf_entries; /* 0x1e9 */
> >> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */
> >> __u8 kbd_status; /* 0x1eb */
> >> - __u8 _pad6[5]; /* 0x1ec */
> >> + __u8 _pad5[3]; /* 0x1ec */
> >> + __u8 sentinel; /* 0x1ef */
> >> + __u8 _pad6[1]; /* 0x1f0 */
> >
> > This needs the -v7 explanation from above as a comment here or somewhere
> > around here, for why we've chosen 0x1ef offset.
>
> no, there is no such comment for other fields there.
That's why I f*cking said "here or somewhere around here"! Or put
it somewhere else altogether, if you don't like it here but PUT IT
SOMEWHERE!
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
